fix: blocked gateway cctx for high-gas deposit #3106
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements and fixes across the ZetaChain node, including a new feature for whitelisting SPL tokens on Solana, improvements in build reproducibility, and the addition of concurrent withdrawal and deposit tests for the TON blockchain. A critical fix prevents blocked cross-chain transactions during "out of gas" errors. New smart contract functionalities and corresponding test cases have also been implemented, enhancing the overall robustness and functionality of the platform. Changes
Possibly related PRs
Suggested labels
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3106 +/- ##
========================================
Coverage 63.29% 63.29%
========================================
Files 422 422
Lines 29959 29959
========================================
Hits 18962 18962
Misses 10157 10157
Partials 840 840
|
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
Removing #3084 as attached PR as it will not fully address the problem |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
e2e/contracts/testgasconsumer/TestGasConsumer.abi (1)
1-47: Consider adding events for better test observability.While the current ABI is functional, adding events to track function execution would enhance debugging capabilities during gas consumption tests. This would make it easier to verify the exact point of gas exhaustion.
Consider adding an event like:
event GasConsumed( bytes origin, address sender, uint256 chainID, address zrc20, uint256 amount, uint256 gasUsed );e2e/contracts/testgasconsumer/TestGasConsumer.sol (1)
9-13: Consider adding context validation.While the zContext struct is well-defined, the contract should validate the context parameters in the onCall function to ensure they meet expected criteria (non-zero address for sender, valid chainID range).
function onCall( zContext calldata _context, address _zrc20, uint256 _amount, bytes calldata _message ) external { + require(_context.sender != address(0), "Invalid sender address"); + require(_context.chainID > 0, "Invalid chain ID"); + require(_context.origin.length > 0, "Invalid origin"); consumeGas(); }e2e/e2etests/test_deposit_and_call_out_of_gas.go (4)
3-13: Consider grouping imports for better organization.The imports could be organized into three distinct groups:
- Standard library imports
- External dependencies
- Internal packages
import ( + // Standard library "math/big" + // External dependencies "github.com/stretchr/testify/require" "github.com/zeta-chain/protocol-contracts/v2/pkg/gatewayevm.sol" + // Internal packages "github.com/zeta-chain/node/e2e/contracts/testgasconsumer" "github.com/zeta-chain/node/e2e/runner" "github.com/zeta-chain/node/e2e/utils" crosschaintypes "github.com/zeta-chain/node/x/crosschain/types" )
15-15: Enhance function documentation for better clarity.Consider adding more detailed documentation following Go's documentation conventions:
-// TestDepositAndCallOutOfGas tests that a deposit and call that consumer all gas will revert +// TestDepositAndCallOutOfGas verifies that a deposit and call operation reverts when gas is exhausted. +// It deploys a gas consumer contract and attempts to perform a deposit with a zero gas limit. +// The test expects the cross-chain transaction to be marked as reverted. +// +// Args: +// - r: E2E test runner instance +// - args: Test arguments where args[0] is the deposit amount in base units
17-20: Improve argument validation and error messaging.The argument validation could be more explicit and user-friendly:
- require.Len(r, args, 1) + require.Len(r, args, 1, "TestDepositAndCallOutOfGas requires exactly 1 argument: deposit amount") amount, ok := big.NewInt(0).SetString(args[0], 10) - require.True(r, ok, "Invalid amount specified for TestV2ETHDepositAndCall") + require.True(r, ok, "Invalid deposit amount: %s. Must be a valid decimal number", args[0])
16-38: Consider adding test cleanup.The test should clean up resources after completion:
Consider adding a cleanup function or using
t.Cleanup()to ensure the deployed contract and any other resources are properly cleaned up after the test completes, especially if this test is part of a larger test suite.Example cleanup pattern:
defer func() { // Add cleanup logic here // For example, you might want to: // 1. Log final contract state // 2. Verify no lingering transactions // 3. Clean up any test data }()e2e/contracts/testgasconsumer/TestGasConsumer.json (1)
6-26: Consider adding validation for the zContext structure.The context structure contains critical cross-chain information. Ensure that:
- The
originbytes field has a maximum length constraint- The
chainIDis validated against supported chainsConsider implementing these validations in the contract constructor or a separate validation function to maintain gas efficiency.
x/fungible/keeper/v2_evm.go (1)
18-20: Consider making the gas limit configurableWhile 1M gas is a reasonable baseline, hardcoding this value as a package-level variable might limit flexibility. Consider:
- Making it configurable through chain parameters
- Adding documentation about the gas limit choice
-// gatewayGasLimit is the gas limit for the gateway functions -var gatewayGasLimit = big.NewInt(1_000_000) +// DefaultGatewayGasLimit is the default gas limit for gateway functions +var DefaultGatewayGasLimit = big.NewInt(1_000_000) + +// GetGatewayGasLimit returns the current gas limit for gateway functions +func (k Keeper) GetGatewayGasLimit(ctx sdk.Context) *big.Int { + // TODO: Implement chain parameter lookup with fallback to default + return DefaultGatewayGasLimit +}e2e/e2etests/e2etests.go (1)
900-907: Consider enhancing test description and adding gas limit parameter.The test case implementation looks good, but consider these improvements:
- The description could be more specific about the expected behavior when gas is exhausted
- Consider adding a gas limit parameter to explicitly control the gas threshold for testing different scenarios
Apply this diff to enhance the test definition:
runner.NewE2ETest( TestDepositAndCallOutOfGasName, - "deposit Ether into ZEVM and call a contract that runs out of gas", + "deposit Ether into ZEVM and verify CCTX reversion when contract call exhausts gas", []runner.ArgDefinition{ {Description: "amount in wei", DefaultValue: "10000000000000000"}, + {Description: "gas limit", DefaultValue: "100000"}, }, TestDepositAndCallOutOfGas, ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/contracts/testgasconsumer/TestGasConsumer.binis excluded by!**/*.bin
📒 Files selected for processing (10)
changelog.md(1 hunks)cmd/zetae2e/local/v2.go(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.abi(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.go(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.json(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.sol(1 hunks)e2e/contracts/testgasconsumer/bindings.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_deposit_and_call_out_of_gas.go(1 hunks)x/fungible/keeper/v2_evm.go(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/contracts/testgasconsumer/TestGasConsumer.go
- e2e/contracts/testgasconsumer/bindings.go
🧰 Additional context used
📓 Path-based instructions (4)
cmd/zetae2e/local/v2.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_deposit_and_call_out_of_gas.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/v2_evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
x/fungible/keeper/v2_evm.go
[warning] 139-139: x/fungible/keeper/v2_evm.go#L139
Added line #L139 was not covered by tests
[warning] 185-185: x/fungible/keeper/v2_evm.go#L185
Added line #L185 was not covered by tests
[warning] 237-237: x/fungible/keeper/v2_evm.go#L237
Added line #L237 was not covered by tests
🔇 Additional comments (13)
e2e/contracts/testgasconsumer/TestGasConsumer.abi (1)
1-47: ABI structure is well-designed for gas consumption testing.
The ABI definition is clean and well-structured, with appropriate parameter types for cross-chain token transfers. The context struct provides essential chain data, and the nonpayable modifier is correctly applied.
e2e/contracts/testgasconsumer/TestGasConsumer.sol (2)
1-8: LGTM! Good choice of Solidity version.
The contract uses Solidity 0.8.26 which includes important security features like overflow checks and improved error handling. The storage variable is correctly scoped as private.
27-41:
Critical: Improve gas consumption reliability and safety.
Several concerns with the current implementation:
- Magic numbers should be declared as constants
- Gas estimates may vary across EVM implementations
- No emergency stop mechanism
- Potential for DOS if gas estimates are incorrect
Consider this safer implementation:
contract TestGasConsumer {
+ uint256 private constant TARGET_GAS = 5000000;
+ uint256 private constant STORAGE_WRITE_GAS = 20000;
+ bool private stopped;
+
+ modifier notStopped() {
+ require(!stopped, "Contract is stopped");
+ _;
+ }
+
+ function emergencyStop() external {
+ stopped = true;
+ }
+
function consumeGas() internal {
- uint256 targetGas = 5000000;
- uint256 storageWriteGasCost = 20000;
- uint256 iterations = targetGas / storageWriteGasCost;
+ uint256 iterations = TARGET_GAS / STORAGE_WRITE_GAS;
+ require(iterations > 0, "Invalid gas calculation");
+ require(iterations < 1000, "Too many iterations");
for (uint256 i = 0; i < iterations; i++) {
+ require(gasleft() > STORAGE_WRITE_GAS, "Insufficient gas");
storageArray.push(i);
}
delete storageArray;
}
}Let's verify the gas consumption estimates:
e2e/contracts/testgasconsumer/TestGasConsumer.json (3)
43-46: Review nonpayable modifier usage.
The nonpayable modifier is appropriate here as this function handles token transfers through _zrc20 parameter rather than native currency. This prevents accidental value transfers through the function call.
49-49: Verify the deployed bytecode matches the source.
The binary appears to be valid EVM bytecode. Ensure it matches the source code compilation with optimization settings appropriate for a gas-intensive contract.
#!/bin/bash
# Look for contract deployment tests
rg -A 5 "TestGasConsumer.*Deploy" "test/"
# Search for optimization settings in hardhat config
rg "solidity.*optimizer" -A 53-47: Verify gas consumption patterns in the onCall function implementation.
The ABI definition suggests this contract is designed to consume gas during cross-chain operations. Given the PR's objective to implement gas limits for omnichain calls, ensure that:
- The contract's gas consumption is predictable and bounded
- The function reverts cleanly when gas limits are exceeded
✅ Verification successful
Gas consumption implementation is well-designed and predictable
The contract implementation shows a deliberate and controlled gas consumption pattern:
- Uses a fixed target gas consumption of 5M gas
- Calculates iterations based on known storage operation costs (20K gas)
- Includes cleanup to prevent state bloat
- Has corresponding e2e test for out-of-gas scenarios
The ABI definition correctly reflects this implementation with appropriate parameter types and nonpayable modifier. The gas consumption is predictable and bounded, making it suitable for testing gas limit implementations in omnichain calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas-related operations in the contract implementation
ast-grep --pattern 'function onCall($$$) {
$$$
gasleft()
$$$
}'
# Look for gas consumption tests
rg -l "TestDepositAndCallOutOfGasName" "test/"
Length of output: 289
Script:
#!/bin/bash
# Look for the actual contract implementation
fd -e sol TestGasConsumer
# Search for gas-related test files
fd -e sol -e js -e ts test.*gas
# Look for the contract source to analyze gas consumption
rg -A 10 "contract TestGasConsumer"
Length of output: 999
Script:
#!/bin/bash
# Get the full implementation of TestGasConsumer contract
cat e2e/contracts/testgasconsumer/TestGasConsumer.sol
# Look for test files that might be using this contract
rg -l "TestGasConsumer" e2e/
Length of output: 1474
cmd/zetae2e/local/v2.go (3)
58-58: LGTM: Test case addition aligns with PR objectives.
The addition of TestDepositAndCallOutOfGasName to the eth-revert test group is appropriate for validating gas limit behavior in omnichain calls.
Line range hint 89-91: Verify if the TODO is still relevant.
The TODO comment references issue #2554 regarding breaking down this routine. Given the new gas-related test cases being added, it might be worth reassessing if this refactoring should be prioritized to improve test organization.
✅ Verification successful
Based on the GitHub issue response, I can generate the final response since the issue #2554 is marked as "CLOSED" and its content clearly shows it was about implementing V2 contract E2E tests with specific test cases for gas and ERC20 operations.
The TODO comment can be removed as the referenced issue #2554 is now closed
The issue has been addressed with the implementation of V2 contract E2E tests, including comprehensive test cases for gas operations, ERC20 transactions, and cross-VM calls. The original refactoring task has been completed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced issue is still open and relevant
gh issue view 2554 --json state,title,body
Length of output: 1170
56-59: Verify test dependencies for the new gas test.
The new test is added after other revert-related tests. Let's verify if there are any specific dependencies or required test ordering.
✅ Verification successful
Let me gather more information about the test implementation to understand its dependencies better.
Test ordering is appropriate with no specific dependencies
The test is correctly placed as it belongs to the revert-related test group. After reviewing the implementation:
- It's a standalone test that deploys its own contract (TestGasConsumer)
- Uses common test utilities and runner methods
- Doesn't depend on state from other tests
- Appropriately tests the revert scenario with gas exhaustion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any dependencies or setup requirements in the test implementation
rg -A 5 "TestDepositAndCallOutOfGas"
Length of output: 2227
Script:
#!/bin/bash
# Check the implementation details of the test
cat e2e/e2etests/test_deposit_and_call_out_of_gas.go
Length of output: 1361
x/fungible/keeper/v2_evm.go (2)
Line range hint 18-237: Verify gas limit behavior across different networks
The implementation of a fixed gas limit needs verification across different network conditions and transaction types.
Consider:
- Monitoring and adjusting the gas limit based on network conditions
- Implementing circuit breakers for high gas situations
- Adding metrics collection for gas usage patterns
Would you like assistance in implementing any of these suggestions?
39-39: Add test coverage for gateway gas limit changes
The gas limit implementation lacks test coverage for several gateway contract calls. Consider adding test cases that verify:
- Gas limit is properly enforced
- Contract calls fail gracefully when gas limit is exceeded
- Different gas consumption scenarios
Also applies to: 89-89, 139-139, 185-185, 237-237
e2e/e2etests/e2etests.go (1)
152-152: LGTM: Test name follows naming convention.
The constant name TestDepositAndCallOutOfGasName follows the established pattern and clearly indicates its purpose.
changelog.md (1)
13-15: LGTM! The changelog entry is well-structured and informative.
The entry properly documents the fix for blocked CCTX during omnichain calls with out-of-gas conditions, aligning with the PR objectives.
Co-authored-by: Dmitry S <[email protected]>
Description
TestDepositAndCallOutOfGasNametest to check CCTX revert if the omnichain call is out of gasCheck the change:
if the value is set back to nil in https://github.com/zeta-chain/node/pull/3106/files#diff-27b3a64d8d9957b22a3aaec6fe8679ed046033e5de6437afe7b46c7efca3c7b7R19 then the
TestDepositAndCallOutOfGasNamewill fail because inbound doesn't get observedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Improvements