test(e2e): improve E2E reliability for tests on live network
#3061
Conversation
lumtis
added
no-changelog
|
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
e2e/runner/v2_zevm.go (1)
Line range hint 134-146: Consider refactoring gas limit management.
The temporary gas limit increase to 10M in V2ERC20WithdrawAndArbitraryCall and V2ERC20WithdrawAndCall suggests these operations require significantly more gas. Consider:
- Document why these specific operations need 40x more gas than the default
- Extract the temporary gas limit modification into a helper function
func withTemporaryGasLimit(r *E2ERunner, tempLimit uint64, fn func() (*ethtypes.Transaction, error)) (*ethtypes.Transaction, error) {
previousLimit := r.ZEVMAuth.GasLimit
r.ZEVMAuth.GasLimit = tempLimit
defer func() {
r.ZEVMAuth.GasLimit = previousLimit
}()
return fn()
}Usage example:
tx, err := withTemporaryGasLimit(r, 10000000, func() (*ethtypes.Transaction, error) {
return r.GatewayZEVM.WithdrawAndCall0(
r.ZEVMAuth,
receiver.Bytes(),
amount,
r.ERC20ZRC20Addr,
payload,
gatewayzevm.CallOptions{GasLimit: gasLimit, IsArbitraryCall: true},
revertOptions,
)
})Also applies to: 169-181
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/runner/v2_zevm.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
e2e/runner/v2_zevm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
e2e/runner/v2_zevm.go (2)
Line range hint 34-52: Well-structured method renaming for clarity.
The renaming from V2ETHWithdrawAndCall to V2ETHWithdrawAndArbitraryCall better reflects the method's purpose and aligns with the IsArbitraryCall: true parameter.
14-14: Document the gas limit rationale and verify sufficiency.
While reducing the gas limit from 1M to 250K aligns with the goal of optimizing costs, please add a comment explaining:
- Why 250K was chosen as the optimal value
- What operations this limit is expected to cover
✅ Verification successful
Based on the search results, I can now verify the gas limit change. Let me analyze the key findings:
-
The gas limit of 250,000 is used consistently in ZEVM-related operations:
- In e2e/runner/v2_zevm.go:
gasLimit = big.NewInt(250000) - Used in various V2 protocol tests for deposits, withdrawals, and arbitrary calls
- In e2e/runner/v2_zevm.go:
-
This value aligns with production gas limits seen in the codebase:
- EVM transfers use 21,000 (zetaclient/chains/evm/constant.go)
- ZRC20 operations use between 100,000 to 400,000 gas
- Complex operations like message passing use up to 400,000 gas
-
The value is well within safe bounds:
- Above minimum (100,000) defined in zetaclient/chains/evm/signer/gas.go
- Below maximum (1,000,000) defined in same file
- Sufficient for standard cross-chain operations
Gas limit of 250K is appropriate and well-documented through usage patterns.
The reduction from 1M to 250K is justified as:
- It's above the minimum required for complex operations
- Provides sufficient headroom for standard transactions
- Aligns with production gas limits seen across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential gas-related issues or discussions in the codebase
rg -i "gas.*limit|insufficient.*gas" --type go
Length of output: 110723
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
|
@fbac pushed some further changes if you want to take a look |
lumtis
changed the title
e2e): reduce gas limit used for withdraw and call testse2e): improve E2E reliability for tests on live network
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3061 +/- ##
===========================================
- Coverage 64.47% 64.33% -0.14%
===========================================
Files 412 419 +7
Lines 28985 29102 +117
===========================================
+ Hits 18687 18723 +36
- Misses 9515 9597 +82
+ Partials 783 782 -1 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Current test that interacts with TestDappV2 during withdrawAndCall tests specify 1M as gas limit.
This makes the cost of tx very expensive on live networks. This PR reduces this number to a smaller value sufficient to run the tests.
The random string for payload allow having tests that have no shared context between each other and therefore run those independently
Summary by CodeRabbit
New Features
Bug Fixes