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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix effectiveGasPrice computation for eth_getTransactionReceipt #2610

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

elfedy
Copy link
Contributor

@elfedy elfedy commented Jan 5, 2024

What does it do?

Fixes wrong value for effectiveGasPrice on eth_getTransactionReceipt which is using NextFeeMultiplier from the block the transaction happened vs the NextFeeMultiplier on the previous block which is the actual value used to compute the base fee for the transaction.

Is there something left for follow-up PRs?

There might be other places where gas_price runtime call with the hash of the block is used to get the base fee. Those places should also be updated for the sake of consistency, maybe abstracting the computation of the base fee. Example: https://github.com/moonbeam-foundation/frontier/blob/2f509926db8df03aa6f831d48fcf18a3c456cc49/client/rpc/src/eth/block.rs#L114

What alternative implementations were considered?

eth_getTransactionReceipt rpc call makes a runtime call to moonbeam internally, there is the argument of implementing the fix on moonbeam via that runtime call vs patching frontier.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

moonbeam-foundation/frontier#199

What value does it bring to the blockchain users?

RPC users will get the actual effectiveGasPrice used by the transaction.

@elfedy elfedy added the B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes label Jan 5, 2024
Copy link
Contributor

github-actions bot commented Jan 5, 2024

Coverage Report

@@                    Coverage Diff                    @@
##           master   elfedy-fix-receipt-gas     +/-   ##
=========================================================
  Coverage   80.94%                   80.94%   0.00%     
  Files         287                      287             
  Lines       94330                    94330             
=========================================================
  Hits        76354                    76354             
  Misses      17976                    17976             
Files Changed Coverage

Coverage generated Fri Jan 12 14:03:00 UTC 2024

@elfedy elfedy added the not-breaking Does not need to be mentioned in breaking changes label Jan 5, 2024
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @elfedy, the test looks good to me, and it fail properly.
Now you can use your frontier branch with the fix

@elfedy elfedy added the D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. label Jan 9, 2024
@elfedy elfedy force-pushed the elfedy-fix-receipt-gas branch from 8db3477 to a4f0a6c Compare January 9, 2024 15:01
@elfedy elfedy marked this pull request as ready for review January 12, 2024 13:31
@librelois librelois changed the title Fix effectiveGasPrice computaton for eth_getTransactionReceipt Fix effectiveGasPrice computation for eth_getTransactionReceipt Jan 12, 2024
@elfedy elfedy merged commit 6e83cb7 into master Jan 16, 2024
29 checks passed
@elfedy elfedy deleted the elfedy-fix-receipt-gas branch January 16, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants