Add E2E tests for state override functionality on eth_call#875
Conversation
WalkthroughIntroduces state and block override field updates (e.g., random→prevRandao, coinbase→feeRecipient), adjusts types for state overrides, enhances EstimateGas logging, and adds a new eth_call state overrides test suite. Test fixtures/ABI updated to expose deployer/balance. Numerous test expectation updates reflect new gas, code size, and trace values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Web3 client
participant API as API (eth_call/eth_estimateGas)
participant Req as Requester
participant BP as OverridableBlocksProvider
participant EVM as EVM
Client->>API: eth_call/estimateGas(args, stateOverrides, blockOverrides)
note right of API: Marshal & log blockOverrides<br/>with prevRandao/feeRecipient
API->>Req: WithStateOverrides(State, StateDiff, Balance)
API->>Req: WithBlockOverrides(FeeRecipient, PrevRandao, ...)
Req->>BP: Build BlockContext overrides
BP-->>Req: BlockContext with PrevRandao/FeeRecipient
Req->>EVM: DryRun/Estimate with overrides
EVM-->>Req: Result (gas/value/trace)
Req-->>API: Response
API-->>Client: JSON-RPC result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
m-Peter
force-pushed
the
mpeter/test-state-overrides-fix
branch
2 times, most recently
from
9c69f5e to
c0cc563
Compare
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
m-Peter
changed the title
eth_call
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/web3js/build_evm_state_test.js (1)
74-76: Fix self-transfer loop condition (currently enforces the opposite).The loop stops only when receiver === sender, forcing self-transfers.
Apply:
- while (receiver.address != sender.address) { - receiver = randomItem(accounts) - } + while (receiver.address === sender.address) { + receiver = randomItem(accounts) + }
🧹 Nitpick comments (18)
tests/web3js/build_evm_state_test.js (1)
198-199: Encode the correct function for blockTime test.You’re calling blockNumber() twice; second should target blockTime().
- let blockTimeData = deployed.contract.methods.blockNumber().encodeABI() + let blockTimeData = deployed.contract.methods.blockTime().encodeABI()tests/web3js/eth_pectra_upgrade_test.js (1)
102-109: Declare res to avoid accidental global leakage.Some runners use strict mode; implicit globals can bite across tests.
- res = await helpers.signAndSend({ + let res = await helpers.signAndSend({tests/web3js/eth_filter_endpoints_test.js (2)
471-477: New pending tx hash expectation updated.If this proves flaky across engine bumps, consider asserting presence/shape over exact hash for the COA companion tx.
- assert.equal( - txHashes[1], - '0x336083e875f62e7c78ecdff3ffa3ab0eba1774eb08a75995b819ff7be9d61429' - ) + assert.match(txHashes[1], /^0x[0-9a-f]{64}$/)
523-541: COA tx deepEqual is brittle; assert stable invariants instead.Hash/value tend to shift; keep strict checks on from/to/gasPrice/value, but avoid pinning tx hash unless required.
- assert.deepEqual(transactions[1], expectedCoaTx) + const coa = transactions[1] + assert.equal(coa.from, '0x0000000000000000000000030000000000000000') + assert.equal(coa.to, '0x658Bdf435d810C91414eC09147DAA6DB62406379') + assert.equal(coa.gasPrice, '0x1') + assert.equal(coa.value, '0x388e84') + assert.match(coa.hash, /^0x[0-9a-f]{64}$/)tests/fixtures/storage.sol (2)
14-14: Consider constant for deployer to avoid a storage slot.Saves one slot and reduces bytecode size; getter can return the constant.
- address deployer = 0xea02F564664A477286B93712829180be4764fAe2; + address constant DEPLOYER = 0xea02F564664A477286B93712829180be4764fAe2;And:
- function getDeployer() public view returns (address) { - return deployer; - } + function getDeployer() public pure returns (address) { + return DEPLOYER; + }
98-103: Tighten revert messages (remove trailing spaces).Minor polish; avoids string-mismatch surprises in tests.
- require(ok, "unsuccessful call to arch "); + require(ok, "unsuccessful call to arch"); ... - require(ok, "unsuccessful call to arch"); + require(ok, "unsuccessful call to arch");Also applies to: 105-110
tests/web3js/eth_transaction_type_fees_test.js (3)
30-31: Reduce brittleness in initial sender balance checksHard-coding exact pre-test balances is fragile across upstream changes. Prefer asserting deltas from observed balances (you already compute expectedBalance post-tx) and avoid asserting the initial absolute value.
Example (outside this hunk):
// Drop the initial strict equality; rely on the delta checks that follow.Also, the error message checks elsewhere hardcode “150”. Consider deriving from conf.minGasPrice to avoid drift:
// outside this hunk assert.include(e.message, `the minimum accepted gas price for transactions is: ${conf.minGasPrice.toString()}`)Also applies to: 75-76, 132-133
69-71: Coinbase balance: assert fee deltas instead of absolute totalsAbsolute coinbase balances can change if any prior test or protocol reward logic shifts. Suggest asserting that coinbase increased by the tx fee, not the exact final total.
Example (outside this hunk):
const before = await web3.eth.getBalance(conf.coinbase) // ... send tx ... const after = await web3.eth.getBalance(conf.coinbase) assert.equal(after - before, res.receipt.gasUsed * expectedPrice)Also applies to: 126-128, 224-226
52-55: Avoid relying on latest block index for tx lookupsUsing getTransactionFromBlock(latest, 0) is order-sensitive. Fetch by hash to avoid flakiness.
Example (outside this hunk):
const tx = await web3.eth.getTransaction(res.receipt.transactionHash) assert.equal(tx.gasPrice, gasPrice)tests/web3js/debug_traces_test.js (6)
31-34: Use Number for length assertions to avoid BigInt/type pitfallschai’s lengthOf expects a numeric length; comparing to BigInt may break under strict comparison or plugin changes. Use numbers.
Example (outside this hunk):
assert.lengthOf(txTrace.returnValue, 10236) assert.lengthOf(txTrace.input, 10454) assert.lengthOf(txTrace.output, 10236)Also applies to: 39-40, 66-67
85-101: Custom JS tracer histogram is very brittle; assert key presence or ranges insteadExact opcode counts can change with compiler or minor bytecode tweaks. Assert a subset and/or minimum counts instead of deep equality of the entire map.
Example (outside this hunk):
const hist = response.body.result ;['PUSH1','PUSH2','SSTORE','SLOAD'].forEach(k => assert.property(hist, k)) assert.isAtLeast(hist.SSTORE ?? 0, 1)Also applies to: 654-684
160-169: prestateTracer exact balances are fragileThese hex balances encode exact fee transfers. Consider asserting structure (keys, nonce increments) and/or computing expected deltas from gasUsed*price rather than hard-coded full balances.
Also applies to: 164-165
197-226: erc7562Tracer opcode counts updatedCounts reflect the larger code path; assertions look internally consistent. Keep in mind the brittleness note above for histograms.
241-248: TraceBlockByNumber/Hash: avoid hard-coding txHash and absolute gas where possibleThe strict txHash matches are sensitive to any upstream hashing changes. If the goal is shape validation, consider asserting:
- there are two traces,
- first is CALL to contract with expected selector/logs,
- second is coinbase fee transfer with expected value,
rather than exact txHash literals.Also applies to: 250-251, 269-275, 293-301, 316-323, 325-326, 344-349, 368-376
572-573: prestateTracer: prefer structural assertions over exact balanceSame brittleness concern: assert nonce progression and presence of account entries; avoid literal balance hex unless strictly required.
tests/web3js/eth_state_overrides_test.js (3)
31-48: Prefer strict equality in all result checksThese assertions compare strings; using assert.strictEqual avoids accidental type coercion and keeps tests robust.
If desirable, switch assert.equal(...) to assert.strictEqual(...) across these expectations.
Also applies to: 65-80, 96-113, 130-145, 160-180, 209-246, 263-280, 298-313, 327-331
6-11: Use const for values not reassignedMinor readability: variables such as deployed, contractAddress, callRetrieve, callGetDeployer, eoaAddr, callGetBalance are never reassigned where declared.
Example:
- let deployed = await helpers.deployContract('storage') - let contractAddress = deployed.receipt.contractAddress - let callRetrieve = deployed.contract.methods.retrieve().encodeABI() - let callGetDeployer = deployed.contract.methods.getDeployer().encodeABI() + const deployed = await helpers.deployContract('storage') + const contractAddress = deployed.receipt.contractAddress + const callRetrieve = deployed.contract.methods.retrieve().encodeABI() + const callGetDeployer = deployed.contract.methods.getDeployer().encodeABI()Also applies to: 146-153, 165-173, 183-189, 314-321
183-331: Optional: add a code-only override caseFor the non-existent address flow you always provide code+state/balance. Adding a simple "code only" override (no storage/balance) would tighten coverage of override semantics.
I can draft the extra case if helpful.
📜 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 (13)
go.mod(1 hunks)tests/e2e_web3js_test.go(1 hunks)tests/fixtures/storage.byte(1 hunks)tests/fixtures/storage.sol(4 hunks)tests/fixtures/storageABI.json(1 hunks)tests/go.mod(1 hunks)tests/web3js/build_evm_state_test.js(1 hunks)tests/web3js/debug_traces_test.js(23 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(4 hunks)tests/web3js/eth_filter_endpoints_test.js(2 hunks)tests/web3js/eth_pectra_upgrade_test.js(2 hunks)tests/web3js/eth_state_overrides_test.js(1 hunks)tests/web3js/eth_transaction_type_fees_test.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/web3js/build_evm_state_test.js (3)
tests/web3js/eth_deploy_contract_and_interact_test.js (3)
code(31-31)web3(4-4)contractAddress(8-8)tests/web3js/eth_state_overrides_test.js (4)
code(185-185)web3(4-4)contractAddress(8-8)contractAddress(186-186)tests/web3js/config.js (1)
web3(2-2)
tests/web3js/eth_state_overrides_test.js (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (9)
helpers(3-3)web3(4-4)deployed(7-7)contractAddress(8-8)callRetrieve(45-45)initValue(44-44)stateOverrides(239-245)response(246-249)code(31-31)
tests/web3js/eth_deploy_contract_and_interact_test.js (3)
tests/web3js/eth_pectra_upgrade_test.js (1)
gasEstimate(75-84)tests/web3js/build_evm_state_test.js (1)
result(179-179)tests/web3js/eth_state_overrides_test.js (2)
result(13-13)result(191-191)
🔇 Additional comments (24)
go.mod (1)
16-16: flow-go bump verified: go mod tidy & verify passed in both root and teststests/go.mod (1)
14-14: Tests module alignment verified Tests/go.mod’s github.com/onflow/flow-go version matches the root module; tidy and verify succeeded.tests/web3js/build_evm_state_test.js (2)
168-168: Updated gas estimate assertion looks correct.
175-175: Updated runtime code length assertion looks correct.tests/web3js/eth_pectra_upgrade_test.js (3)
130-132: Trace gas deltas acknowledged.Numbers reflect upstream changes; assertion matches.
182-182: debug_traceCall gasUsed delta acknowledged.
190-190: Inner STATICCALL gas delta acknowledged.tests/web3js/eth_deploy_contract_and_interact_test.js (4)
21-21: Updated deployment gasUsed expectation looks consistent.
236-236: Updated estimateGas at 'latest' acknowledged.
277-277: eth_estimateGas with state overrides: updated hex result acknowledged.
299-299: State-diff zero-to-nonzero gas delta expectation updated; looks good.tests/fixtures/storage.byte (1)
1-1: Pin solc v0.8.30 and regenerate storage fixture
Pin compilation to solc 0.8.30 and recompilestorage.solwithsolc --bin-runtime, updatetests/fixtures/storage.byteto the new bytecode (≈10236 bytes, tail marker …0081e0033), and verify the runtime code hash matchesstorage.sol.tests/fixtures/storageABI.json (2)
165-183: ABI: New getBalance view getter looks correctInputs/outputs and mutability align with the intended usage in tests. No issues spotted.
184-196: ABI: New getDeployer view getter looks correctSignature and mutability are consistent with the Solidity intent for a simple address getter.
tests/web3js/eth_transaction_type_fees_test.js (1)
24-26: Deployment receipt gasUsed expectation updatedThe new 1200498n matches the rest of this PR’s gas deltas. Looks good.
tests/web3js/debug_traces_test.js (8)
63-68: callTracer top-level gas/gasUsed/input/output updates are consistentValues align with the higher deployment gas and bytecode size. No issues.
129-131: callTracer gas/gasUsed for state-changing tx looks goodHex values match the structLog totals in this block. No concerns.
413-415: verifyArchCallToFlowBlockHeight callTracer values updatedNew gas/gasUsed pair is coherent with other gas shifts. Looks good.
465-466: traceCall gas/gasUsed expectations updated; okNumbers align with the new bytecode. No issues.
Also applies to: 491-496
607-608: erc7562Tracer gasUsed updated; consistentNo issues.
710-711: traceCall with stateOverrides: gasUsed updated; override behavior validatedOutput reflects overridden storage (0x3e8). This is the core of the PR—nice.
791-799: Flow block height traceCall gas/gasUsed changes; consistent with prior shiftsLooks correct.
839-839: Sum traceCall gasUsed updatedChange is consistent; assertions remain valid.
tests/e2e_web3js_test.go (1)
77-80: LGTM: new subtest wiring is correctThe new "apply state overrides" subtest is placed logically after deployment/interaction and before storage-slot checks. No issues spotted.
m-Peter
force-pushed
the
mpeter/test-state-overrides-fix
branch
from
c0cc563 to
fa6fa4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (20)
tests/web3js/eth_filter_endpoints_test.js (2)
475-477: Reduce brittleness: avoid pinning the COA tx hash literalThe COA transfer hash can change with minor upstream behavior (gas/ordering). Prefer validating shape and distinctness from the user tx instead of a fixed value.
- assert.equal( - txHashes[1], - '0x336083e875f62e7c78ecdff3ffa3ab0eba1774eb08a75995b819ff7be9d61429' - ) + assert.isTrue( + web3.utils.isHexStrict(txHashes[1]) && txHashes[1].length === 66, + 'COA tx hash must be a 32-byte hex string' + ) + assert.notEqual( + txHashes[1], + res.receipt.transactionHash, + 'COA tx must be distinct from the user transaction' + )
529-535: Relax expectations for dynamic COA fields (hash/value) in full-tx assertionsExact COA hash/value are environment-sensitive. Keep the strong assertions for stable fields; bind hash/value to the observed tx to avoid flakiness.
- hash: '0xc5ee4679ca0cce3b964a60e8d789ca2c97236c8d86dc15a3f4fd5e402a6089da', + hash: transactions[1].hash, @@ - value: '0x388e84', + value: transactions[1].value,Optionally, also assert they look sane:
assert.isTrue(web3.utils.isHexStrict(transactions[1].hash) && transactions[1].hash.length === 66) assert.isTrue(web3.utils.toBN(transactions[1].value).gtn(0))api/debug.go (2)
182-189: Pass through BlockOverrides wholesale to avoid silently dropping fieldsOnly Number/Time/FeeRecipient/PrevRandao are forwarded. Passing the struct directly makes this future-proof (e.g., BaseFeePerGas, BlobBaseFee, GasLimit, BeaconRoot, Withdrawals), while the provider can choose which to honor.
- if config.BlockOverrides != nil { - blocksProvider = blocksProvider.WithBlockOverrides(ðTypes.BlockOverrides{ - Number: config.BlockOverrides.Number, - Time: config.BlockOverrides.Time, - FeeRecipient: config.BlockOverrides.FeeRecipient, - PrevRandao: config.BlockOverrides.PrevRandao, - }) - } + if config.BlockOverrides != nil { + blocksProvider = blocksProvider.WithBlockOverrides(config.BlockOverrides) + }Please confirm which override fields the underlying flow-go BlockContext supports today, and document/validate-err on unsupported ones instead of silently ignoring them.
212-237: State override: consider handling MovePrecompileTo or rejecting it explicitlyOverrideAccount gained MovePrecompileTo, but it’s currently ignored here. That’s a silent no-op which can mislead callers.
If supported upstream, wire it through; otherwise, fail fast:
for addr, account := range *config.StateOverrides { + if account.MovePrecompileTo != nil { + // Either wire a query.WithStateOverrideMovePrecompileTo(...) here, + // or return a clear error to avoid a silent no-op. + return nil, fmt.Errorf("account %s: 'movePrecompileToAddress' is not supported", addr.Hex()) + }Would you like me to open a small follow-up to either implement or explicitly reject this field?
tests/web3js/contract_call_overrides_test.js (1)
90-90: prevRandao rename is correct and aligns with geth semanticsLooks good; thanks for aligning the override key with PrevRandao.
Consider adding a sibling assertion for feeRecipient (coinbase) overrides to cover both renamed fields end-to-end.
services/requester/requester.go (1)
538-551: Type adjustments for overrides look correctSwitching to single-pointer Balance and value maps for State/StateDiff matches the new types. The conversions should be safe with the existing nil-guards.
For consistency with DebugAPI (which uses Balance.ToInt()), you might prefer:
- opts = append(opts, query.WithStateOverrideBalance(addr, (*big.Int)(account.Balance))) + opts = append(opts, query.WithStateOverrideBalance(addr, account.Balance.ToInt()))This reduces pointer-cast friction and reads clearer.
eth/types/types.go (1)
452-462: BlockOverrides renames and additions align with geth; double-check JSON shapesField names (FeeRecipient, PrevRandao, BaseFeePerGas, BlobBaseFee, BeaconRoot, Withdrawals) match current geth nomenclature. Since there are no explicit json tags, confirm the expected camelCase payload keys match what your RPC layer expects (e.g., baseFeePerGas vs baseFee).
If any naming divergences arise, add explicit json tags to lock the wire format.
tests/web3js/debug_traces_test.js (13)
35-44: Reduce brittleness: don’t deepEqual the entire first structLog.Exact gas/gasCost/stack snapshots are sensitive to upstream opcode/gas tweaks. Assert key invariants instead.
- assert.deepEqual( - txTrace.structLogs[0], - { - pc: 0, - op: 'PUSH1', - gas: 1079409, - gasCost: 3, - depth: 1, - stack: [] - } - ) + const first = txTrace.structLogs[0] + assert.equal(first.pc, 0) + assert.equal(first.op, 'PUSH1') + assert.equal(first.depth, 1) + assert.deepEqual(first.stack, []) + assert.isAbove(first.gas, 0)
63-68: Hex gas fields: assert format and invariants, not exact numbers.Comparing exact gas/gasUsed and input/output byte lengths makes tests fragile. Check hex format and gasUsed ≤ gas.
- assert.equal(txTrace.gas, '0x127b39') - assert.equal(txTrace.gasUsed, '0x125172') + assert.match(txTrace.gas, /^0x[0-9a-f]+$/) + assert.match(txTrace.gasUsed, /^0x[0-9a-f]+$/) + { + const g = Number(web3.utils.hexToNumberString(txTrace.gas)) + const gu = Number(web3.utils.hexToNumberString(txTrace.gasUsed)) + assert.isAtLeast(g, gu) + } - assert.lengthOf(txTrace.input, 10454) - assert.lengthOf(txTrace.output, 10236) + assert.isAbove(txTrace.input.length, 0) + assert.isAtLeast(txTrace.output.length, 0)
85-105: Custom JS tracer: assert a stable subset instead of the full histogram.Opcode counts fluctuate with compiler and dependency changes. Validate presence and minimum counts for pivotal ops.
- assert.deepEqual( - txTrace, - { - PUSH1: 3, - MSTORE: 1, - PUSH2: 4, - PUSH0: 4, - DUP2: 3, - SWAP1: 3, - SSTORE: 2, - POP: 2, - PUSH20: 3, - EXP: 1, - SLOAD: 1, - MUL: 2, - NOT: 1, - AND: 2, - DUP4: 1, - OR: 1, - DUP1: 1, - CODECOPY: 1, - RETURN: 1 - } - ) + // Stable subset + assert.includeKeys(txTrace, ['PUSH1','PUSH2','PUSH0','SSTORE','SLOAD','SWAP1','DUP2']) + assert.isAtLeast(txTrace.PUSH1, 1) + assert.isAtLeast(txTrace.SSTORE, 1) + assert.isAtLeast(txTrace.SLOAD, 1)
129-138: Repeatable pattern: replace exact gas/gasUsed with invariant checks.Same rationale as above; suggest extracting a tiny helper to DRY this across the file.
- assert.equal(txTrace.gas, '0x6f84') - assert.equal(txTrace.gasUsed, '0x6e29') + assert.match(txTrace.gas, /^0x[0-9a-f]+$/) + assert.match(txTrace.gasUsed, /^0x[0-9a-f]+$/) + { + const g = Number(web3.utils.hexToNumberString(txTrace.gas)) + const gu = Number(web3.utils.hexToNumberString(txTrace.gasUsed)) + assert.isAtLeast(g, gu) + }Optional local helper (outside this block):
function assertGasInvariant(hexGas, hexGasUsed) { assert.match(hexGas, /^0x[0-9a-f]+$/) assert.match(hexGasUsed, /^0x[0-9a-f]+$/) const g = Number(web3.utils.hexToNumberString(hexGas)) const gu = Number(web3.utils.hexToNumberString(hexGasUsed)) assert.isAtLeast(g, gu) }
160-169: Prestate tracer balances: prefer relational checks over exact values.Exact wei balances (pre/post) will drift with baseFee/feeRecipient changes. Assert nonce delta and that EOA balance decreased, system account increased.
- assert.deepEqual( - txTrace.pre['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'], - { balance: '0x456391823a384734', nonce: 1 } - ) + assert.equal(txTrace.pre['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'].nonce, 1) - assert.deepEqual( - txTrace.post['0x0000000000000000000000030000000000000000'], - { balance: '0x408c06' } - ) + assert.match(txTrace.post['0x0000000000000000000000030000000000000000'].balance, /^0x[0-9a-f]+$/) - assert.deepEqual( - txTrace.post['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'], - { balance: '0x4563918239f7bb2e', nonce: 2 } - ) + assert.equal(txTrace.post['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'].nonce, 2) + { + const pre = BigInt(txTrace.pre['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'].balance) + const post = BigInt(txTrace.post['0xfacf71692421039876a5bb4f10ef7a439d8ef61e'].balance) + assert.isTrue(post <= pre) // fees paid + }
197-226: erc7562Tracer opcode counts: assert presence and types, not exact tallies.Counts for JUMP/JUMPI/SWAP/JUMPDEST vary with compiler and linkers. Validate key opcodes exist and no outOfGas.
- assert.deepEqual( - txTrace, - { - ..., - usedOpcodes: { - '0x56': 10, - '0x57': 8, - '0x5b': 15, - '0xa3': 1 - }, - ... - } - ) + assert.isFalse(txTrace.outOfGas) + assert.containsAllKeys(txTrace.usedOpcodes, ['0x51','0x55','0x56','0x57','0x5b']) + assert.isAtLeast(txTrace.usedOpcodes['0x56'], 1) + assert.isAtLeast(txTrace.usedOpcodes['0x57'], 1)
241-242: De-hardcode tx hashes in traceBlock assertions.Use live receipts to avoid brittle constants.
- assert.equal(txTraces[0].txHash, '0x2a2526cfe1c5533b5debbf7942cb16f25b9e7800c5c33c2d85695256d2fa44a5') + assert.equal(txTraces[0].txHash, receipt.transactionHash) ... - assert.equal(txTraces[0].txHash, '0x2a2526cfe1c5533b5debbf7942cb16f25b9e7800c5c33c2d85695256d2fa44a5') + assert.equal(txTraces[0].txHash, receipt.transactionHash)Also applies to: 316-318, 325-325, 368-369
247-248: Same as earlier: don’t deepEqual entire structLog entry for blocks.Keep op/pc/depth checks; avoid exact gas/gasCost.
- assert.deepEqual( - txTraces[0].result.structLogs[0], - { pc: 0, op: 'PUSH1', gas: 7344, gasCost: 3, depth: 1, stack: [] } - ) + const first = txTraces[0].result.structLogs[0] + assert.equal(first.pc, 0) + assert.equal(first.op, 'PUSH1') + assert.equal(first.depth, 1) + assert.deepEqual(first.stack, []) + assert.isAbove(first.gas, 0)Also applies to: 322-323
264-264: Terminology nit: “fee recipient” vs “coinbase”.Since BlockOverrides renamed Coinbase→FeeRecipient, consider aligning the comment to “fee recipient.”
- assert.lengthOf(txTraces, 2) // the 2nd tx trace is from the transfer of fees to coinbase + assert.lengthOf(txTraces, 2) // the 2nd tx trace is the fee transfer to the fee recipientAlso applies to: 339-339
413-415: VerifyArchCall trace: assert call graph semantics instead of gas figures.Focus on presence of the STATICCALL to the precompile and returned value; avoid pinning gas/gasUsed.
- assert.equal(txTrace.gas, '0xb5c3') - assert.equal(txTrace.gasUsed, '0x619f') + assertGasInvariant(txTrace.gas, txTrace.gasUsed)
465-466: DRY and stabilize gas assertions across traceCall suite.There are many exact gas/gasUsed equals; replace with a single helper (assertGasInvariant) and call it in each place. This keeps tests resilient to upstream gas schedule tweaks.
- assert.equal(updateTrace.gas, 28213) + assert.isAbove(updateTrace.gas, 0) ... - assert.equal(updateTrace.gasUsed, '0x6e35') + assertGasInvariant(updateTrace.gas, updateTrace.gasUsed) ... - assert.equal(callTrace.gasUsed, '0x5bdf') + assertGasInvariant(callTrace.gas, callTrace.gasUsed) ... - assert.equal(txTrace.gasUsed, '0x5bdf') + assertGasInvariant(txTrace.gas, txTrace.gasUsed) ... - assert.equal(callTrace.gas, '0xcdd4') - assert.equal(callTrace.gasUsed, '0xb445') + assertGasInvariant(callTrace.gas, callTrace.gasUsed) ... - assert.equal(updateTrace.gasUsed, '0x6092') + assertGasInvariant(updateTrace.gas, updateTrace.gasUsed)Helper (place near top of file):
function assertGasInvariant(g, gu) { const gas = typeof g === 'number' ? g : Number(web3.utils.hexToNumberString(g)) const used = typeof gu === 'number' ? gu : Number(web3.utils.hexToNumberString(gu)) assert.isAtLeast(gas, used) assert.isAbove(gas, 0) }Also applies to: 491-496, 535-538, 605-608, 708-711, 789-799, 836-839
579-586: Tracer outputs (4byte/erc7562/custom): prefer structural checks.Deep-equality on exact maps is brittle. For 4byte, assert that the selector you expect is present; for erc7562, assert presence of accessedSlots and types; for custom, assert minimum opcode set.
If helpful, I can push a helper to assert subset inclusion for these tracer results.
Also applies to: 593-600, 641-686
46-52: Deduplicate tracer config literals.Create small factory helpers for callTracer/prestateTracer/4byte/erc7562 configs to reduce repeated object literals and future edit surface.
Example:
const makeCallTracer = (withLog=false, onlyTopCall=true) => ({ tracer: 'callTracer', tracerConfig: { withLog, onlyTopCall }, })Also applies to: 439-445, 382-387, 770-777, 811-817
📜 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 (18)
api/debug.go(1 hunks)eth/types/types.go(1 hunks)go.mod(1 hunks)services/requester/overridable_blocks_provider.go(1 hunks)services/requester/requester.go(1 hunks)tests/e2e_web3js_test.go(1 hunks)tests/fixtures/storage.byte(1 hunks)tests/fixtures/storage.sol(4 hunks)tests/fixtures/storageABI.json(1 hunks)tests/go.mod(1 hunks)tests/web3js/build_evm_state_test.js(1 hunks)tests/web3js/contract_call_overrides_test.js(1 hunks)tests/web3js/debug_traces_test.js(23 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(4 hunks)tests/web3js/eth_filter_endpoints_test.js(2 hunks)tests/web3js/eth_pectra_upgrade_test.js(2 hunks)tests/web3js/eth_state_overrides_test.js(1 hunks)tests/web3js/eth_transaction_type_fees_test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/e2e_web3js_test.go
- tests/go.mod
- tests/web3js/eth_transaction_type_fees_test.js
- go.mod
- tests/fixtures/storageABI.json
- tests/web3js/eth_state_overrides_test.js
- tests/fixtures/storage.byte
- tests/web3js/eth_pectra_upgrade_test.js
- tests/web3js/build_evm_state_test.js
- tests/fixtures/storage.sol
- tests/web3js/eth_deploy_contract_and_interact_test.js
🧰 Additional context used
🧬 Code graph analysis (2)
api/debug.go (1)
eth/types/types.go (1)
BlockOverrides(451-462)
eth/types/types.go (1)
models/transaction.go (1)
BaseFeePerGas(32-32)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (4)
services/requester/overridable_blocks_provider.go (1)
57-63: Correctly mapped PrevRandao and FeeRecipient in BlockContextThe rename wiring is right and matches the new API.
If/when flow-go exposes support for GasLimit/BaseFeePerGas/BeaconRoot/Withdrawals overrides in BlockContext, consider extending this mapping accordingly to avoid surprises for callers passing those fields.
eth/types/types.go (1)
439-445: OverrideAccount shape updates look goodDropping the extra pointer layer and adding MovePrecompileTo aligns with recent geth shapes.
If MovePrecompileTo is intended to be user-facing here, ensure RPC docs and server-side handling (debug/eth_call paths) are updated to avoid silent no-ops.
tests/web3js/debug_traces_test.js (2)
692-699: Good coverage for stateOverrides path (CALL with stateDiff).Asserting the overridden storage slot returns 0x3e8 is the right smoke test for the override plumbing. Nice.
730-736: Type safety: ensure getBlockNumber returns BigInt in this env.You subtract 1n; if getBlockNumber ever returns a Number, this throws. Consider coercing explicitly.
- let latestHeight = await web3.eth.getBlockNumber() + let latestHeight = BigInt(await web3.eth.getBlockNumber())
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/web3js/debug_traces_test.js (1)
730-736: Fix potential BigInt/Number mix in block tag arithmetic
latestHeight - 1nwill throw ifgetBlockNumber()returns a Number (common in web3). Make the subtraction type-safe.Apply:
- [traceCall, web3.utils.toHex(latestHeight - 1n), callTracer] + [traceCall, web3.utils.toHex( + typeof latestHeight === 'bigint' ? latestHeight - 1n : latestHeight - 1 + ), callTracer]
♻️ Duplicate comments (1)
tests/web3js/debug_traces_test.js (1)
31-34: Resolved: replaced BigInt with Number inassert.lengthOfNice fix addressing the earlier flakiness with Chai’s
lengthOf. Looks good.Also applies to: 66-67
🧹 Nitpick comments (3)
tests/web3js/debug_traces_test.js (3)
62-65: Avoid hard-coded addresses; assert against runtime valuesUse
conf.eoa.address.toLowerCase()andcontractAddress.toLowerCase()instead of baked literals to reduce brittleness across environments and deployments. This also documents intent better.Example changes:
- assert.equal(txTrace.from, '0xfacf71692421039876a5bb4f10ef7a439d8ef61e') + assert.equal(txTrace.from, conf.eoa.address.toLowerCase()) - assert.equal(txTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab') + assert.equal(txTrace.to, contractAddress.toLowerCase())For places using
deepEqualwith full objects, consider asserting key fields separately (from/to/type/value) to avoid embedding address literals in large expected objects.Also applies to: 128-132, 196-201, 268-276, 345-351, 495-496, 605-609, 709-712
31-41: Reduce brittleness of exact gas/txHash/structLog expectationsExact gas, txHash, and opcode/gas-cost-derived fields are sensitive to upstream changes (e.g., EVM/tracer adjustments, dependency bumps). Prefer structural or relational assertions where possible.
Targeted tweaks:
- Gas/gasUsed: assert hex format and ordering rather than exact values.
- txHash: for the first trace, assert equals the actual receipt hash; for fee-transfer traces, assert hex shape (or derive dynamically), not a fixed literal.
- structLogs: assert presence/shape of first entry and minimal invariants (pc/op/depth), not precise gas numbers.
Illustration:
- assert.equal(txTrace.gas, '0x127b39') - assert.equal(txTrace.gasUsed, '0x125172') + assert.match(txTrace.gas, /^0x[0-9a-f]+$/) + assert.match(txTrace.gasUsed, /^0x[0-9a-f]+$/) + assert.isAtLeast(parseInt(txTrace.gas, 16), parseInt(txTrace.gasUsed, 16)) - assert.equal(txTraces[0].txHash, '0x2a2526cfe1c5533b5debbf7942cb16f25b9e7800c5c33c2d85695256d2fa44a5') + assert.equal(txTraces[0].txHash, receipt.transactionHash) - assert.deepEqual(txTraces[0].result.structLogs[0], - { pc: 0, op: 'PUSH1', gas: 7344, gasCost: 3, depth: 1, stack: [] }) + assert.equal(txTraces[0].result.structLogs[0].pc, 0) + assert.equal(txTraces[0].result.structLogs[0].op, 'PUSH1') + assert.equal(txTraces[0].result.structLogs[0].depth, 1)Similarly consider relaxing exact opcode-count maps (JS tracer / erc7562Tracer) to check key presence and rough shape rather than exact counts, which can drift with compiler/optimizer or tracer changes.
Also applies to: 63-67, 129-131, 198-199, 241-251, 268-305, 316-326, 343-381, 789-805, 835-839
447-455: Be consistent with numeric argument types passed to contract methodsYou mix Number literals (e.g.,
500,1500) and BigInt (100n). For consistency and to avoid accidental Number overflow in future edits, consider using BigInt for these too (web3 supports BigInt).- let callData = deployed.contract.methods.storeWithLog(500).encodeABI() + let callData = deployed.contract.methods.storeWithLog(500n).encodeABI() ... - let updateData = deployed.contract.methods.store(1500).encodeABI() + let updateData = deployed.contract.methods.store(1500n).encodeABI()Also applies to: 720-727
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/web3js/debug_traces_test.js(23 hunks)tests/web3js/eth_state_overrides_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web3js/eth_state_overrides_test.js
⏰ 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). (1)
- GitHub Check: Test
m-Peter
force-pushed
the
mpeter/test-state-overrides-fix
branch
from
32b65ab to
6cda1f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/web3js/debug_traces_test.js (1)
33-33: Replaced BigInt in lengthOf with Number — LGTM.Matches prior guidance; avoids Chai edge cases with BigInt lengths.
Also applies to: 66-67
🧹 Nitpick comments (6)
tests/web3js/estimate_gas_overrides_test.js (2)
81-81: Renamed override keys to prevRandao/feeRecipient — LGTM; tiny naming nit.The payload keys match the new API surface. For clarity, consider renaming the local var
randomtoprevRandaoand using object shorthand.- let random = '0x7914bb5b13bac6f621bc37bbf6e406fbf4472aaaaf17ec2f309a92aca4e27fc0' + let prevRandao = '0x7914bb5b13bac6f621bc37bbf6e406fbf4472aaaaf17ec2f309a92aca4e27fc0' @@ - [txArgs, 'latest', null, { prevRandao: random }] + [txArgs, 'latest', null, { prevRandao }]Also applies to: 99-99
39-39: Avoid mixing Number and BigInt in equality assertions.These asserts compare a Number (hexToNumber) to a BigInt literal. Prefer consistent Number usage to prevent subtle type issues across environments.
- assert.equal(web3.utils.hexToNumber(response.body.result), 21473n) + assert.strictEqual(web3.utils.hexToNumber(response.body.result), 21473) @@ - assert.equal(web3.utils.hexToNumber(response.body.result), 21473n) + assert.strictEqual(web3.utils.hexToNumber(response.body.result), 21473) @@ - assert.equal(web3.utils.hexToNumber(response.body.result), 21473n) + assert.strictEqual(web3.utils.hexToNumber(response.body.result), 21473) @@ - assert.equal(web3.utils.hexToNumber(response.body.result), 273693n) + assert.strictEqual(web3.utils.hexToNumber(response.body.result), 273693)Also applies to: 57-57, 75-75, 103-103
tests/web3js/contract_call_overrides_test.js (1)
90-90: prevRandao key update — LGTM; small var naming clean‑up.The switch to
{ prevRandao: ... }aligns with the new types. For consistency, renamerandom->prevRandaoand use shorthand; also keep comparisons consistent.- let random = '0x7914bb5b13bac6f621bc37bbf6e406fbf4472aaaaf17ec2f309a92aca4e27fc0' + let prevRandao = '0x7914bb5b13bac6f621bc37bbf6e406fbf4472aaaaf17ec2f309a92aca4e27fc0' @@ - [call, 'latest', null, { prevRandao: random }] + [call, 'latest', null, { prevRandao }] @@ - assert.equal(response.body.result, random) + assert.equal(response.body.result, prevRandao)api/api.go (1)
726-729: Early marshal of blockOverrides — OK; optionally tag logs with endpoint sooner.Current early returns log via
b.logger(no endpoint field). Consider initializing a minimal logger before marshaling so errors carry the endpoint tag.func (b *BlockChainAPI) EstimateGas( @@ - stateOverridesArgs, err := json.Marshal(stateOverrides) + l0 := b.logger.With().Str("endpoint", EthEstimateGas).Logger() + stateOverridesArgs, err := json.Marshal(stateOverrides) if err != nil { - return handleError[hexutil.Uint64](err, b.logger, b.collector) + return handleError[hexutil.Uint64](err, l0, b.collector) } blockOverridesArgs, err := json.Marshal(blockOverrides) if err != nil { - return handleError[hexutil.Uint64](err, b.logger, b.collector) + return handleError[hexutil.Uint64](err, l0, b.collector) }tests/web3js/debug_traces_test.js (2)
241-248: Avoid hard‑coding tx hashes in traceBlock asserts to reduce brittleness.Use the receipt’s actual tx hash instead of fixed literals; values can change across builds.
- assert.equal(txTraces[0].txHash, '0x2a2526cfe1c5533b5debbf7942cb16f25b9e7800c5c33c2d85695256d2fa44a5') + assert.equal(txTraces[0].txHash, receipt.transactionHash) @@ - assert.equal(txTraces[1].txHash, '0x34f823d6fcef9cafccf7b15ec97f7e0734b1b97e0b32992d4243d2d580a8d2b4') + assert.notEqual(txTraces[1].txHash, receipt.transactionHash) // fee transfer tx @@ - assert.equal(txTraces[0].txHash, '0x2a2526cfe1c5533b5debbf7942cb16f25b9e7800c5c33c2d85695256d2fa44a5') + assert.equal(txTraces[0].txHash, receipt.transactionHash) @@ - assert.equal(txTraces[1].txHash, '0x34f823d6fcef9cafccf7b15ec97f7e0734b1b97e0b32992d4243d2d580a8d2b4') + assert.notEqual(txTraces[1].txHash, receipt.transactionHash)Also applies to: 250-251, 316-323, 325-326
85-101: Opcode histogram asserts are very strict; consider loosening to key presence.Exact opcode counts may drift with compiler or minor bytecode/layout changes. If flakiness becomes an issue, assert presence/minimum counts instead of exact tallies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
api/api.go(2 hunks)api/debug.go(1 hunks)eth/types/types.go(1 hunks)services/requester/overridable_blocks_provider.go(1 hunks)services/requester/requester.go(1 hunks)tests/e2e_web3js_test.go(1 hunks)tests/fixtures/storage.byte(1 hunks)tests/fixtures/storage.sol(4 hunks)tests/fixtures/storageABI.json(1 hunks)tests/go.mod(2 hunks)tests/web3js/build_evm_state_test.js(1 hunks)tests/web3js/contract_call_overrides_test.js(1 hunks)tests/web3js/debug_traces_test.js(23 hunks)tests/web3js/estimate_gas_overrides_test.js(2 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(4 hunks)tests/web3js/eth_filter_endpoints_test.js(2 hunks)tests/web3js/eth_pectra_upgrade_test.js(2 hunks)tests/web3js/eth_state_overrides_test.js(1 hunks)tests/web3js/eth_transaction_type_fees_test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/e2e_web3js_test.go
- tests/web3js/build_evm_state_test.js
- tests/web3js/eth_state_overrides_test.js
- api/debug.go
- tests/fixtures/storageABI.json
- tests/web3js/eth_transaction_type_fees_test.js
- services/requester/overridable_blocks_provider.go
- tests/web3js/eth_deploy_contract_and_interact_test.js
- tests/web3js/eth_pectra_upgrade_test.js
- tests/web3js/eth_filter_endpoints_test.js
- tests/fixtures/storage.byte
- services/requester/requester.go
- eth/types/types.go
- tests/fixtures/storage.sol
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (3)
api/api.go (1)
741-741: Including blockOverrides in logs — LGTM.This will greatly help debugging override-related estimate variance.
tests/web3js/contract_call_overrides_test.js (1)
84-96: Ensure hex conversion is BigInt-safe (avoid Number overflow).
Replace web3.utils.hexToNumber(response.body.result) with web3.utils.hexToBigInt(...) or web3.utils.hexToNumberString(...) so the 256‑bit prevrandao cannot overflow a JS Number. Automated check couldn't run in the sandbox because 'web3' is not installed — verify locally whether your web3 version's hexToNumber returns a BigInt; if it does not, make the change. File: tests/web3js/contract_call_overrides_test.js lines 84-96.tests/go.mod (1)
10-10: Deps bumped — verify toolchain and upstream compatibility.
- Go: tests/go.mod declares go 1.25.0 and the verification run used go1.25.0 — ensure CI is set to use Go 1.25.x. (tests/go.mod: line 3)
- Pins: tests/go.mod locks github.com/onflow/flow-emulator v1.7.2 (line 10) and github.com/onflow/flow-go-sdk v1.8.2 (line 13); google.golang.org/protobuf v1.36.9 is present indirect (line 250).
- Action required — upstream behavior: verify that flow-emulator v1.7.2 and flow-go-sdk v1.8.2 actually include the block/state override (storage replacement) semantics this PR relies on by checking their release notes/PRs.
- Protobuf: go mod graph shows flow-go@v0.43.0 depends on google.golang.org/protobuf v1.36.7 while tests reference v1.36.9 (indirect); patch-level mismatch is probably safe but confirm generated .pb.go were produced with a compatible protoc/protoc-gen-go to avoid generator/runtime mismatches in tests.
2 participants
Depends on: onflow/flow-go#7737
Closes: #793
Description
Adds E2E tests to verify the correct functionality of state overrides for
eth_callJSON-RPC endpoint.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Refactor
Tests
Chores