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

Add --skip-simulation and --gas-estimate-multiplier cli options to forge script command #2524

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

ckoopmann
Copy link
Contributor

@ckoopmann ckoopmann commented Jul 30, 2022

Motivation

In some edge cases the on-chain simulation currently blocks users from running certain types of transactions, even though they would work fine when submitted on chain.
See #2522

For cases like these it'd be great if the user could bypass on-chain simulation.

Solution

Add command line parameter --skip-simulation that the user can set to disable on chain simulation.
When skipping simulation the gas costs will be estimated through the rpc-provider individually before each transaction submission.

When testing that new functionality the transactions ran out of gas and failed so I added the parameter --gas-estimate-multiplier to manually adjust the factor by which gas estimates are inflated (or reduced).
Default value for this is 130 (meaning gas estimates will be multiplied by 1.3) which was the value previously hardcoded for estimations coming from the on-chain simulation.
For consistency this default will also be applied to the provider estimate in the case the respective chain has a different gas calculation, which was previously used as returned by the provider (see this change), this should be the only change to the existing behaviour (when the user uses none of the new arguments).

@ckoopmann ckoopmann marked this pull request as draft July 30, 2022 08:56
@ckoopmann ckoopmann force-pushed the disable-on-chain-simulation branch from 3baee7d to 6b44708 Compare July 30, 2022 09:47
@onbjerg onbjerg added the T-feature Type: feature label Jul 30, 2022
@onbjerg onbjerg linked an issue Jul 30, 2022 that may be closed by this pull request
@ckoopmann ckoopmann force-pushed the disable-on-chain-simulation branch from 6cea2cc to f1fe4e3 Compare July 31, 2022 03:19
@ckoopmann ckoopmann force-pushed the disable-on-chain-simulation branch from f1fe4e3 to 502a2be Compare July 31, 2022 03:33
@ckoopmann ckoopmann marked this pull request as ready for review August 1, 2022 00:28
@ckoopmann ckoopmann changed the title Add parameter to allow users to skip on-chain simulation when broadcasting scripts Add --skip-simulation and --gas-estimate-multiplier cli options to forge script command Aug 1, 2022
@ckoopmann
Copy link
Contributor Author

ckoopmann commented Aug 2, 2022

@onbjerg: Any feedback / review on this would be greatly appreciated 🙏
(sorry for tagging, but github doesnt let me request a review / assign a reviewer - also didn't see any mentioning of correct etiquette on requesting a review in CONTRIBUTING.md)

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise it looks good to me :) Sorry for the slow turnaround, notifications have been blowing up 📱

cli/src/cmd/forge/script/broadcast.rs Outdated Show resolved Hide resolved
cli/src/cmd/forge/script/broadcast.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg linked an issue Aug 3, 2022 that may be closed by this pull request
2 tasks
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

pending smol nits @ckoopmann

@onbjerg onbjerg merged commit b107a6e into foundry-rs:master Aug 3, 2022
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
…o `forge script` (foundry-rs#2524)

* Add skip-simulation flag to bypass on-chain simulation

* Add gas-estimate-multiplier cli argument

* chore: nits

Co-authored-by: Oliver Nordbjerg <hi@notbjerg.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
3 participants