chore: migrate erc20 token pair #783
Conversation
WalkthroughThe changes in this pull request enhance the migration functionality within the Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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 (2)
x/erc20/migrations/v8/keys.go (2)
72-75: Provide more context in the error message for missing metadataWhen metadata is not found (line 74), the error returned indicates the denomination that was not found. Consider providing additional context to help diagnose the issue more effectively, such as suggesting possible reasons or next steps.
Consider updating the error message as follows:
- return sdkerrors.ErrKeyNotFound.Wrapf("metadata not found: %s", tokenPair.GetDenom()) + return sdkerrors.ErrKeyNotFound.Wrapf("metadata not found for denomination '%s'. Ensure that the denomination is registered and metadata is properly set.", tokenPair.GetDenom())
66-89: Add unit tests formigrateTokenPairto ensure migration correctnessThe new
migrateTokenPairfunction is pivotal to the migration process. Adding unit tests for this function will help ensure that it behaves correctly under various scenarios, such as when metadata is missing, or denominations have different cases.Would you like assistance in creating unit tests for
migrateTokenPairto improve test coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- x/erc20/migrations/v8/keys.go (3 hunks)
- x/erc20/migrations/v8/legacy.go (0 hunks)
- x/erc20/migrations/v8/migrate.go (3 hunks)
💤 Files with no reviewable changes (1)
- x/erc20/migrations/v8/legacy.go
🔇 Additional comments (6)
x/erc20/migrations/v8/migrate.go (5)
24-41: LGTM! Variable renaming improves clarity.The change from
baseDenomtonewBaseDenommakes the code more self-documenting, while maintaining the correct token exclusion logic and special case handling for PUNDIX.
Line range hint
46-51: LGTM! Method maintains correct routing logic.The method correctly determines whether to handle IBC or bridge tokens based on the alias prefix.
86-89: LGTM! Improved error handling for ERC20 token retrieval.The addition of explicit error handling for GetERC20Token improves the robustness of the code.
59-59: Verify parameter ordering in AddIBCToken call.Please ensure that the parameter order (channel, base, alias) matches the keeper's AddIBCToken method signature to prevent runtime errors.
✅ Verification successful
Parameter order in AddIBCToken call is incorrect
The keeper's
AddIBCTokenmethod signature inx/erc20/keeper/ibc_token.gois:func (k Keeper) AddIBCToken(ctx context.Context, channel, baseDenom, ibcDenom string)But the call in the migration code uses:
m.keeper.AddIBCToken(ctx, channel, base, alias)The parameter order should be:
ctx(correct)channel(correct)baseasbaseDenom(correct)aliasasibcDenom(correct)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the AddIBCToken method definition to verify parameter order ast-grep --pattern 'func $_AddIBCToken($$$) { $$$ }'Length of output: 63
Script:
#!/bin/bash # Let's try a different approach to find the AddIBCToken method definition # First, let's use ripgrep to find the method definition rg "func.*AddIBCToken" -A 5 # Also search for any interface definitions that might contain this method rg "interface.*\{" -A 10 -B 2 | rg -A 10 "AddIBCToken"Length of output: 462
91-94: Verify the impact of removing LegacyIsNativeERC20.The change from LegacyIsNativeERC20 to direct IsNativeERC20 check looks correct, but we should verify there are no remaining references to the legacy method.
✅ Verification successful
No remaining references to LegacyIsNativeERC20 found, migration is correct
The verification confirms:
- No references to
LegacyIsNativeERC20exist in the codebase- The
IsNativeERC20()method is consistently implemented inx/erc20/types/token.goand used correctly in:
- Migration code (
x/erc20/migrations/v8/migrate.go)- Convert logic (
x/erc20/keeper/convert.go)- Crosschain precompile (
x/crosschain/precompile/keeper.go)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to LegacyIsNativeERC20 rg "LegacyIsNativeERC20" # Search for IsNativeERC20 usage to ensure consistent implementation ast-grep --pattern 'IsNativeERC20()'Length of output: 63
Script:
#!/bin/bash # Search for any remaining references to LegacyIsNativeERC20 with case insensitive flag rg -i "legacyisnativeerc20" # Search for IsNativeERC20 method implementations and usages with context rg -A 3 "IsNativeERC20" # Look for any legacy references in migration files fd -e go . x/erc20/migrations | xargs catLength of output: 14641
x/erc20/migrations/v8/keys.go (1)
82-82: Confirm the correctness of modifyingtokenPair.DenomdirectlyIn line 82,
tokenPair.Denomis modified by setting it tostrings.ToLower(md.Symbol). Ensure that changing theDenomfield directly does not have unintended side effects elsewhere in the codebase, especially iftokenPairis used after this modification.To verify if
tokenPair.Denommodification is safe, please review the usage oftokenPairafter this assignment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/erc20/migrations/v8/keys.go (3 hunks)
🔇 Additional comments (5)
x/erc20/migrations/v8/keys.go (5)
4-5: LGTM! Import changes align with new functionality.The added imports for
stringsandsdkerrorsare necessary for the new token pair migration logic.Also applies to: 9-11
21-23: LGTM! Migration order and error handling are appropriate.The addition of
migrateTokenPairmaintains consistent error handling and follows a logical migration sequence.
71-71: Previous review comment is still valid: Handle unmarshaling errors safely.Using
MustUnmarshalcan cause panics. The previous suggestion to useUnmarshalwith proper error handling should be implemented.
76-81: Previous review comment is still valid: Ensure consistent store cleanup.The store cleanup inconsistency noted in the previous review remains - the old key should be deleted in both branches to prevent data inconsistency.
66-88: Consider adding validation for the token pair structure.The function should validate the token pair structure after unmarshaling to ensure all required fields are present and valid.
Let's verify the token pair structure requirements:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores