fix: backport fix sui for invalid receiver address#3954
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 📝 WalkthroughWalkthroughThis update introduces stricter validation for Sui receiver addresses in ZEVM withdrawal flows. A new end-to-end test ensures withdrawals to invalid addresses are correctly rejected. The Sui address validation logic is refactored and made more rigorous, and the ZEVM withdrawal process now explicitly checks receiver address validity before broadcasting. Several Sui-related E2E tests are temporarily disabled, focusing test coverage on the new invalid receiver scenario. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZEVM
participant SuiValidator
participant SuiChain
User->>ZEVM: Request Sui withdrawal (with receiver address)
ZEVM->>SuiValidator: Validate receiver address
alt Address valid
SuiValidator-->>ZEVM: OK
ZEVM->>SuiChain: Broadcast withdrawal transaction
SuiChain-->>ZEVM: Transaction result
else Address invalid
SuiValidator-->>ZEVM: Invalid address error
ZEVM->>SuiChain: Broadcast cancel transaction
SuiChain-->>ZEVM: Cancel transaction result
end
ZEVM-->>User: Withdrawal outcome (success or failure)
sequenceDiagram
participant E2ETest
participant ZEVM
participant SuiValidator
E2ETest->>ZEVM: Initiate withdrawal to invalid Sui address
ZEVM->>SuiValidator: Validate address
SuiValidator-->>ZEVM: Invalid address error
ZEVM-->>E2ETest: Transaction fails as expected
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3954 +/- ##
===========================================
+ Coverage 64.10% 64.11% +0.01%
===========================================
Files 474 474
Lines 34838 34842 +4
===========================================
+ Hits 22333 22340 +7
+ Misses 11479 11477 -2
+ Partials 1026 1025 -1
🚀 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 (3)
x/crosschain/keeper/evm_hooks_test.go (1)
10-10: Align import groups to Go conventions
Project-local imports should be grouped after third-party libraries and separated by blank lines. Please relocategithub.com/zeta-chain/node/pkg/contracts/suiinto the ZetaChain imports block (below the external dependencies) for consistent grouping.cmd/zetae2e/local/local.go (1)
489-503: Appropriate focused testing approach for validating the invalid receiver fix.Temporarily disabling other Sui tests to focus on the new
TestSuiWithdrawInvalidReceivertest is a sound strategy during development. This ensures the fix is thoroughly validated in isolation before re-enabling the full test suite.Consider adding a TODO comment indicating when these tests should be re-enabled:
suiTests := []string{ e2etests.TestSuiDepositName, + // TODO: Re-enable these tests after invalid receiver fix validation is complete // e2etests.TestSuiDepositAndCallRevertName,pkg/contracts/sui/address.go (1)
30-33: Consider error handling implications.The
DecodeAddressfunction was simplified by removing error handling, which makes the API cleaner but eliminates validation. Ensure that all callers of this function provide valid byte slices, as invalid input will now cause runtime panics rather than graceful error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_sui_withdraw_invalid_receiver.go(1 hunks)pkg/contracts/sui/address.go(1 hunks)pkg/contracts/sui/address_test.go(5 hunks)x/crosschain/keeper/evm_hooks.go(1 hunks)x/crosschain/keeper/evm_hooks_test.go(1 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)zetaclient/chains/sui/signer/signer_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_hooks.gox/crosschain/keeper/evm_hooks_test.gozetaclient/chains/sui/signer/signer.goe2e/e2etests/test_sui_withdraw_invalid_receiver.goe2e/e2etests/e2etests.gozetaclient/chains/sui/signer/signer_test.gopkg/contracts/sui/address.gopkg/contracts/sui/address_test.gocmd/zetae2e/local/local.go
🧬 Code Graph Analysis (5)
x/crosschain/keeper/evm_hooks.go (1)
pkg/contracts/sui/address.go (2)
DecodeAddress(31-33)ValidateAddress(37-54)
zetaclient/chains/sui/signer/signer.go (1)
pkg/contracts/sui/address.go (1)
ValidateAddress(37-54)
e2e/e2etests/test_sui_withdraw_invalid_receiver.go (4)
e2e/runner/runner.go (1)
E2ERunner(80-210)e2e/utils/parsing.go (1)
ParseBigInt(27-32)e2e/utils/evm.go (1)
MustWaitForTxReceipt(19-48)e2e/utils/require.go (1)
RequiredTxFailed(46-55)
e2e/e2etests/e2etests.go (2)
e2e/runner/e2etest.go (2)
NewE2ETest(41-58)ArgDefinition(88-91)e2e/e2etests/test_sui_withdraw_invalid_receiver.go (1)
TestSuiWithdrawInvalidReceiver(14-42)
cmd/zetae2e/local/local.go (1)
e2e/e2etests/e2etests.go (1)
TestSuiWithdrawInvalidReceiverName(118-118)
🔇 Additional comments (11)
changelog.md (1)
9-12: Changelog update is accurate
The new fix entry underUNRELEASEDclearly describes the backport and is formatted correctly within the “Fixes” section.x/crosschain/keeper/evm_hooks.go (1)
368-371: LGTM: Simplified Sui address validation logic aligns with updated address package.The validation logic correctly reflects the refactored
pkg/contracts/sui/address.gowhereDecodeAddressno longer returns an error andValidateAddressperforms comprehensive validation including format, case, length, and hex encoding checks.zetaclient/chains/sui/signer/signer.go (2)
108-118: Excellent defensive programming: Early receiver address validation prevents invalid transactions.The addition of receiver address validation before transaction broadcasting is a robust approach that:
- Catches invalid addresses early in the process
- Provides clear error logging for debugging
- Prevents unnecessary TSS operations on invalid transactions
121-121: LGTM: Correct integration of receiver validation into broadcast decision logic.The conditional properly gates withdrawal broadcasting on both compliance checks and receiver address validity, ensuring that only valid transactions proceed to broadcast while invalid ones are canceled.
e2e/e2etests/test_sui_withdraw_invalid_receiver.go (1)
13-42: Well-structured E2E test validates complete invalid receiver address handling.The test excellently follows the AAA pattern and provides comprehensive validation of the fix:
- Arrange: Properly sets up test prerequisites including ZRC20 approval and revert options
- Act: Correctly performs withdrawal to invalid receiver address
- Assert: Appropriately verifies that the transaction fails in ZEVM as expected
The use of
RevertOptionsdemonstrates thorough understanding of the withdrawal flow and ensures proper fallback handling.e2e/e2etests/e2etests.go (2)
118-118: LGTM: Test constant follows naming convention.The constant naming is consistent with other Sui test constants in the file.
1000-1008: LGTM: Test registration is well-structured.The test registration follows the established pattern with appropriate arguments and defaults. The invalid receiver address (EVM format) is suitable for testing the validation logic.
zetaclient/chains/sui/signer/signer_test.go (1)
213-300: LGTM: Comprehensive test for invalid receiver address handling.The test case effectively validates the behavior when an invalid Sui receiver address (EVM format) is provided. The test correctly expects the "increase_nonce" function to be called instead of "withdraw", which aligns with the cancel transaction flow for invalid addresses. The test structure and assertions are consistent with existing test patterns.
pkg/contracts/sui/address.go (1)
35-54: LGTM: Stricter validation improves address format consistency.The
ValidateAddressfunction (renamed fromValidAddress) implements more rigorous validation:
- Enforces exact 64-character hex strings
- Requires lowercase format
- Uses
strings.CutPrefixfor cleaner prefix handlingThe stricter validation aligns with the PR objective to prevent invalid receiver addresses from causing issues downstream.
pkg/contracts/sui/address_test.go (2)
57-79: LGTM: Test simplification aligns with function changes.The
TestDecodeAddresstest was appropriately simplified to match the updatedDecodeAddressfunction that no longer returns errors.
81-145: LGTM: Comprehensive test coverage for stricter validation.The renamed
TestValidSuiAddressfunction properly tests the enhanced validation logic:
- Verifies rejection of uppercase addresses
- Confirms rejection of short addresses
- Maintains coverage for existing validation scenarios
- Uses clear test case names and appropriate assertions
The test suite effectively validates the stricter address format requirements.
…kport-fix-sui-invalid-receiver-address
4 participants
Description
This PR is to backport #3945.
In case it happens again (due to any other edge case) in near future and requires quick fix, this PR handles invalid receiver address in both
zetacoreandzetaclient.How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Refactor