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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't use provider gas estimate for optimism #2016

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

chad-js
Copy link
Contributor

@chad-js chad-js commented Jun 17, 2022

Motivation

Context provided here: #2002. This change ensures that gas estimation is not underestimated for Optimism.

Solution

Following the solution to ensure we don't calculate gas differently for Optimism, as explained here by @mds1 and @joshieDo (thanks for the help! 馃槃) #2002 (comment). I am only addressing the first point in this changeset.

To test, successfully ran the same script with the same contract and args as specified in the issue on anvil forks of optimism and optimism kovan. Also successfully ran a separate deploy script that was previously failing on prod optimism.

@chad-js chad-js changed the title fix: use provider gas estimate for optimism fix: don't use provider gas estimate for optimism Jun 17, 2022
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.

ty

@mds1 this should be ok, right?

@chad-js
Copy link
Contributor Author

chad-js commented Jun 17, 2022

just renamed my commit lol realized I named incorrectly

@mds1
Copy link
Collaborator

mds1 commented Jun 17, 2022

Yep this is perfect, good to merge IMO

Thanks @chad-js!

@mattsse mattsse added C-forge Command: forge Cmd-forge-create Command: forge create Cmd-forge-script Command: forge script labels Jun 17, 2022
@mattsse mattsse merged commit f59861e into foundry-rs:master Jun 17, 2022
0xca11 pushed a commit to 0xca11/foundry that referenced this pull request Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-create Command: forge create Cmd-forge-script Command: forge script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants