fix: enforce checksum format for asset address in ZRC20 #3278
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📝 WalkthroughWalkthroughThe pull request introduces several enhancements and modifications across various components of the ZetaChain node. Key updates include the addition of a Changes
Assessment against linked 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
|
lumtis
added
UPGRADE_LIGHT_TESTS
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3278 +/- ##
===========================================
+ Coverage 61.76% 61.79% +0.03%
===========================================
Files 428 429 +1
Lines 30791 30816 +25
===========================================
+ Hits 19018 19043 +25
Misses 10914 10914
Partials 859 859
|
![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 (8)
pkg/crypto/evm_address.go (1)
16-20: Consider enhancing address validation.While the current implementation covers basic EVM address validation, consider adding a check for mixed-case addresses to prevent potential issues with case-sensitive comparisons.
func IsEVMAddress(address string) bool { - return len(address) == 42 && strings.HasPrefix(address, "0x") && common.IsHexAddress(address) + if len(address) != 42 || !strings.HasPrefix(address, "0x") || !common.IsHexAddress(address) { + return false + } + // Ensure address doesn't mix cases inappropriately + return address == strings.ToLower(address) || IsChecksumAddress(address) }x/fungible/keeper/migrator.go (1)
21-25: Enhance migration logging.Consider adding more detailed logging to track migration progress and completion.
func (m Migrator) Migrate2to3(ctx sdk.Context) error { - ctx.Logger().Info("Migrating fungible module from v2 to v3") + ctx.Logger().Info("Starting fungible module migration", "from_version", "2", "to_version", "3") + startTime := time.Now() + + if err := v3.MigrateStore(ctx, m.fungibleKeeper); err != nil { + ctx.Logger().Error("Migration failed", "error", err) + return err + } + + ctx.Logger().Info("Migration completed successfully", "duration", time.Since(startTime)) return v3.MigrateStore(ctx, m.fungibleKeeper) }x/crosschain/types/message_whitelist_erc20.go (1)
69-81: Consider enhancing address validationThe validation function correctly implements the checksum format requirement for EVM addresses. However, consider the following improvements:
- Add validation for Solana addresses mentioned in the PR objectives
- Consider adding length validation for EVM addresses
func validateAssetAddress(address string) error { if address == "" { return errors.New("empty asset address") } // if the address is an evm address, check if it is in checksum format - if crypto.IsEVMAddress(address) && !crypto.IsChecksumAddress(address) { - return errors.New("evm address is not in checksum format") + if crypto.IsEVMAddress(address) { + if len(address) != 42 { + return errors.New("invalid evm address length") + } + if !crypto.IsChecksumAddress(address) { + return errors.New("evm address is not in checksum format") + } + return nil + } + + // validate solana address format + if crypto.IsSolanaAddress(address) { + // Add Solana-specific validation + return nil } - // currently no specific check is implemented for other address format - return nil + return errors.New("unsupported address format") }x/fungible/migrations/v3/migrate_test.go (1)
13-48: Enhance test coverage with additional test casesWhile the current test cases cover basic scenarios, consider adding:
- Negative test cases with invalid addresses
- More descriptive test names explaining the transformation
tests := []struct { name string assetList []string expectedList []string }{ { - name: "no asset to update", + name: "should succeed: empty asset list requires no updates", assetList: []string{}, expectedList: []string{}, }, { - name: "assets to update", + name: "should succeed: mixed addresses are properly checksummed", assetList: []string{ "", "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", // lowercase "", "0xc0ffee254729296a45a3885639AC7E10F9d54979", // checksum "", "", "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", "BrS9iNMC3y8J4QTmCz8VrGrYepdoxXYvKxcDMiixwLn5", "0x999999CF1046E68E36E1AA2E0E07105EDDD1F08E", // uppcase }, expectedList: []string{ "", "0x5a4f260A7D716c859A2736151cB38b9c58C32c64", "", "0xc0ffee254729296a45a3885639AC7E10F9d54979", "", "", "Gh9ZwEmdLJ8DscKNTkTqPbNwLNNBjuSzaG9Vp2KGtKJr", "BrS9iNMC3y8J4QTmCz8VrGrYepdoxXYvKxcDMiixwLn5", "0x999999cf1046e68e36E1aA2E0E07105eDDD1f08E", }, }, + { + name: "should fail: invalid EVM address format", + assetList: []string{ + "0xinvalid", + }, + expectedList: []string{ + "0xinvalid", + }, + }, }testutil/sample/fungible.go (1)
23-23: Consider documenting LiquidityCap field and its rangeThe LiquidityCap field is added without documentation explaining its purpose and the rationale behind the value range (0-10B).
+ // LiquidityCap defines the maximum allowed liquidity for the foreign coin + // Range: 0-10B (arbitrary range for testing purposes) LiquidityCap: UintInRange(0, 10000000000),x/crosschain/types/message_whitelist_erc20_test.go (1)
74-85: Consider adding edge cases for address validationThe test case for invalid checksum format is good, but consider adding edge cases:
- Address with mixed case but invalid checksum
- Address with invalid characters
- Address with incorrect length
{ name: "evm asset address with invalid checksum format", msg: types.NewMsgWhitelistERC20( sample.AccAddress(), "0x5a4f260a7d716c859a2736151cb38b9c58c32c64", 1, "name", "symbol", 6, 10, ), error: true, }, +{ + name: "evm address with invalid characters", + msg: types.NewMsgWhitelistERC20( + sample.AccAddress(), + "0x5a4f260g7d716c859a2736151cb38b9c58c32c64", + 1, + "name", + "symbol", + 6, + 10, + ), + error: true, +},pkg/crypto/evm_address_test.go (1)
39-84: Consider adding test case for invalid hex charactersWhile the test coverage is good, consider adding a test case for addresses containing invalid hex characters.
+ { + name: "address with invalid hex characters", + address: "0x5a4f260g7d716c859a2736151cb38b9c58c32c64", + },changelog.md (1)
26-26: Consider adding more context to the changelog entry.The entry correctly documents the fix for enforcing checksum format, but could be more descriptive about the impact and scope.
Consider expanding the entry to:
-* [3278](https://github.com/zeta-chain/node/pull/3278) - enforce checksum format for asset address in ZRC20 +* [3278](https://github.com/zeta-chain/node/pull/3278) - enforce checksum format for EVM asset addresses when whitelisting new assets in ZRC20, includes migration script for existing addresses
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
changelog.md(1 hunks)pkg/crypto/address.go(0 hunks)pkg/crypto/address_test.go(0 hunks)pkg/crypto/evm_address.go(1 hunks)pkg/crypto/evm_address_test.go(1 hunks)testutil/sample/fungible.go(1 hunks)x/crosschain/types/message_whitelist_erc20.go(3 hunks)x/crosschain/types/message_whitelist_erc20_test.go(1 hunks)x/fungible/keeper/migrator.go(1 hunks)x/fungible/migrations/v3/migrate.go(1 hunks)x/fungible/migrations/v3/migrate_test.go(1 hunks)x/fungible/module.go(2 hunks)
💤 Files with no reviewable changes (2)
- pkg/crypto/address.go
- pkg/crypto/address_test.go
🧰 Additional context used
📓 Path-based instructions (9)
testutil/sample/fungible.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/module.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_whitelist_erc20_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/migrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/migrations/v3/migrate_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/crypto/evm_address_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/crypto/evm_address.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/migrations/v3/migrate.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (10)
pkg/crypto/evm_address.go (2)
11-14: LGTM! Clean implementation of empty address check.
The function correctly handles both empty struct and zero address cases.
22-30: LGTM! Robust checksum implementation.
Both functions correctly leverage go-ethereum's implementation for checksum validation and conversion.
x/crosschain/types/message_whitelist_erc20.go (2)
55-55: LGTM: Improved error message formatting
The error message now includes the underlying error details, which will help with debugging.
57-58: LGTM: Proper error handling for asset address validation
The validation error is correctly wrapped with a domain-specific error type.
x/crosschain/types/message_whitelist_erc20_test.go (2)
86-98: LGTM! Good test coverage for valid EVM addresses
The test case properly validates a correctly checksummed EVM address.
Line range hint 99-107: LGTM! Good test coverage for Solana addresses
The test case correctly handles Solana addresses, maintaining compatibility with non-EVM chains.
pkg/crypto/evm_address_test.go (2)
10-37: LGTM! Thorough test coverage for empty address detection
The test cases cover all scenarios for empty address validation including zero address and regular addresses.
118-156: LGTM! Comprehensive test coverage for checksum conversion
The test cases thoroughly cover all scenarios including:
- Already checksummed addresses
- Lowercase and uppercase conversions
- Empty and non-EVM address handling
x/fungible/module.go (2)
126-129: LGTM! Proper migration registration with error handling
The migration registration is correctly implemented with appropriate error handling through panic for initialization failures.
160-160: LGTM! Consensus version bump aligns with migration
The consensus version is correctly bumped to 3 to match the new migration.
Co-authored-by: Dmitry S <[email protected]>
Description
Closes #3274
Add a check to enforce the checksum format for EVM address while whitelisting a new asset
Add a migration script for the current asset address
Summary by CodeRabbit
New Features
/systemtimetelemetry endpoint to enhance telemetry capabilities.LiquidityCapfield to theForeignCoinsstruct.Bug Fixes
MsgWhitelistERC20structure.Refactor
Tests