test: allow custom gasLimit in ERC20 withdrawAndCall E2E test#3972
test: allow custom gasLimit in ERC20 withdrawAndCall E2E test#3972ws4charlie merged 7 commits intodevelopfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a configurable gas limit argument to three ERC20 withdrawal end-to-end tests. The corresponding test functions are updated to accept and process the new gas limit parameter. The ZEVM runner's Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant ZEVMRunner
participant ZEVMGateway
TestRunner->>ZEVMRunner: TestERC20WithdrawAndCall(amount, gasLimit)
ZEVMRunner->>ZEVMGateway: ERC20WithdrawAndCall(receiver, amount, payload, revertOptions, gasLimit)
ZEVMGateway-->>ZEVMRunner: Transaction result
ZEVMRunner-->>TestRunner: Assertion and result
This diagram illustrates the updated flow where the test runner passes both the amount and the gas limit to the ZEVM runner, which then forwards the gas limit to the ZEVM gateway during the withdrawal and call operation. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🔭 Outside diff range comments (1)
e2e/runner/zevm.go (1)
225-233:⚠️ Potential issueGracefully handle nil or zero
gasLimitparameter
ERC20WithdrawAndCallnow trusts external callers to pass a non-nil, sensiblegasLimit.
Make the API fool-proof by falling back to the default when the argument is nil or zero:func (r *E2ERunner) ERC20WithdrawAndCall( … gasLimit *big.Int, ) *ethtypes.Transaction { - // this function take more gas than default 500k + if gasLimit == nil || gasLimit.Sign() == 0 { + gasLimit = newDefaultGasLimit() + } // this function take more gas than default 500kThe same guard would be useful in
ETHWithdrawAndCall.
♻️ Duplicate comments (2)
e2e/e2etests/test_erc20_withdraw_and_call_no_message.go (1)
16-26: Duplicate validation logicSame
gasLimitsanity check as suggested forTestERC20WithdrawAndCallshould be added here to keep the tests consistent.e2e/e2etests/test_erc20_withdraw_revert_and_abort.go (1)
17-22: ValidategasLimitinputApply the same range validation to avoid flaky abort tests caused by malformed CLI arguments.
🧹 Nitpick comments (3)
e2e/runner/zevm.go (1)
201-208: Comment is outdated – “default 500k” vs 250 000The comment states “default 500k” while the code uses
defaultGasLimit(250 000).
Update the remark to avoid confusion for future maintainers.e2e/e2etests/test_erc20_withdraw_and_call.go (1)
16-26: Consider validating numeric range ofgasLimitargument
require.Lenensures presence, but an out-of-range value (e.g., negative or extremely big) will only surface at execution time. A quick sanity check prevents wasted CI cycles:require.True(r, gasLimit.Sign() > 0 && gasLimit.Uint64() <= 15_000_000, "gasLimit must be between 1 and 15M, got %s", gasLimit)e2e/e2etests/e2etests.go (1)
490-493: Argument description can be clearerTo align wording with the ETH equivalents (lines 359-370) consider:
- {Description: "gas limit for withdraw and call", …} + {Description: "gas limit (ZEVM execution)", …}Minor, but improves UX for CLI users who scroll through
--help.Also applies to: 500-502, 525-527
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/e2etests/e2etests.go(3 hunks)e2e/e2etests/test_erc20_withdraw_and_call.go(3 hunks)e2e/e2etests/test_erc20_withdraw_and_call_no_message.go(3 hunks)e2e/e2etests/test_erc20_withdraw_revert_and_abort.go(2 hunks)e2e/runner/zevm.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_erc20_withdraw_revert_and_abort.goe2e/e2etests/test_erc20_withdraw_and_call_no_message.goe2e/e2etests/test_erc20_withdraw_and_call.goe2e/e2etests/e2etests.goe2e/runner/zevm.go
🧬 Code Graph Analysis (4)
e2e/e2etests/test_erc20_withdraw_revert_and_abort.go (1)
e2e/utils/parsing.go (1)
ParseBigInt(27-32)
e2e/e2etests/test_erc20_withdraw_and_call_no_message.go (1)
e2e/utils/parsing.go (1)
ParseBigInt(27-32)
e2e/e2etests/test_erc20_withdraw_and_call.go (1)
e2e/utils/parsing.go (1)
ParseBigInt(27-32)
e2e/runner/zevm.go (1)
e2e/contracts/gatewayzevmcaller/GatewayZEVMCaller.go (1)
CallOptions(33-36)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint
- GitHub Check: gosec
- GitHub Check: build-and-test
- GitHub Check: build-zetanode
- GitHub Check: analyze (go)
- GitHub Check: build
🔇 Additional comments (1)
e2e/runner/zevm.go (1)
117-118: Use fresh instances instead of sharingdefaultGasLimitEven if you keep the global constant pointer, each of these call-sites should defensively clone the value:
- gatewayzevm.CallOptions{GasLimit: defaultGasLimit, IsArbitraryCall: true}, + gatewayzevm.CallOptions{GasLimit: new(big.Int).Set(defaultGasLimit), IsArbitraryCall: true},Otherwise any unexpected mutation (see previous comment) propagates to all transactions.
Also applies to: 165-168, 215-216, 264-265, 284-285, 307-308
3 participants
Description
The
eth_withdraw_and_calltakes two argumentsamountandgasLimit. Similarly, use same arguments for theerc20_withdraw_and_callE2E test so that they can be adjusted in the CI nightly tests.How Has This Been Tested?
Summary by CodeRabbit