Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace hardcoded tx costs with a call to eth_estimateGas #46

Merged

Conversation

gballet
Copy link
Contributor

@gballet gballet commented Dec 8, 2023

When using tx-fuzz on the verkle testnet, we found some issues caused by the change in the gas schedule. Transactions now have to pay for the witness costs, so call estimateGas in order to find out what the price of a transaction is going to be, before sending it.

Creating this PR as a draft as there is still an open question (see comment).

transactions.go Outdated
conf.gasLimit = gas

// this works because noAlStrategies ⊂ alStrategies
return alStrategies[index](conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to pass the client around so what I did was to get a first transaction generated, have it estimated, and then generate a new one with the same gas. I don't really like this, because these are two different transactions and that could fail.

@gballet gballet changed the title Use estimate gas Replace hardcoded tx costs with a call to eth_estimateGas Dec 8, 2023
Copy link
Owner

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@MariusVanDerWijden MariusVanDerWijden marked this pull request as ready for review January 15, 2024 08:53
@MariusVanDerWijden MariusVanDerWijden merged commit 264171c into MariusVanDerWijden:master Jan 15, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants