fix(zetaclient): evm chains: use gasLimit from CCTX#3680
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 📝 WalkthroughWalkthroughThe changes remove predefined gas limit constants and simplify gas limit validation logic across the EVM chain code. The constant Changes
Sequence Diagram(s)sequenceDiagram
participant TX as "Transaction Request"
participant GasHandler as "gasFromCCTX Function"
participant Signer as "Transaction Signer"
TX->>GasHandler: Provide gas limit from context
alt Gas limit exceeds maxGasLimit
GasHandler->>TX: Return adjusted gas limit (maxGasLimit)
else
GasHandler->>TX: Return provided gas limit
end
TX->>Signer: Sign transaction with finalized gas limit
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
|
swift1337
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3680 +/- ##
===========================================
- Coverage 64.59% 64.58% -0.01%
===========================================
Files 470 470
Lines 32935 32927 -8
===========================================
- Hits 21275 21267 -8
Misses 10692 10692
Partials 968 968
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
zetaclient/chains/evm/signer/gas.go (1)
69-75: Gas limit validation simplified to only check upper bound.The validation logic has been streamlined to only enforce a maximum gas limit, removing the previous minimum gas limit check. This change effectively addresses the PR objective of using the gas limit from CCTX instead of hardcoded values, particularly for Arbitrum's variable behavior.
Consider adding a comment explaining the design decision to not enforce a minimum gas limit, making it explicit that the caller is responsible for providing an adequate value, as mentioned in the PR objectives:
if limit > maxGasLimit { limit = maxGasLimit logger.Warn(). Uint64("cctx.initial_gas_limit", params.CallOptions.GasLimit). Uint64("cctx.gas_limit", limit). Msgf("Gas limit is too high; Setting to the maximum (%d)", maxGasLimit) } + // No minimum gas limit is enforced - the caller is responsible for providing an adequate gas value
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
zetaclient/chains/evm/common/constant.go(0 hunks)zetaclient/chains/evm/signer/gas.go(2 hunks)zetaclient/chains/evm/signer/gas_test.go(1 hunks)zetaclient/chains/evm/signer/outbound_data_test.go(1 hunks)zetaclient/chains/evm/signer/sign.go(0 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/chains/evm/common/constant.go
- zetaclient/chains/evm/signer/sign.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/evm/signer/gas.gozetaclient/chains/evm/signer/gas_test.go
🔇 Additional comments (5)
zetaclient/chains/evm/signer/gas.go (1)
13-13: Renamed constant clarifies system constraints.With the removal of
minGasLimit, the codebase now only enforces a maximum gas limit, aligning with the approach to use gas limits from the CCTX directly.zetaclient/chains/evm/signer/outbound_data_test.go (1)
50-50: Test assertion updated to use dynamic gas limit.The test now correctly verifies that the gas limit in the outbound data matches the one from the CCTX parameters, reflecting the shift from hardcoded minimum values to dynamic limits from the transaction context.
zetaclient/chains/evm/signer/gas_test.go (3)
32-33: Test case updated with standardized gas limit value.The pre-London gas test now uses a standardized gas limit of 21,000 rather than a value related to the removed minimum gas limit constant. This change properly reflects the new approach of using the gas limit directly from the CCTX.
44-45: Post-London gas test updated with standardized gas limit.Similar to the pre-London gas test, this test case has been updated to use the standardized gas limit of 21,000, aligning with the new gas limit handling approach.
24-93:Details
✅ Verification successful
Test cases simplified by removing minimum gas limit checks.
The test suite has been appropriately updated by removing the test cases that verified behavior related to gas limits being too low, as this validation is no longer performed in the code. The remaining test cases sufficiently cover the current functionality.
This change aligns with the PR objective of relying on the gas limit specified in the CCTX rather than enforcing minimum values, particularly for Arbitrum's variable behavior.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to minimum gas limits across the codebase that might need to be updated echo "Searching for any remaining references to minimum gas limits..." rg -i "min.*gas.*limit" --type go echo "Checking for any hardcoded gas limit values in EVM chain code..." rg -A 2 -B 2 "(21000|21_000|100000|100_000)" --type go zetaclient/chains/evm/Length of output: 2566
Approved: Removal of Minimum Gas Limit Checks Confirmed
The gas test suite changes align with the PR objective. Our verification shows that the test file in
zetaclient/chains/evm/signer/gas_test.gonow focuses solely on the intended behaviors (legacy vs. post-London logic, ceiling enforcement on high gas limits, and proper error handling) without any checks enforcing a minimum gas limit. References to minimum gas limits in other parts of the repository are unrelated to this test and were not intended to be covered by these changes.
- The removed test cases for gas limits being too low are correct since that validation has been moved.
- The remaining tests verify that the gas limit specified in the CCTX is correctly used, particularly for Arbitrum’s variable behavior.
3 participants
Description
Hardcoding evm gas limit for simple transfers doesn't work anymore due to variable arbitrum behavior.
So we can increase gas limit in arbitrum gas zrc20, and use that instead. We don't need minimum gas limit check anymore, we can just use gas limit provided in callOptions.gasLimit.
For erc20s zrc20s it is 100k already and for gas tokens 21k, with exception of arbitrum where we will bump to 100k on smart contract level.
For contracts it would be caller responsibility to put adequate value.
How Has This Been Tested?
Summary by CodeRabbit
Refactor
Bug Fixes
Tests