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
feat: add gas limit configuration #1009
Conversation
Need to add project tests still which is a WIP, I know I'll at least be adding in functional tests that configure a project with a gas limit and then manually call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I added some mostly small feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool, looking forward to testing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I added some pretty small feedbacks.
Feel free to ping me again if you address any of it and neeed a re-approval
eth_config = config.get_config("ethereum") | ||
|
||
assert eth_config.rinkeby.gas_limit == "auto" | ||
assert eth_config.local.gas_limit == "auto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 0.6 or any future release, we may want to consider making max
the default for local
.
I think it will give speed improvements to running test suites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, and what's funny is I thought previously I had local set to max
but I can't seem to find trace of that now 🤔 it was before that pre-audit work so I think I lost a bit of track with the gear switching
I may need to revisit this for #957 but I'll see about including such changes in that PR if need be 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to make it use max now, that is probably fine. It should not affect anything really so maybe not a real breaking change
Co-authored-by: Juliya Smith <yingthi@live.com>
Co-authored-by: Juliya Smith <yingthi@live.com>
Co-authored-by: Juliya Smith <yingthi@live.com>
Co-authored-by: Juliya Smith <yingthi@live.com>
Co-authored-by: Juliya Smith <yingthi@live.com>
Co-authored-by: Juliya Smith <yingthi@live.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed one
What I did
fixes: #985
Added
gas_limit
as an available parameter forNetworkConfig
(and subsequentlyNetworkAPI
)How to verify it
WIP
Checklist