Fix gas estimation insufficiency for EIP-7702 transactions
#921
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughAdds transaction code authorizations to dry-run/gas-estimate call sites, updates tests to use eth_estimateGas and adjust transaction gas/destinations, and bumps Flow-related dependency versions in module files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant DryCall
participant Executor
rect rgb(245, 250, 255)
Note over Gateway,DryCall: Dry-run now receives Code Authorizations
Client->>Gateway: Request eth_estimateGas / submit tx
Gateway->>DryCall: DryCall(tx.Data(), tx.SetCodeAuthorizations(), tx.Value(), ...)
DryCall->>Executor: Execute dry-run with code-auth context
Executor-->>DryCall: Gas usage result
DryCall-->>Gateway: Return gas estimate
Gateway-->>Client: Use returned gas for tx submission
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/web3js/eth_pectra_upgrade_test.js (1)
60-60: Consider using gas estimation instead of hardcoding.The hardcoded gas value
63779ncould become outdated if gas calculation logic changes. Since this test validates pre-Pectra rejection behavior rather than gas correctness, consider either:
- Using
eth_estimateGasto dynamically determine the gas value, or- Adding a comment explaining the hardcoded value's origin
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
api/debug.go(1 hunks)go.mod(1 hunks)services/requester/requester.go(1 hunks)tests/go.mod(1 hunks)tests/web3js/eth_eip_7702_sending_transactions_test.js(5 hunks)tests/web3js/eth_pectra_upgrade_test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T17:20:28.143Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
Applied to files:
tests/go.mod
🧬 Code graph analysis (1)
tests/web3js/eth_eip_7702_sending_transactions_test.js (6)
tests/web3js/eth_pectra_upgrade_test.js (11)
conf(2-2)require(1-1)require(6-6)require(7-7)require(8-8)helpers(3-3)web3(4-4)eoa(11-11)authorization(47-50)response(118-121)hash(22-22)tests/web3js/eth_deploy_contract_and_interact_test.js (5)
conf(2-2)require(1-1)helpers(3-3)web3(4-4)response(281-284)tests/web3js/debug_traces_test.js (6)
conf(2-2)require(1-1)helpers(3-3)web3(4-4)response(22-25)response(457-460)tests/web3js/contract_call_overrides_test.js (6)
conf(2-2)require(1-1)helpers(3-3)web3(4-4)response(35-38)response(125-128)tests/web3js/eth_eip_7702_contract_write_test.js (10)
require(1-1)require(2-2)require(3-3)require(4-4)eoa(7-7)authorization(43-46)authorization(106-109)hash(18-18)hash(49-54)hash(112-117)tests/web3js/viem/config.js (5)
require(1-1)require(2-2)require(3-3)relay(24-24)walletClient(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (8)
api/debug.go (1)
239-247: LGTM! Authorization list correctly added to trace path.The addition of
tx.SetCodeAuthorizations()ensures thatdebug_traceCallproperly accounts for EIP-7702 SetCode authorizations during dry-run execution, which is essential for accurate tracing of EIP-7702 transactions.services/requester/requester.go (1)
570-578: LGTM! Core fix for EIP-7702 gas estimation.The addition of
tx.SetCodeAuthorizations()to theDryCallinvocation ensures that gas estimation correctly accounts for SetCode authorization processing. This addresses the PR's primary objective: makingeth_estimateGasapply the authorization list during dry-run execution, yielding accurate gas estimates for EIP-7702 transactions.Note that lines 439-441 properly add the per-authorization gas overhead (
CallNewAccountGas) on top of the base dry-run estimate.tests/go.mod (1)
10-12: LGTM! Test dependencies properly aligned.The version bumps for
flow-emulatorandflow-goensure the test environment has the updated DryCall signature and maintains consistency with the main module's dependencies.tests/web3js/eth_eip_7702_sending_transactions_test.js (5)
6-8: LGTM! Imports added for gas estimation functionality.The added imports provide the necessary utilities for calling
eth_estimateGasvia RPC and performing hex conversions for the authorization list parameters.
52-77: LGTM! Gas estimation validation correctly implemented.The test properly constructs transaction arguments including the authorization list, calls
eth_estimateGas, and validates the result. This confirms that the fix correctly accounts for EIP-7702 SetCode authorizations during gas estimation.The expected value
63779nprovides a regression test to catch future gas calculation changes.
87-87: LGTM! Test uses estimated gas for transaction execution.Using the estimated gas value validates that the estimation is not only correct numerically but also sufficient for actual transaction execution, confirming the fix works end-to-end.
142-177: LGTM! Self-executing test correctly implements gas estimation and address handling.The gas estimation pattern mirrors the first test, and the change to
relay.addressas the transaction target is correct for self-executing EIP-7702 transactions where the sender designates the contract onto their own address.
212-212: LGTM! Correct target address for post-designation transaction.After self-executing designation, the ping transaction correctly targets
relay.address, which now has the contract code designated onto it.
go.mod
Outdated
| github.com/onflow/atree v0.11.0 | ||
| github.com/onflow/cadence v1.8.3 | ||
| github.com/onflow/flow-go v0.44.0-experimental-cadence-v1.8.3.0.20251111112227-097f521427a5 | ||
| github.com/onflow/flow-go v0.44.0-experimental-cadence-v1.8.3.0.20251114081035-3a1a77607d8a |
There was a problem hiding this comment.
is there a tagged version for this yet?
There was a problem hiding this comment.
Looking at https://github.com/onflow/flow-go/tags, there doesn't seem to be any tagged version for the v0.44 release/feature branch.
I did update the version to the latest commit on that branch, as I was still using the commit from a PR.
There was a problem hiding this comment.
Update: I have used the newly tagged version of v0.44, in 80bd06b .
m-Peter
force-pushed
the
mpeter/dry-call-apply-set-code-authorizations
branch
from
929a78f to
f51e9ef
Compare
m-Peter
force-pushed
the
mpeter/dry-call-apply-set-code-authorizations
branch
from
f51e9ef to
80bd06b
Compare
Depends on: onflow/flow-go#8148
Closes: #920
Description
For the gas estimation to work properly for
EIP-7702transactions, we must apply any givenSetCodeAuthorizationlist toDryCall.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.