fix: out of gas on ZetaClient during onRevert
#3144
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the deployment logic of the Changes
Possibly related issues
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 #3144 +/- ##
===========================================
- Coverage 63.19% 63.13% -0.06%
===========================================
Files 423 423
Lines 29887 29880 -7
===========================================
- Hits 18886 18865 -21
- Misses 10163 10177 +14
Partials 838 838
|
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: 2
🧹 Outside diff range and nitpick comments (10)
zetaclient/zetacore/constant.go (1)
49-50: Enhance documentation with gas consumption breakdownConsider adding a more detailed comment explaining:
- The breakdown of gas consumption for different operations during
onRevert- Why 7M gas is necessary (specific operations and their costs)
- Any chain-specific considerations
- // The value is set to 7M because in case of onRevert call, it might consume lot of gas + // Gas limit breakdown for onRevert operations: + // - Base operation: ~4M gas + // - Additional operations in testDApp: ~300K gas + // - Buffer for complex operations: ~2.7M gas + // Total: 7M gas to ensure sufficient capacity for all onRevert scenariose2e/e2etests/test_deploy_contract.go (3)
48-48: Consider adding a named parameter for clarity.The boolean parameter's purpose isn't immediately clear from the context. Consider using a named parameter for better code readability.
- true, + isZetaChain: true,
68-68: Consider adding a named parameter for clarity.Similar to the ZEVM deployment, the boolean parameter's purpose should be made explicit.
- false, + isZetaChain: false,
47-49: Document the significance of the isZetaChain parameter.The new boolean parameter appears to be crucial for determining the deployment context, but its significance and impact on gas consumption (particularly for
onRevertoperations) isn't documented. Consider adding comments explaining:
- The purpose of this parameter
- Its relationship to the gas consumption fix
- The expected behavior differences between ZetaChain and non-ZetaChain deployments
Add documentation above both deployment functions:
// deployZEVMTestDApp deploys the TestDAppV2 contract on ZetaChain // with ZetaChain-specific configurations enabled, including adjusted // gas limits for operations like onRevert.// deployEVMTestDApp deploys the TestDAppV2 contract on Ethereum // with standard EVM configurations, using default gas limits // for contract operations.Also applies to: 67-69
e2e/runner/v2_setup_evm.go (1)
119-122: Consider improving the verification organization and documentation.While the verification logic is correct, consider these improvements:
- Add a more descriptive comment explaining why this check is crucial (e.g., "Verify contract is deployed in non-ZetaChain context").
- Consider moving this verification block closer to the TestDAppV2 deployment check for better code organization.
ensureTxReceipt(txTestDAppV2, "TestDAppV2 deployment failed") + + // Verify contract is deployed in non-ZetaChain context + isZetaChain, err := r.TestDAppV2EVM.IsZetaChain(&bind.CallOpts{}) + require.NoError(r, err) + require.False(r, isZetaChain) // whitelist the ERC20 txWhitelist, err := r.ERC20CustodyV2.Whitelist(r.EVMAuth, r.ERC20Addr)pkg/contracts/testdappv2/TestDAppV2.abi (1)
157-169: LGTM: Chain context identification function.The
isZetaChainview function provides a clean way to determine the chain context, which is essential for proper gas management and cross-chain operations.Consider documenting the implications of this chain identification mechanism in the contract's documentation, particularly how it affects gas consumption patterns across different chains.
x/fungible/keeper/v2_deposits_test.go (1)
Line range hint
1-300: Consider adding test cases for gas consumption scenarios.While the existing test cases cover various deposit scenarios, they don't explicitly test the gas consumption during
onRevertoperations. Consider adding test cases that:
- Verify the behavior with different gas limits
- Test the
onReverthook with the increased gas limit- Validate the behavior when gas consumption exceeds the previous 4M limit but is within the new 7M limit
Would you like me to help draft these additional test cases?
pkg/contracts/testdappv2/TestDAppV2.sol (2)
15-15: Adjust Modifier Order for Consistency with Solidity Style GuideConsider reordering the modifiers for
isZetaChainto enhance readability and adhere to Solidity conventions:- bool immutable public isZetaChain; + bool public immutable isZetaChain;
147-161: Refine Gas Consumption Logic inconsumeGas()While the current implementation achieves the goal of consuming gas, consider refining the method to account for variations in gas costs and to optimize performance.
Consider using a dynamic gas consumption loop:
function consumeGas() internal { uint256 startGas = gasleft(); while (startGas - gasleft() < 500000) { // Empty loop to consume gas } }This approach consumes approximately 500,000 gas without relying on specific gas costs for storage operations, making it more robust against changes in gas pricing.
pkg/contracts/testdappv2/TestDAppV2.go (1)
Some invocations of
DeployTestDAppV2are missing the newisZetaChain_parameter. Please update all function calls to include this parameter to prevent build failures.
- e2e/e2etests/test_deploy_contract.go: Multiple invocations of
DeployTestDAppV2without theisZetaChain_parameter.🔗 Analysis chain
Line range hint
67-76: Update all calls toDeployTestDAppV2with the new parameterThe function
DeployTestDAppV2now includes an additional parameterisZetaChain_. Ensure that all invocations of this function throughout the codebase are updated to include this parameter to prevent build failures.Run the following script to identify all calls to
DeployTestDAppV2that may require updates:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'DeployTestDAppV2' in Go files. # Search for 'DeployTestDAppV2(' across the codebase. rg --type go 'DeployTestDAppV2\('Length of output: 659
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-67: pkg/contracts/testdappv2/TestDAppV2.go#L67
Added line #L67 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/contracts/testdappv2/TestDAppV2.binis excluded by!**/*.bin
📒 Files selected for processing (9)
e2e/e2etests/test_deploy_contract.go(2 hunks)e2e/runner/v2_setup_evm.go(3 hunks)e2e/runner/v2_setup_zeta.go(3 hunks)pkg/contracts/testdappv2/TestDAppV2.abi(2 hunks)pkg/contracts/testdappv2/TestDAppV2.go(4 hunks)pkg/contracts/testdappv2/TestDAppV2.json(3 hunks)pkg/contracts/testdappv2/TestDAppV2.sol(4 hunks)x/fungible/keeper/v2_deposits_test.go(1 hunks)zetaclient/zetacore/constant.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
e2e/e2etests/test_deploy_contract.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/v2_setup_evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/v2_setup_zeta.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/testdappv2/TestDAppV2.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/v2_deposits_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/constant.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
pkg/contracts/testdappv2/TestDAppV2.go
[warning] 67-67: pkg/contracts/testdappv2/TestDAppV2.go#L67
Added line #L67 was not covered by tests
[warning] 76-76: pkg/contracts/testdappv2/TestDAppV2.go#L76
Added line #L76 was not covered by tests
[warning] 414-420: pkg/contracts/testdappv2/TestDAppV2.go#L414-L420
Added lines #L414 - L420 were not covered by tests
[warning] 422-424: pkg/contracts/testdappv2/TestDAppV2.go#L422-L424
Added lines #L422 - L424 were not covered by tests
[warning] 431-432: pkg/contracts/testdappv2/TestDAppV2.go#L431-L432
Added lines #L431 - L432 were not covered by tests
[warning] 438-439: pkg/contracts/testdappv2/TestDAppV2.go#L438-L439
Added lines #L438 - L439 were not covered by tests
🔇 Additional comments (14)
zetaclient/zetacore/constant.go (1)
49-50: Verify the increased gas limit value
While increasing the gas limit addresses the immediate out-of-gas issue, the jump from 4M to 7M gas seems disproportionate to the stated 300K additional gas consumption. Let's verify this value.
e2e/runner/v2_setup_zeta.go (2)
6-6: LGTM: Required import for contract interaction.
The addition of the bind package is necessary for the new contract verification functionality.
78-81: LGTM: Robust contract verification.
The added verification ensures that TestDAppV2 is correctly deployed as a ZetaChain contract. The error handling is comprehensive and follows Go best practices.
e2e/runner/v2_setup_evm.go (2)
7-7: LGTM: Import addition is appropriate.
The bind package import is correctly placed and necessary for the new contract interaction functionality.
105-105: LGTM: TestDAppV2 deployment modification is correct.
The deployment call has been properly updated to include the new isZetaChain parameter, with appropriate error handling.
pkg/contracts/testdappv2/TestDAppV2.abi (1)
2-12: LGTM: Constructor implementation with chain context.
The constructor addition with isZetaChain_ parameter is well-structured and follows ABI standards.
Let's verify the constructor usage across the codebase:
✅ Verification successful
LGTM: Constructor implementation with chain context.
The isZetaChain_ parameter is correctly passed in all instances of DeployTestDAppV2, ensuring consistent contract deployment across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TestDAppV2 deployments to ensure the boolean parameter is properly passed
rg -A 3 "DeployTestDAppV2"
Length of output: 1605
x/fungible/keeper/v2_deposits_test.go (1)
54-54: Verify the impact of the true parameter on gas consumption.
The change to pass true to DeployContract appears related to the PR's objective of addressing gas consumption issues. However, the test cases don't explicitly verify the gas consumption behavior during onRevert operations.
Let's verify the gas consumption implications:
✅ Verification successful
Verified: The true parameter enables gas consumption testing during onRevert
The change to pass true to DeployContract is intentional and aligns with the test's purpose of verifying gas consumption behavior during onRevert operations. The TestDAppV2 contract is specifically designed to consume gas during reverts when deployed on ZetaChain, helping validate the system's handling of gas-intensive revert scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas-related configurations and tests
rg -A 5 "gasLimit|onRevert"
# Look for related test cases
ast-grep --pattern 'func Test$_.*Revert.*\(.*\) {
$$$
}'
Length of output: 191163
pkg/contracts/testdappv2/TestDAppV2.json (3)
3-13: LGTM: Constructor properly handles ZetaChain context.
The constructor follows Solidity best practices with:
- Appropriate parameter naming convention using underscore suffix
- Proper state mutability modifier
158-170: LGTM: isZetaChain function implementation is correct.
The function follows best practices:
- View modifier for gas optimization
- Boolean return type matches the context check requirement
319-319: Verify bytecode size impact on deployment.
The contract bytecode is substantial. While this is expected given the contract's functionality, it's worth noting that:
- Large contract sizes can impact deployment costs
- Some networks have contract size limits
pkg/contracts/testdappv2/TestDAppV2.sol (3)
9-10: Initialization of storageArray for Gas Simulation is Appropriate
The addition of storageArray to simulate gas consumption aligns with the intended functionality.
47-49: Constructor Update Correctly Initializes isZetaChain
The constructor appropriately sets the isZetaChain immutable variable based on the deployment context.
127-129: Verify Additional Gas Consumption Does Not Exceed Block Gas Limit
The invocation of consumeGas() within onRevert serves the purpose of increasing gas consumption for testing. Ensure that this additional gas usage does not cause transactions to exceed the block gas limit, which could result in unintended reverts.
pkg/contracts/testdappv2/TestDAppV2.go (1)
54-55: Verify the accuracy of the updated ABI and binary data
The ABI and Bin fields in TestDAppV2MetaData have been updated to reflect the new constructor and methods. Please confirm that these changes correctly represent the latest contract modifications.
Description
Use 7M gas instead of 4M when processing a revert outbound.
To test the change, the
onReverthook on ZetaChain testDApp consume 300k more gasTo check the change is effective:
Revert back this line: https://github.com/zeta-chain/node/pull/3144/files#diff-503de1565f57cda7a6a71c86db258cf0b5eb1a346eb009991b2f2ea1d30df70eR50
The
v2EthWithdrawAndRevertTestwill fail because of the current issue (outbound not observed)Summary by CodeRabbit
New Features
Bug Fixes
PostVoteOutboundRevertGasLimitto accommodate potential high gas consumption.Tests