-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Fix gas report generation #4824
Conversation
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: @nomicfoundation/hardhat-toolbox@4.0.0 |
This should work. |
I like the idea and looks very clean, but I don't like that it's implicit (e.g. is not obvious for a contributor). I need to review first to fully understand the changes. I think it's fine but not preferrable. |
Removed |
hardhat.config.js
Outdated
// "implies" doesn't work correctly. we force gas if gasReport is set | ||
argv.gas ||= !!argv.gasReport; |
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.
Pretty weird it's not working as expected. I'll take a look to understand what's going on here. We shouldn't merge it if we're misinterpreting how it works.
Otherwise, we may remove the implies
.
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.
LGTM but I see the runnings are still not producing the gas comparison. Is it because it's not merged yet?
Yes, gas reports produced by PR are not pushed, because nothing is going to be compared to that. |
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Gas report generaton has been broken for a while.
It looks like the json file is no longer generated by default. Adding an envvar should fix that.
WIP: multiple issues have been identified and are being worked on.