Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Gas Mechanics] Implement Gas history; Migrate to Arbitrum Gas model #1714
[Gas Mechanics] Implement Gas history; Migrate to Arbitrum Gas model #1714
Changes from 25 commits
5c99fe1
dcfae70
18b5ebd
3b57b36
d260860
f39a41d
0796abe
2c5d732
51830c7
9610077
6aaa044
d6859a0
4328651
4fa9570
1603add
8e3258f
6328ef1
5af1c41
18f5af0
040433e
4424153
a0f632c
b952cdf
32314bb
b9cb6e6
d4dcd1c
c08dccc
55f68ae
3949823
50bdd25
36c95bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm not quite clear what prevents a user from setting the "GasFeeCap=0", and become a "free transaction"?
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.
The mempool would reject such a transaction. The only way to get it in is bypass the mempool.
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.
The update to the
GetTransactionCount
method includes conditional logic for batch retrieval based on the presence of a third parameter. The error message on line 687 could be more descriptive by including the tag value that caused the error.Committable suggestion
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.
The logic in the
GetTransactionCount
method for retrieving the nonce seems correct. However, the comment on line 706 suggests that there might be a known issue with race conditions that should be addressed. It would be prudent to track this as a TODO for future resolution.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.
The
EstimateGas
function now includes additional calculations for gas estimation, which aligns with the PR's objectives. However, the error messages on lines 1042 and 1049 are generic and do not provide enough context about the failure. It would be beneficial to include more details in the error messages to aid in debugging.Committable suggestion
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.
The
EstimateGas
function now includes additional calculations for gas estimation, which aligns with the PR's objectives. However, the error messages on lines 1042, 1049, and 1055 are generic and do not provide enough context about the failure. It would be beneficial to include more details in the error messages to aid in debugging.Committable suggestion
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.
The logic for converting L1 cost to L2 gas seems to be implemented correctly. However, the comment on line 1060 mentions adding overhead when the base fee becomes dynamic. This should be tracked as a TODO to ensure that the dynamic base fee is handled correctly in the future.