test(e2e): add e2e test for v2 deposit and call with swap #3188
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
|
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📝 WalkthroughWalkthroughThe pull request introduces a series of updates to the ZetaChain node, including new features for whitelisting SPL tokens on Solana, integration of SPL deposits and withdrawals, and the addition of end-to-end tests for deposit and swap functionalities. It also includes significant refactoring efforts, such as the removal of the HSM signer and simplifications in the zetaclientd configuration. Various fixes address issues within the emissions module, peer discovery mechanisms, and transaction handling, enhancing overall functionality and stability. 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
|
![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 (7)
e2e/e2etests/test_v2_deposit_and_call_swap.go (3)
44-49: Consider using a test helper for gas limit managementThe gas limit modification is a critical part of the test but could be encapsulated better:
func withTemporaryGasLimit(r *runner.E2ERunner, gasLimit uint64, fn func() error) error { previousLimit := r.ZEVMAuth.GasLimit r.ZEVMAuth.GasLimit = gasLimit defer func() { r.ZEVMAuth.GasLimit = previousLimit }() return fn() }
51-64: Consider extracting magic numbers into named constantsThe test uses several magic numbers for token amounts and liquidity values. Consider defining these as named constants at the package level for better maintainability and clarity.
const ( initialLiquidityAmount = 1e8 minLiquidityAmount = 1e5 liquidityTimeoutMins = 10 )
1-88: Consider adding test documentation and prerequisitesThe test implements a complex scenario but lacks comprehensive documentation. Consider adding:
- A detailed test description in the function's documentation
- Prerequisites and setup requirements
- Expected outcomes and verification steps
- Cleanup procedures
Example documentation:
// TestV2DepositAndCallSwap validates the v2 deposit and call functionality with swap // Prerequisites: // - Deployed v2 contracts // - Sufficient token balances // Steps: // 1. Creates token pair // 2. Adds liquidity // 3. Performs deposit and call // Expected outcome: // - Successful swap execution // - Correct token balances // - OutboundMined CCTX statuse2e/contracts/zevmswap/ZEVMSwapApp.abi (1)
76-120: Consider architectural implications of duplicate function signaturesThe newly added
onCallfunction has an identical signature to the existingonCrossChainCallfunction. This raises several architectural considerations:
- Both functions accept the same parameters and have the same state mutability
- The duplication might indicate a need for a more elegant design pattern
Consider:
- Implementing a shared internal function if the logic is similar
- Clearly documenting the distinct purposes of each function
- Adding appropriate access control mechanisms
cmd/zetae2e/local/v2.go (1)
Line range hint
89-91: Consider implementing a more modular test framework structure.While the current implementation works well, there are opportunities for architectural improvements:
- Break down the test routine into smaller, focused test suites
- Implement a builder pattern for test configuration
- Consider using test fixtures for common setup scenarios
This would align with the TODO comment and make the test framework more maintainable and extensible.
e2e/e2etests/e2etests.go (1)
1077-1082: Consider adding test parameters for swap configuration.The test registration looks good, but given that this test simulates a swap and withdrawal scenario with gas limit considerations, consider adding configuration parameters such as:
- Swap amount
- Gas limit thresholds
- Token pairs for the swap
This would make the test more flexible for different test scenarios.
Example addition:
runner.NewE2ETest( TestV2DepositAndCallSwapName, "evm -> zevm deposit and call with swap and withdraw", - []runner.ArgDefinition{}, + []runner.ArgDefinition{ + {Description: "swap amount", DefaultValue: "1000000000000000000"}, + {Description: "gas limit", DefaultValue: "2000000"}, + }, TestV2DepositAndCallSwap, ),e2e/contracts/zevmswap/ZEVMSwapApp.go (1)
337-356: Enhance parameter naming for better code clarity.Consider renaming the
arg0parameter tocontextin theOnCallmethods. Using a descriptive name improves readability and aligns with Go's best practices for expressive code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/contracts/zevmswap/ZEVMSwapApp.binis excluded by!**/*.bin
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetae2e/local/v2.go(1 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.abi(1 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.go(2 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.json(2 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.sol(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_v2_deposit_and_call_swap.go(1 hunks)
🧰 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/contracts/zevmswap/ZEVMSwapApp.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_v2_deposit_and_call_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (10)
e2e/e2etests/test_v2_deposit_and_call_swap.go (2)
16-19: Consider adding test cleanup and documentation
The TODO comment indicates this test is temporary and should be removed after issue #2711 is completed. Consider:
- Adding a link to PR 3106 in the comment since it's related to the gas limit fix being tested
- Adding cleanup steps to remove test data after execution
85-88: Enhance error handling and test assertions
While the test verifies the CCTX status, consider adding more comprehensive assertions:
- Verify the token balances after the swap
- Add timeout handling for the WaitCctxMinedByInboundHash call
- Add assertions for the expected gas consumption
e2e/contracts/zevmswap/ZEVMSwapApp.abi (1)
76-120: Verify security considerations for onCall function
The ABI indicates this is a public function without visible access controls. Given its cross-chain nature:
Please ensure:
- The contract implementation includes appropriate access controls
- The function validates the Context parameters
- The function properly handles edge cases for amount and message parameters
cmd/zetae2e/local/v2.go (1)
43-43: LGTM! Test placement is well-considered.
The addition of TestV2DepositAndCallSwapName to the ERC20 token workflow group is appropriate. The test is correctly positioned after the basic deposit/call tests, ensuring necessary prerequisites (like ETH deposits for fees) are executed first.
Please verify that the test dependencies in TestV2DepositAndCallSwap align with the existing test order, particularly regarding any required initial state or token balances.
e2e/contracts/zevmswap/ZEVMSwapApp.sol (1)
114-116: Add slippage protection
The swap is currently configured with no minimum output amount (using 0), which could lead to significant slippage in production.
Consider adding a slippage parameter or minimum output calculation to protect against unfavorable trades.
e2e/contracts/zevmswap/ZEVMSwapApp.json (2)
77-121: LGTM! The onCall function addition is well-structured.
The function signature and parameter types are properly defined, following the same pattern as the existing onCrossChainCall function. This addition aligns well with the PR's objective of testing v2 deposit and call functionality with swaps.
194-194: Verify bytecode update for gas optimization.
The bytecode has been updated to include the new onCall function. Given that this PR aims to address gas consumption issues, particularly with the cosmos-sdk gas limit for MsgVoteInbound:
- Consider running gas estimation tests to verify the bytecode's gas efficiency
- Ensure the implementation doesn't introduce unnecessary gas overhead
e2e/e2etests/e2etests.go (1)
175-175: LGTM! Test name follows naming convention.
The constant name TestV2DepositAndCallSwapName follows the established pattern and clearly indicates its purpose.
changelog.md (1)
17-17: LGTM!
The changelog entry accurately reflects the PR's objective of adding end-to-end tests for v2 deposit and call with swap functionality.
e2e/contracts/zevmswap/ZEVMSwapApp.go (1)
41-42: ABI and bytecode are correctly updated to include the new onCall function.
The ABI and bytecode modifications accurately reflect the addition of the onCall method, ensuring that the contract bindings remain consistent with the contract's interface.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
e2e/e2etests/test_v2_deposit_and_call_swap.go (4)
16-19: Enhance TODO comment structureConsider restructuring the TODO comment to follow a more standard format:
-// TODO: This test is similar to TestCrosschainSwap -// purpose is to test similar scenario with v2 contracts where there is swap + withdraw in onCall -// to showcase that it's not reverting with gas limit issues -// this test should be removed when this issue is completed: https://github.com/zeta-chain/node/issues/2711 +// TODO(#2711): Temporary test for v2 contracts swap + withdraw in onCall +// This test validates that the gas limit issues are resolved and should be removed +// once the crosschain swap test supports v2 contracts. +// Related test: TestCrosschainSwap
21-24: Add pair creation validationConsider adding assertions to verify the pair was created successfully:
tx, err := r.UniswapV2Factory.CreatePair(r.ZEVMAuth, r.ERC20ZRC20Addr, r.ETHZRC20Addr) require.NoError(r, err) utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) +pair, err := r.UniswapV2Factory.GetPair(nil, r.ERC20ZRC20Addr, r.ETHZRC20Addr) +require.NoError(r, err) +require.NotEqual(r, common.Address{}, pair, "pair should be created")
75-83: Validate memo encoding and document RevertOptionsConsider adding validation for the encoded memo and documenting the RevertOptions:
+// Validate memo structure +require.Greater(r, len(memobytes), 0, "encoded memo should not be empty") +// RevertOptions with zero gas limit means use the default gas limit tx = r.V2ERC20DepositAndCall( r.ZEVMSwapAppAddr, big.NewInt(8e7), memobytes, gatewayevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, )
84-88: Add comprehensive CCTX validationConsider adding more assertions to validate the complete CCTX state:
cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout) r.Logger.CCTX(*cctx, "deposit_and_call") require.Equal(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status) +require.NotEmpty(r, cctx.GetOutboundTxParams(), "outbound tx params should be present") +require.Greater(r, cctx.GetOutboundTxGasUsed(), uint64(0), "gas should be used in outbound tx")cmd/zetae2e/local/v2.go (1)
Line range hint
89-91: Consider implementing the planned test organization improvements.The TODO comment indicates future plans to break down this routine (issue #2554). Given the addition of new test cases like the swap test, this might be a good time to consider that refactoring. A more granular organization could help with:
- Clearer test dependencies and setup requirements
- Better isolation of test scenarios (basic operations, swaps, reverts)
- Improved maintainability and debugging
e2e/contracts/zevmswap/ZEVMSwapApp.go (3)
340-342: Enhance parameter naming for clarityConsider renaming the parameter
arg0tocontextto improve readability and align with Go naming conventions.Apply this diff to update the parameter name:
-func (_ZEVMSwapApp *ZEVMSwapAppTransactor) OnCall(opts *bind.TransactOpts, arg0 Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { - return _ZEVMSwapApp.contract.Transact(opts, "onCall", arg0, zrc20, amount, message) +func (_ZEVMSwapApp *ZEVMSwapAppTransactor) OnCall(opts *bind.TransactOpts, context Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { + return _ZEVMSwapApp.contract.Transact(opts, "onCall", context, zrc20, amount, message) }
347-349: Enhance parameter naming for clarityConsider renaming the parameter
arg0tocontextto improve readability and align with Go naming conventions.Apply this diff to update the parameter name:
-func (_ZEVMSwapApp *ZEVMSwapAppSession) OnCall(arg0 Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { - return _ZEVMSwapApp.Contract.OnCall(&_ZEVMSwapApp.TransactOpts, arg0, zrc20, amount, message) +func (_ZEVMSwapApp *ZEVMSwapAppSession) OnCall(context Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { + return _ZEVMSwapApp.Contract.OnCall(&_ZEVMSwapApp.TransactOpts, context, zrc20, amount, message) }
354-356: Enhance parameter naming for clarityConsider renaming the parameter
arg0tocontextto improve readability and align with Go naming conventions.Apply this diff to update the parameter name:
-func (_ZEVMSwapApp *ZEVMSwapAppTransactorSession) OnCall(arg0 Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { - return _ZEVMSwapApp.Contract.OnCall(&_ZEVMSwapApp.TransactOpts, arg0, zrc20, amount, message) +func (_ZEVMSwapApp *ZEVMSwapAppTransactorSession) OnCall(context Context, zrc20 common.Address, amount *big.Int, message []byte) (*types.Transaction, error) { + return _ZEVMSwapApp.Contract.OnCall(&_ZEVMSwapApp.TransactOpts, context, zrc20, amount, message) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/contracts/zevmswap/ZEVMSwapApp.binis excluded by!**/*.bin
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetae2e/local/v2.go(1 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.abi(1 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.go(2 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.json(2 hunks)e2e/contracts/zevmswap/ZEVMSwapApp.sol(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_v2_deposit_and_call_swap.go(1 hunks)
🧰 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/contracts/zevmswap/ZEVMSwapApp.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_v2_deposit_and_call_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (11)
e2e/contracts/zevmswap/ZEVMSwapApp.abi (2)
76-120: Clarify the purpose of duplicate function signatures
The newly added onCall function appears to have an identical signature to the existing onCrossChainCall function. Both functions accept the same parameters and have the same modifiers. This duplication could lead to confusion about which function should be used in different scenarios.
Could you please clarify:
- The specific use case for
onCallvsonCrossChainCall - Whether this is part of a transition strategy for v2 contracts
- If there are plans to deprecate one of these functions in the future
76-120: Verify v2 contract integration
The new onCall function appears to be part of the v2 deposit and call implementation mentioned in the PR objectives.
Please ensure:
- The function name and signature align with other v2 contract interfaces
- The Context struct fields (origin, sender, chainID) are compatible with v2 cross-chain communication protocols
- The function's nonpayable modifier is appropriate for the intended v2 deposit and call flow
cmd/zetae2e/local/v2.go (1)
43-43: LGTM! Verify test dependencies.
The addition of TestV2DepositAndCallSwapName to the ERC20 workflow test group is well-placed. Since swap operations typically require sufficient liquidity, ensure that the test has proper setup and dependencies (like TestOperationAddLiquidityETHName and TestOperationAddLiquidityERC20Name which are present in the revert test group).
e2e/contracts/zevmswap/ZEVMSwapApp.sol (1)
99-99: LGTM - Improves code readability
e2e/contracts/zevmswap/ZEVMSwapApp.json (2)
77-121: LGTM: Well-structured onCall function addition
The new onCall function is well-defined with appropriate parameter types and follows the same structure as the existing onCrossChainCall function, which is suitable for the v2 deposit and call testing requirements.
Please ensure that the gas consumption patterns of this function have been tested thoroughly, particularly given the PR's focus on addressing gas limit issues in MsgVoteInbound.
194-194: LGTM: Bytecode update is consistent
The bytecode has been properly updated to include the new onCall function implementation.
e2e/e2etests/e2etests.go (2)
175-175: LGTM: Constant follows naming convention.
The constant name and value follow the established pattern in the file.
1077-1082: Consider adding test arguments for configurability.
The test case is well-integrated and matches the PR objectives. However, consider whether parameters like swap amounts or gas limits should be configurable through arguments, similar to other test cases in the suite.
Example addition:
runner.NewE2ETest(
TestV2DepositAndCallSwapName,
"evm -> zevm deposit and call with swap and withdraw",
- []runner.ArgDefinition{},
+ []runner.ArgDefinition{
+ {Description: "deposit amount in wei", DefaultValue: "1000000000000000000"},
+ {Description: "swap amount in wei", DefaultValue: "500000000000000000"},
+ },
TestV2DepositAndCallSwap,
),changelog.md (1)
17-17: LGTM!
The changelog entry accurately reflects the PR objectives, documenting the addition of the e2e test for v2 deposit and call with swap functionality.
e2e/contracts/zevmswap/ZEVMSwapApp.go (2)
41-42: ABI and binary data updated correctly
The ABI and Bin fields have been updated to include the new onCall method, reflecting the contract changes accurately.
340-356: Verify security considerations for 'OnCall' method
Ensure that the OnCall method's implementation accounts for necessary validations to prevent potential security vulnerabilities, especially since it differs from OnCrossChainCall by not enforcing sender validation.
|
I'm also thinking maybe we can look into movin the logic in TestDAppV2. But this would be much easier to manage if we have a single test contract we have to deploy for live networks |
Description
Similar to existing crosschain swap test, but crosschain swap test can't be used with v2 contracts yet. This test can be removed with this issue: #2711 but for now its needed to test similar scenario as issue on testnet, where there is a swap and withdraw in onCall function, and cosmos-sdk gas limit for MsgVoteInbound is reached.
This also confirms the fix: #3106 - checked locally, without this fix, cosmos sdk 7M gas is drained, because evm gas limit missing is calling eth_estimateGas which spends a lot of cosmos gas.
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
onCallfunction in the ZEVMSwapApp contract for enhanced cross-chain operations.Bug Fixes
Tests