test(e2e): sui withdraws #3595
test(e2e): sui withdraws #3595
Conversation
# Conflicts: # changelog.md # e2e/config/config.go # pkg/contracts/sui/crypto.go # pkg/contracts/sui/gateway.go # zetaclient/chains/sui/observer/observer_test.go
![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
🧹 Nitpick comments (3)
e2e/e2etests/test_sui_withdraw.go (1)
11-29: New end-to-end test for Sui withdrawalsThis test effectively validates the Sui withdrawal flow from approval to transaction verification. It follows the standard E2E testing pattern established in the codebase.
Consider enhancing the test with balance verification:
+ // Check initial balance + initialBalance, err := r.GetSUIZRC20Balance(signer.Address()) + require.NoError(r, err, "get initial balance") + // perform the withdraw tx := r.SuiWithdrawSUI(signer.Address(), amount) r.Logger.EVMTransaction(*tx, "withdraw") // wait for the cctx to be mined cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout) r.Logger.CCTX(*cctx, "withdraw") require.Equal(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status) + + // Verify balance change + finalBalance, err := r.GetSUIZRC20Balance(signer.Address()) + require.NoError(r, err, "get final balance") + expectedFinalBalance := new(big.Int).Sub(initialBalance, amount) + require.Equal(r, expectedFinalBalance.String(), finalBalance.String(), "balance should decrease by withdrawn amount")e2e/e2etests/test_sui_token_withdraw.go (1)
11-29: Add logging before initiating the withdrawal operation.The test function is well-structured and follows the correct sequence of operations (approval, withdrawal, transaction verification). Consider adding a log statement before line 23 to indicate the withdrawal is being initiated, similar to how you're tracking the CCTX afterward.
// perform the withdraw +r.Logger.Info("Initiating Sui fungible token withdrawal for amount %s", amount.String()) tx := r.SuiWithdrawFungibleToken(signer.Address(), amount)e2e/e2etests/test_sui_token_deposit_and_call_revert.go (1)
20-25: Pool liquidity values could be parameterized.The hardcoded values for adding liquidity should be extracted as constants or configurable parameters at the top of the file, especially if they need to be adjusted in the future or if they'll be reused across multiple tests.
+// Liquidity constants +var ( + zetaLiquidityAmount = big.NewInt(1e18) + zrc20LiquidityAmount = big.NewInt(9000000000) +) // add liquidity in pool to allow revert fee to be paid -zetaAmount := big.NewInt(1e18) -zrc20Amount := big.NewInt(9000000000) -r.AddLiquiditySUI(zetaAmount, zrc20Amount) -r.AddLiquiditySuiFungibleToken(zetaAmount, zrc20Amount) +r.AddLiquiditySUI(zetaLiquidityAmount, zrc20LiquidityAmount) +r.AddLiquiditySuiFungibleToken(zetaLiquidityAmount, zrc20LiquidityAmount)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/observer/query_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (26)
cmd/zetae2e/local/local.go(1 hunks)cmd/zetae2e/local/sui.go(1 hunks)contrib/localnet/scripts/start-zetacored.sh(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)e2e/e2etests/e2etests.go(4 hunks)e2e/e2etests/test_migrate_tss.go(1 hunks)e2e/e2etests/test_sui_deposit_and_call_revert.go(1 hunks)e2e/e2etests/test_sui_token_deposit_and_call_revert.go(1 hunks)e2e/e2etests/test_sui_token_withdraw.go(1 hunks)e2e/e2etests/test_sui_withdraw.go(1 hunks)e2e/runner/accounting.go(1 hunks)e2e/runner/liquidity.go(1 hunks)e2e/runner/runner.go(2 hunks)e2e/runner/setup_sui.go(5 hunks)e2e/runner/setup_zevm.go(1 hunks)e2e/runner/sui.go(2 hunks)pkg/chains/chain.go(2 hunks)pkg/crypto/tss.go(2 hunks)pkg/crypto/tss_test.go(5 hunks)proto/zetachain/zetacore/observer/query.proto(2 hunks)x/crosschain/keeper/cctx_orchestrator_validate_inbound.go(2 hunks)x/crosschain/keeper/cctx_orchestrator_validate_inbound_test.go(2 hunks)x/crosschain/types/cmd_cctxs.go(2 hunks)x/observer/keeper/grpc_query_tss.go(2 hunks)x/observer/keeper/grpc_query_tss_test.go(3 hunks)zetaclient/chains/sui/signer/signer_tx.go(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- x/crosschain/keeper/cctx_orchestrator_validate_inbound.go
- x/crosschain/types/cmd_cctxs.go
- x/crosschain/keeper/cctx_orchestrator_validate_inbound_test.go
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.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.
cmd/zetae2e/local/sui.goe2e/runner/accounting.goe2e/e2etests/test_sui_deposit_and_call_revert.gocmd/zetae2e/local/local.gozetaclient/chains/sui/signer/signer_tx.gopkg/chains/chain.goe2e/runner/runner.goe2e/e2etests/test_sui_token_deposit_and_call_revert.goe2e/runner/setup_zevm.goe2e/runner/liquidity.gopkg/crypto/tss.goe2e/runner/sui.goe2e/e2etests/test_sui_token_withdraw.gopkg/crypto/tss_test.goe2e/e2etests/test_migrate_tss.goe2e/e2etests/test_sui_withdraw.gox/observer/keeper/grpc_query_tss_test.goe2e/e2etests/e2etests.goe2e/runner/setup_sui.gox/observer/keeper/grpc_query_tss.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/observer/query.proto
`**/*.sh`: Review the shell scripts, point out issues relati...
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/scripts/start-zetacored.sh
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-upgrade-test / e2e
- GitHub Check: build-and-test
🔇 Additional comments (43)
cmd/zetae2e/local/sui.go (1)
47-47:❓ Verification inconclusive
Significant increase in minted token amount.
The amount of Sui USDC tokens minted has been increased from 10 million to 100 billion. This substantial increase appears necessary to support the new end-to-end tests but should be monitored for potential effects on test performance or token economics simulations.
🏁 Script executed:
#!/bin/bash # Check for any potential overflow issues or constraints related to token amounts in Sui tests # Search for token amount validation or constraints in test files rg -A 2 -B 2 "amount.*constraint|limit.*amount|max.*amount" --glob "*.go" ./e2e/e2etests/test_sui_*.goLength of output: 431
Action Required: Manual Verification of Increased Token Minting Amount
The call at
cmd/zetae2e/local/sui.go:47now mints 100 billion Sui USDC tokens (i.e.,"100000000000"), up from the previous value of 10 million. Our automated search across related end-to-end test files did not surface explicit validations or constraints regarding token amounts. However, given the substantial increase, please ensure that:
- There are no hidden overflow or performance issues in the Sui token minting logic.
- The increased minted value does not adversely affect the related test simulations or token economics.
Please perform manual verification of these factors to confirm stability.
e2e/runner/setup_zevm.go (1)
63-63: Added support for Sui TSS address.The addition of the Sui TSS address assignment completes the TSS address handling alongside the existing Bitcoin and Ethereum addresses, ensuring that Sui operations can properly utilize the threshold signature scheme.
pkg/chains/chain.go (2)
4-4: Added import for hex encoding.The
encoding/hexpackage is now imported to support encoding Sui addresses.
94-95: Added Sui address encoding support.The implementation correctly handles Sui consensus addresses by converting them to a "0x"-prefixed hexadecimal format, which aligns with Sui's address representation standard.
cmd/zetae2e/local/local.go (1)
429-429:✅ Verification successful
Added comprehensive Sui test cases.
New end-to-end test cases have been added for Sui operations, including deposit and call revert scenarios as well as withdrawal tests for both SUI and Sui fungible tokens. This enhances test coverage for the Sui integration.
Also applies to: 433-435
🏁 Script executed:
#!/bin/bash # Verify that the referenced test files exist and contain proper implementations # Check for the existence of test implementation files echo "Verifying Sui test files existence:" find ./e2e/e2etests -name "test_sui_*.go" | sort # Check that the test names are properly defined in e2etests.go echo -e "\nVerifying test definitions in e2etests.go:" rg "TestSui(Withdraw|DepositAndCallRevert|TokenWithdraw|TokenDepositAndCallRevert)Name" ./e2e/e2etests/e2etests.goLength of output: 1241
Subject: Verified Sui Test Case Integration
The updated end-to-end test cases for Sui operations have been successfully verified. The referenced test names and their corresponding test implementations exist as expected:
e2etests.TestSuiDepositAndCallRevertNameis correctly referenced incmd/zetae2e/local/local.go.- The necessary test files (e.g.,
test_sui_deposit_and_call_revert.go) are present in thee2e/e2etestsdirectory.- The test definitions in
e2e/e2etests/e2etests.goconfirm that all Sui test names, including deposit, call revert, and withdrawal tests for both SUI and Sui fungible tokens, are properly defined.These validations confirm that the new Sui tests have been added comprehensively and are in active use.
proto/zetachain/zetacore/observer/query.proto (2)
249-249: TSS address response now includes Sui format.The addition of the Sui TSS address field aligns with the PR objectives to enhance Sui integration for TSS address querying. The field numbering follows protocol buffer conventions, maintaining backward compatibility.
260-260: Consistent addition of Sui field in historical address response.This change maintains consistency with the regular TSS address query response by adding the Sui address field with the same field number (3). This ensures API uniformity across related endpoints.
e2e/e2etests/test_migrate_tss.go (1)
132-132: Updated function name to maintain consistent TSS capitalization.The function name has been standardized from
GetTssAddrBTCtoGetTSSAddrBTCto maintain consistent capitalization of the TSS acronym throughout the codebase.e2e/runner/accounting.go (1)
98-98: Updated function name to maintain consistent TSS capitalization.The function name has been standardized from
GetTssAddrBTCtoGetTSSAddrBTCto maintain consistent capitalization of the TSS acronym throughout the codebase.e2e/e2etests/test_sui_deposit_and_call_revert.go (1)
1-30: Implemented E2E test for Sui deposit with expected reversion.This new test successfully implements one of the PR objectives by testing the
deposit_and_call_revertfunctionality for SUI. The test validates that:
- A deposit transaction with a "revert" instruction correctly reverts
- The CCTX status is properly updated to
Reverted- Appropriate coin type and cross-chain call flags are set
The implementation follows the established E2E testing patterns and includes proper validation of the transaction state.
docs/openapi/openapi.swagger.yaml (2)
58814-58815: Ensure Consistent Extension of Schema for Sui PropertyThe new
sui:property is correctly introduced with astringtype. Please verify that its indentation and placement conform to the overall YAML schema style. Additionally, ensure that any backend implementations or documentation referencing this property are updated accordingly.
58823-58824: Validate Sui Field in QueryGetTssAddressResponseThe addition of the
sui:property under theQueryGetTssAddressResponseobject is consistent with the PR objectives. Confirm that thestringtype appropriately represents the SUI address format and that this change is reflected in all related API documentation and tests.e2e/runner/runner.go (2)
79-79: New field for Sui TSS address addedThe addition of
SuiTSSAddressto theE2ERunnerstruct aligns with the PR objective of adding a TSS address in Sui format. This field will store the Threshold Signature Scheme address in Sui's required format.
252-254: Address propagation for Sui TSS AddressCorrectly propagating the Sui TSS address in the
CopyAddressesFrommethod ensures consistent TSS address usage across runner instances. This maintains the pattern established for other chain-specific TSS addresses.zetaclient/chains/sui/signer/signer_tx.go (3)
31-33: Refined logic for handling cross-chain callsThe condition has been enhanced to explicitly handle reverting cross-chain calls as regular withdrawals by checking both the cross-chain call flag and the transaction status. The comment clarifies the implementation intent.
76-79: Added request type for transaction executionThe addition of the
RequestTypefield with "WaitForLocalExecution" value ensures the transaction waits for local execution before returning, providing stronger consistency guarantees.
37-38:❓ Verification inconclusive
Added 0x prefix for ERC20 coin type
The code now ensures proper formatting for Sui ERC20 token identifiers by adding the required "0x" prefix.
🏁 Script executed:
#!/bin/bash # Check if the code handles cases where the asset might already have the 0x prefix ast-grep --pattern $'case params.CoinType == coin.CoinType_ERC20: $$$ coinType = "0x" + cctx.InboundParams.Asset $$$'Length of output: 124
Action Required: Verify Unconditional Prefixing Assumption
The change now unconditionally prepends
"0x"tocctx.InboundParams.Asset(seezetaclient/chains/sui/signer/signer_tx.go, lines 37–38). While the review comment indicates this was intended for formatting ERC20 token identifiers, the code does not check if the asset already contains the"0x"prefix. It is important to confirm that:
- The asset value is guaranteed to be without a
"0x"prefix when the coin type is ERC20 (or more generally, for coin types other than SUI).- No unintended double-prefixing occurs if the asset is already prefixed elsewhere.
The previous AST pattern check did not return output, suggesting that the expected conditional context (e.g., a
case params.CoinType == coin.CoinType_ERC20:branch) might not be present or might be structured differently. Please manually verify whether the asset is always provided without the prefix or if additional conditioning is required.e2e/runner/liquidity.go (2)
33-37: Implementation of SUI liquidity addition looks good.The function follows the established pattern in the codebase for adding liquidity to a token pool. It first approves the token for the Uniswap router and then adds liquidity with appropriate amounts.
39-43: Implementation of Sui fungible token liquidity addition looks good.The function properly follows the same pattern as other liquidity addition functions, maintaining consistency throughout the codebase by first approving the token and then adding the specified amounts to the pool.
x/observer/keeper/grpc_query_tss_test.go (3)
155-166: TSS address retrieval implementation improved with proper error handling.The changes standardize the function naming convention from
GetTssAddr*toGetTSSAddr*for consistency, and add proper error handling after each function call. The addition of Sui address support enhances the functionality while maintaining the same pattern.
182-193: TSS address retrieval correctly implemented for testnet4.The implementation for the testnet4 test case follows the same pattern as the previous test case, with proper error handling and the addition of Sui address support.
253-264: TSS address retrieval correctly implemented for finalized height scenario.The implementation for the finalized height scenario correctly implements the same pattern as the other test cases, maintaining consistency throughout the codebase.
pkg/crypto/tss_test.go (2)
17-17: Function naming standardized for TSS acronym.The function names have been updated to use "TSS" in uppercase consistently, which aligns with the proper acronym convention for "Threshold Signature Scheme". The associated function calls have also been updated correctly.
Also applies to: 44-44, 95-95, 139-139
56-93: Well-structured test function for Sui address retrieval.The new test function follows the established pattern of other TSS address tests with appropriate test cases for both success and error conditions. The implementation correctly validates the Sui address derivation from the TSS public key.
e2e/runner/setup_sui.go (4)
45-49: Improved TSS funding mechanism.The code has been simplified by directly calling
RequestSuiFromFaucetwithout a header map. The addition of TSS funding is valuable for tests, though the TODO comment correctly identifies this as potentially removable in the future.
81-81: Enhanced object ID handling for Sui operations.The variable declarations have been updated to include
withdrawCapID, and the code properly identifies and stores both thewhitelistCapIDandwithdrawCapIDobjects. This is essential for the withdrawal functionality implementation.Also applies to: 107-114
87-87: Improved error messages with better context.Error messages now include more specific identifiers, making debugging easier and more effective when objects aren't found.
Also applies to: 96-96, 105-105, 199-199, 206-206
128-129: Critical withdraw cap transfer to TSS.The implementation correctly sends the withdraw capability to the TSS address, which is essential for withdrawal tests. The
sendWithdrawCapToTSSfunction is well-structured and follows the established pattern for Sui transactions in the codebase.Also applies to: 320-330
e2e/runner/sui.go (3)
19-27: Clean abstraction for token approval operations.These methods provide a clear, token-specific interface for approval operations, enhancing readability and reducing the risk of errors when working with different token types.
29-67: Well-implemented withdrawal functions for both SUI and fungible tokens.The withdrawal functions follow a consistent pattern, properly handling address decoding, error checking, and transaction execution. The revert options are correctly set with zero gas limit, which is appropriate for this use case.
142-142: Improved error message with context data.Including the actual coin type in the error message provides valuable context for debugging, improving the developer experience when troubleshooting issues.
pkg/crypto/tss.go (3)
13-14: Standardized TSS acronym naming across functions.Function names have been updated to use consistent uppercase "TSS" notation, improving naming convention consistency across the codebase.
Also applies to: 30-31
20-20: Improved variable naming convention.The variable has been renamed from
decompresspubkeytodecompressPubKey, aligning with Go's camelCase naming convention for local variables, which enhances code readability.Also applies to: 25-25
40-52: Clean implementation of Sui TSS address retrieval.The new
GetTSSAddrSuifunction follows the established pattern of the other TSS address retrieval functions, with proper error handling and a concise implementation. It correctly leverages thezetasui.AddressFromPubKeyECDSAfunction to derive the Sui address from the decompressed public key.x/observer/keeper/grpc_query_tss.go (2)
50-82: LGTM: Well-structured TSS address retrieval with added Sui supportThe code correctly adds support for retrieving the Sui TSS address alongside existing Ethereum and Bitcoin addresses. The implementation follows consistent error handling patterns.
93-125: LGTM: Consistent implementation of Sui address in historical TSS queryThe historical TSS query method has been properly updated to include Sui address retrieval, maintaining consistency with the primary TSS address query method.
e2e/e2etests/e2etests.go (7)
90-97: LGTM: Good test organization for Sui functionalityThe added constants for Sui-related tests follow the consistent naming convention already established in the codebase for other chains.
699-704: Updated test parameter aligns with realistic token quantitiesThe increase in default amount from 1000000 to 10000000000 mist provides a more realistic test scenario for SUI deposits.
714-722: LGTM: Well-structured test definition for Sui deposit and call revertThe new test follows the established pattern of other deposit and call revert tests in the codebase, with appropriate minimum version requirement.
726-731: Updated test parameter aligns with realistic token quantitiesThe increase in default amount provides a more realistic test scenario for Sui token deposits, consistent with the change to the SUI deposit test.
741-749: LGTM: Well-structured test definition for Sui token deposit and call revertThe test definition properly follows the established pattern for token deposit and call revert tests.
750-758: LGTM: Well-structured test definition for Sui withdrawalThe new withdrawal test follows the established pattern in the codebase with appropriate version requirement.
759-767: LGTM: Well-structured test definition for Sui token withdrawalThe test definition properly follows the established pattern for token withdrawal tests with appropriate version requirement.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3595 +/- ##
===========================================
- Coverage 64.55% 64.54% -0.01%
===========================================
Files 469 469
Lines 32723 32760 +37
===========================================
+ Hits 21123 21145 +22
- Misses 10639 10650 +11
- Partials 961 965 +4
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Summary by CodeRabbit
New Features
Tests
Documentation