Check the returned error code on empty revert data#880
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughAdds Web3.js tests that verify eth_estimateGas, eth_call, and debug_traceCall behavior for empty and non-empty revert data, and asserts block-height–specific gas estimates. No production API or exported/public declaration changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant RPC as EVM RPC Node
participant Contract as Deployed Contract
Note over Test,RPC: Requests: eth_estimateGas / eth_call / debug_traceCall
Test->>RPC: eth_estimateGas / eth_call (storeButRevert(150)) at block '0x1' / 'latest'
RPC->>Contract: execute call (with state-overrides if present)
Contract-->>RPC: revert (empty data or non-empty revert payload)
RPC-->>Test: eth_call/error (innerError.code=3, innerError.data='0x' or hex, message='execution reverted')
RPC-->>Test: debug_traceCall result (gas, failed=true, returnValue, structLogs)
rect rgba(0,128,0,0.06)
Note right of RPC: Tests assert error shape, block-specific gas estimates, and trace fields
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (1)
246-269: Exact gas constants may be brittle across node/config variants.Hardcoding
22026n(block '0x1') and25052n('latest') can drift with EVM changes or config. Consider asserting relative behavior (e.g., different values between heights) or gating exact values behind a fixture/env flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/web3js/eth_deploy_contract_and_interact_test.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (2)
tests/web3js/config.js (1)
web3(2-2)tests/web3js/helpers.js (2)
web3(5-5)conf(2-2)
⏰ 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/check-error-code-with-empty-revert-data
branch
from
cb4656e to
a06f41c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/web3js/debug_traces_test.js (3)
881-885: Deflake: don’t hard‑code gas or structLogs length for empty‑revert traceGas and structLogs counts are sensitive to EVM/tracer changes and can cause brittle tests. The PR’s intent is validating failed=true and empty revert data; keep those, relax the rest.
- assert.equal(traceResult.gas, 26678) assert.equal(traceResult.failed, true) assert.equal(traceResult.returnValue, '0x') - assert.lengthOf(traceResult.structLogs, 138) + assert.isNumber(traceResult.gas) + assert.isArray(traceResult.structLogs) + assert.isAtLeast(traceResult.structLogs.length, 1)
903-907: Match custom error by selector, not full ABI payload; relax gas/lengthComparing the entire revert payload is brittle (contract text, ABI layout, padding). Assert the 4‑byte selector and basic shape instead; also avoid pinning gas/structLogs length.
- assert.equal(traceResult.gas, 21808) assert.equal(traceResult.failed, true) - assert.equal(traceResult.returnValue, '0x9195785a00000000000000000000000000000000000000000000000000000000000000050000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000001056616c756520697320746f6f206c6f7700000000000000000000000000000000') - assert.lengthOf(traceResult.structLogs, 215) + assert.isTrue(traceResult.returnValue.startsWith('0x9195785a')) // custom error selector + assert.isAbove(traceResult.returnValue.length, 10) // has encoded args/message + assert.isNumber(traceResult.gas) + assert.isArray(traceResult.structLogs) + assert.isAtLeast(traceResult.structLogs.length, 1)If you want stronger checks without brittleness, we can parse the payload to verify the string argument equals “Value is too low” (no new deps) by slicing/decoding the hex. Want me to add that helper?
863-872: Reduce duplication: small builder for traceCall envelopesYou re-create the same traceCall envelope multiple times. A tiny local helper keeps the test shorter and easier to tweak.
+ const makeTraceCall = (data, gas = '0x95ab') => ({ + from: conf.eoa.address, + to: contractAddress, + data, + value: '0x0', + gasPrice: web3.utils.toHex(conf.minGasPrice), + gas, + }) - traceCall = { - from: conf.eoa.address, - to: contractAddress, - data: callStoreButRevert, - value: '0x0', - gasPrice: web3.utils.toHex(conf.minGasPrice), - gas: '0x95ab' - } + traceCall = makeTraceCall(callStoreButRevert) ... - traceCall = { - from: conf.eoa.address, - to: contractAddress, - data: callCustomError, - value: '0x0', - gasPrice: web3.utils.toHex(conf.minGasPrice), - gas: '0x95ab' - } + traceCall = makeTraceCall(callCustomError)Also applies to: 887-895
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web3js/eth_deploy_contract_and_interact_test.js
🧰 Additional context used
🧬 Code graph analysis (1)
tests/web3js/debug_traces_test.js (2)
tests/web3js/eth_deploy_contract_and_interact_test.js (5)
deployed(7-7)conf(2-2)contractAddress(8-8)web3(4-4)helpers(3-3)tests/web3js/config.js (1)
web3(2-2)
⏰ 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/check-error-code-with-empty-revert-data
branch
from
a06f41c to
3342b8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/web3js/debug_traces_test.js (3)
877-885: Avoid brittle assertions: relax exact gas and structLogs checks.Gas and structLogs length can shift with compiler/EVM changes; assert invariants instead.
- assert.isDefined(response.body) + assert.isDefined(response.body.result) - let traceResult = response.body.result - assert.equal(traceResult.gas, 26677) + let traceResult = response.body.result + assert.isAbove(traceResult.gas, 0) assert.equal(traceResult.failed, true) assert.equal(traceResult.returnValue, '0x') - assert.lengthOf(traceResult.structLogs, 138) + assert.isArray(traceResult.structLogs) + assert.isAbove(traceResult.structLogs.length, 0)
903-907: Don’t assert the full revert payload; check selector/pattern (and optionally decode).Full ABI payloads are sensitive to encoding/solc changes. Assert the selector and presence of data instead; optionally decode to verify arguments.
- traceResult = response.body.result - assert.equal(traceResult.gas, 21786) - assert.equal(traceResult.failed, true) - assert.equal(traceResult.returnValue, '0x9195785a00000000000000000000000000000000000000000000000000000000000000050000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000001056616c756520697320746f6f206c6f7700000000000000000000000000000000') - assert.lengthOf(traceResult.structLogs, 210) + traceResult = response.body.result + assert.isAbove(traceResult.gas, 0) + assert.equal(traceResult.failed, true) + assert.isTrue(traceResult.returnValue.startsWith('0x9195785a')) // selector + assert.isAbove(traceResult.returnValue.length, 10) // has encoded args + assert.isArray(traceResult.structLogs) + assert.isAbove(traceResult.structLogs.length, 0)Optional: decode and assert arguments with web3.eth.abi if you want stronger checks without hard‑coding the blob.
862-899: Consider splitting into dedicated tests for revert scenarios.Keeping these in a separate “it(...)” per case improves isolation and failure signals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web3js/eth_deploy_contract_and_interact_test.js
🧰 Additional context used
🧬 Code graph analysis (1)
tests/web3js/debug_traces_test.js (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (9)
callStoreButRevert(216-216)callStoreButRevert(233-233)deployed(7-7)conf(2-2)contractAddress(8-8)web3(4-4)helpers(3-3)callCustomError(110-110)callCustomError(180-180)
⏰ 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 (1)
tests/web3js/debug_traces_test.js (1)
862-907: Good coverage for empty vs. non‑empty revert trace paths.Adding struct‑logger checks for both cases tightens compatibility with geth behavior and the PR objective.
m-Peter
force-pushed
the
mpeter/check-error-code-with-empty-revert-data
branch
from
3342b8d to
5e926de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/web3js/eth_deploy_contract_and_interact_test.js (2)
214-229: Consider usingconstfor the immutable call data variable.Line 216 declares
callStoreButRevertwithletbut never reassigns it. For clarity and to prevent accidental mutations, consider usingconst.Apply this diff:
- let callStoreButRevert = deployed.contract.methods.storeButRevert(150n).encodeABI() + const callStoreButRevert = deployed.contract.methods.storeButRevert(150n).encodeABI()
231-246: Consider usingconstfor the immutable call data variable.Line 233 declares
callStoreButRevertwithletbut never reassigns it. For clarity and to prevent accidental mutations, consider usingconst.Apply this diff:
- let callStoreButRevert = deployed.contract.methods.storeButRevert(150n).encodeABI() + const callStoreButRevert = deployed.contract.methods.storeButRevert(150n).encodeABI()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web3js/debug_traces_test.js
🧰 Additional context used
🧬 Code graph analysis (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (1)
tests/web3js/debug_traces_test.js (4)
callStoreButRevert(864-864)deployed(6-6)web3(4-4)conf(2-2)
⏰ 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 (1)
tests/web3js/eth_deploy_contract_and_interact_test.js (1)
248-271: LGTM! Block height handling is correctly tested.The tests properly validate that gas estimation accounts for contract existence at different block heights, with clear comments explaining the intent.
2 participants
Work towards: #840
Description
Make sure that we return the proper error code for contract calls / gas estimations that revert without any explicit revert reason.
Ref: ethereum/go-ethereum#31456
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit