-
Notifications
You must be signed in to change notification settings - Fork 275
refactor: use time MS in gas fee calcs #1804
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
Conversation
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.
Confirmed that:
- all of the changes in the original
coreth
PR that are relevant tosubnet-evm
are included. subnet-evm
specific changes, particularly ingas_limit.go
(due to still using the windower mechanism) appear correct.
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.
just a small comment, mostly lgtm
upon @ceyonur's recommendation, added |
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 the future, I strongly prefer keeping sync PRs 1 to 1 for the sake of reviewers. It may leave some PRs in an intermediate state individually, but they're must easier to compare and ensure all of the intended changes are included properly.
That being said, I was able to review the individual commit and this LGTM, there is just one place I commented that we may have missed in coreth
.
I agree, I forgot that those changes were in another PR. |
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.
there is another place to be changed
Why this should be merged
Roughly syncs ava-labs/coreth#1288 and ava-labs/coreth#1329, just to maintain identical structure throughout the repo.
How this works
Same changes, except for the ACP-176 one. This is NOT a cherry-pick, since every line would have conflicted due to the feeConfig.
How this was tested
Existing UT.
Need to be documented?
No
Need to update RELEASES.md?
No