refactor: migrate bridge token data and update metadata alias #749
Conversation
WalkthroughThe pull request introduces several enhancements to the application, primarily focusing on cross-chain functionalities. A new method, 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: 5
🧹 Outside diff range and nitpick comments (6)
x/crosschain/keeper/legacy.go (2)
9-10: Enhance the deprecation notice.While the current deprecation notice is clear, it could be more informative. Consider expanding it to include:
- The reason for deprecation
- Alternative method(s) to use instead
- The exact version when it will be removed (if known)
For example:
// Deprecated: This function will be removed in v8 due to [reason]. // Use [alternative method] instead. // Will be removed in v8.0.0.
10-20: LGTM: Function implementation is correct, consider adding input validation.The
LegacyGetDenomBridgeTokenfunction correctly implements the legacy lookup for bridge tokens. It appropriately handles cases where no data is found and returns a pointer toBridgeTokenwhich is suitable for potentially large structs.However, consider adding input validation for the
denomparameter to ensure it's not empty or malformed before using it to generate the key. This could prevent potential issues with invalid inputs.Example:
if denom == "" { return nil, false } // Optionally, add more specific validation based on your denom formatx/erc20/migrations/v8/token.go (2)
9-60: LGTM: IBC denomination trace maps are well-structured.The separation of testnet and mainnet IBC denomination traces is a good practice. The comments providing information about the source chains are helpful.
Consider adding a brief comment explaining the purpose of these maps at the beginning of each map declaration. For example:
// testnetIBCDenomTrace maps IBC denominations to their corresponding channel identifiers for the testnet environment testnetIBCDenomTrace := map[string]string{ // ... (existing entries) } // mainnetIBCDenomTrace maps IBC denominations to their corresponding channel identifiers for the mainnet environment mainnetIBCDenomTrace := map[string]string{ // ... (existing entries) }This would provide immediate context for developers working with these maps.
62-65: LGTM: Testnet excluded bridge token map is defined correctly.The
testnetExcludeBridgeTokenmap provides a way to exclude specific tokens from bridge operations in the testnet environment, which is a good practice for network-specific configurations.Consider adding a brief comment explaining the purpose of this map. For example:
// testnetExcludeBridgeToken maps token addresses to a boolean indicating whether they should be excluded from bridge operations in the testnet environment testnetExcludeBridgeToken := map[string]bool{ // ... (existing entry) }This would provide immediate context for developers working with this map.
x/erc20/migrations/v8/migrate.go (1)
16-43: Handle potential cases where no tokens are migratedIn the
MigrateTokenfunction, after processing the denom metadata, there might be scenarios where no tokens are added due to the conditions in your loops and if-statements. Consider adding logging or error handling to account for situations where tokens are expected to be migrated but none are added. This will help in debugging and ensuring the migration process completes as intended.x/erc20/module.go (1)
88-95: Consider using a more descriptive parameter name inNewAppModuleThe parameter
cksin theNewAppModulefunction could be renamed tocrossChainKeepersfor clarity and consistency with the struct field name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- app/keepers/keepers.go (1 hunks)
- app/modules.go (1 hunks)
- app/upgrade_test.go (1 hunks)
- app/upgrades/v8/upgrade.go (3 hunks)
- x/crosschain/keeper/legacy.go (1 hunks)
- x/erc20/migrations/v8/legacy.go (1 hunks)
- x/erc20/migrations/v8/migrate.go (1 hunks)
- x/erc20/migrations/v8/migrations.go (1 hunks)
- x/erc20/migrations/v8/token.go (1 hunks)
- x/erc20/module.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (14)
x/crosschain/keeper/legacy.go (1)
1-7: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct and relevant for the functionality implemented in this file.
x/erc20/migrations/v8/legacy.go (1)
3-9: LGTM: Imports are appropriate and concise.The imported packages are relevant to the function's implementation, and there are no unused imports.
x/erc20/migrations/v8/migrations.go (3)
1-11: LGTM: Package declaration and imports are appropriate.The package is correctly named
v8, aligning with the migration version. The imports include necessary Cosmos SDK types and custom types from fx-core, which are appropriate for the migration task.
13-19: LGTM: Migrator struct is well-defined.The
Migratorstruct encapsulates all necessary dependencies for the migration process, including store key, codec, keeper, bank keeper, and cross-chain keepers. The use of a slice forcrossChainKeepersallows for multiple cross-chain integrations, which is a flexible design choice.
21-29: LGTM: NewMigrator constructor is well-implemented.The
NewMigratorfunction provides a clean way to create aMigratorinstance. It follows dependency injection principles by accepting all necessary dependencies as parameters. This approach enhances testability and flexibility of the code.app/upgrade_test.go (1)
83-83: Verify the necessity of explicitly setting the chain IDThe addition of
ctx = ctx.WithChainID(chainId)ensures that the chain ID is explicitly set in the context. While this improves clarity and consistency, it's worth noting that the chain ID is already set in the header used to create the context.Could you clarify if this addition addresses any specific issues? If not, consider whether this explicit setting is necessary given the existing header setup.
To verify the impact of this change, you can run the following script:
This will help identify if there are places in the codebase where the chain ID is accessed directly from the context, which would justify this addition.
✅ Verification successful
Consistent usage of
WithChainIDacross the codebaseThe use of
ctx.WithChainID(chainId)inapp/upgrade_test.goaligns with similar implementations inapp/keepers/keepers.goandapp/export_test.go. This ensures consistent handling of the chain ID within the context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any occurrences where the chain ID might be accessed directly from the context # rather than through the header, which could justify this addition. rg --type go 'ctx\.ChainID\(\)' app rg --type go 'WithChainID' appLength of output: 289
x/erc20/migrations/v8/token.go (3)
1-7: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct and align with the file's location and purpose.
67-74: LGTM:getIBCDenomTracefunction is well-implemented.The function correctly differentiates between testnet and mainnet environments using the chain ID from the context. It provides a clean way to retrieve the channel identifier for a given IBC denomination.
The implementation is concise and follows Go's idiomatic return style for map lookups.
76-81: LGTM with a question: Verify mainnet behavior forgetExcludeBridgeToken.The function correctly differentiates between testnet and mainnet environments using the chain ID from the context. For the testnet, it checks if the token should be excluded from bridge operations.
Please confirm if the current implementation for mainnet (always returning
false) is intentional. If it is, consider adding a comment explaining why no tokens are excluded in the mainnet environment. If not, you might want to add amainnetExcludeBridgeTokenmap similar to the testnet one.To verify the usage of this function, you can run the following script:
This will help ensure that the function is being used as intended across the codebase.
✅ Verification successful
Verified mainnet behavior for
getExcludeBridgeToken.The function is only used in
migrate.goand intentionally returnsfalsefor mainnet environments, ensuring no tokens are excluded during production operations. Consider adding a comment to clarify this behavior for future reference.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any usage of getExcludeBridgeToken function in the codebase # Test: Search for function calls to getExcludeBridgeToken rg --type go "getExcludeBridgeToken\s*\(" .Length of output: 218
app/keepers/keepers.go (2)
102-107: LGTM: Useful addition for cross-chain keeper aggregation.The
ToSlicemethod is a valuable addition to theCrossChainKeepersstruct. It provides a convenient way to access all cross-chain keepers as a slice, which can be beneficial for operations that need to iterate over or process all keepers collectively.
108-108: Verify initialization of embedded CrossChainKeepers.The inclusion of
CrossChainKeepersin theAppKeepersstruct is a good way to organize related keepers. However, ensure that theCrossChainKeepersis properly initialized in theNewAppKeeperfunction. This initialization is not visible in the provided code snippet and should be verified.To confirm the proper initialization, please run the following script:
This will help us verify that
CrossChainKeepersis being set up correctly when creating a newAppKeepersinstance.app/upgrades/v8/upgrade.go (2)
44-45: Integration of metadata update within upgrade handlerThe addition of
updateMetadata(cacheCtx, app.BankKeeper)correctly integrates the metadata update process into the upgrade handler.
84-102: 🛠️ Refactor suggestion
⚠️ Potential issueEnsure complete update of denomination units
The
updateMetadatafunction currently modifies only the first denomination unit (md.DenomUnits[0]). To ensure consistency across all units, consider iterating over allDenomUnitsto remove aliases and update denominations.Here is a suggested refactor:
func updateMetadata(ctx sdk.Context, bankKeeper bankkeeper.Keeper) { mds := bankKeeper.GetAllDenomMetaData(ctx) for _, md := range mds { if md.Base == fxtypes.DefaultDenom || len(md.DenomUnits) == 0 || len(md.DenomUnits[0].Aliases) == 0 { continue } // remove aliases from all denom units - md.DenomUnits[0].Aliases = []string{} + for i := range md.DenomUnits { + md.DenomUnits[i].Aliases = []string{} + } // update base denom newBase := strings.ToLower(md.Symbol) if md.Base != newBase { oldBase := md.Base md.Base = newBase // update denom in all denom units that match the old base - md.DenomUnits[0].Denom = newBase + for i := range md.DenomUnits { + if md.DenomUnits[i].Denom == oldBase { + md.DenomUnits[i].Denom = newBase + } + } } bankKeeper.SetDenomMetaData(ctx, md) } }Verify impact of base denomination changes
Changing
md.Baseand updating denom units may have implications elsewhere in the codebase where the original base denominations are referenced. Please ensure that all dependencies and references to the old base denominations are updated accordingly to prevent inconsistencies.x/erc20/module.go (1)
114-117: EnsurecrossChainKeepersis properly initialized before migrationPlease verify that
am.crossChainKeepersis properly initialized before it's used in the migration. An uninitialized or nilcrossChainKeepersmay lead to runtime errors during the migration process.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
erc20module for improved configuration and migration support.Chores