chore: migrate pundix chain balance #800
Conversation
WalkthroughThe changes in this pull request enhance the upgrade process for a Cosmos SDK application by introducing new functionalities and tests related to the Pundix chain migration. Key additions include new functions and methods for managing token migrations, specifically for Pundix and Purse tokens. The test suite is expanded to validate these migrations, ensuring that the application state aligns with expected conditions post-upgrade. The overall structure of the upgrade process is modified to incorporate these new migration steps and associated error handling. 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: 7
🧹 Outside diff range and nitpick comments (4)
app/upgrades/v8/pundix.go (4)
64-64: Address the TODO: Migrate other data.Line 64 contains a TODO comment indicating that other data needs to be migrated. Ensure that all necessary data migrations are implemented before the upgrade to avoid data inconsistency.
Would you like assistance in implementing the remaining data migrations? I can help generate the necessary code or open a GitHub issue to track this task.
96-96: Improve error message for clarity.The error message
"pundix ibc amount not match"is grammatically incorrect and may be unclear to users. Consider rephrasing it for better clarity.Suggested change:
- return sdkerrors.ErrInvalidCoins.Wrap("pundix ibc amount not match") + return sdkerrors.ErrInvalidCoins.Wrap("PUNDIX IBC amounts do not match")
127-127: Incomplete TODO: Migrate staking and rewards for module accounts.Line 127 contains a TODO comment indicating that staking and reward migrations for module accounts are pending. This is crucial to ensure that module accounts maintain correct balances post-migration.
Do you need assistance with implementing the migration for staking and rewards? I can help generate the necessary code or open a GitHub issue to track this task.
276-276: Incomplete TODO: Migrate staking and rewards for module accounts.Similar to the PUNDIX migration, the PURSE migration function has a TODO comment at line 276 regarding the migration of staking and rewards for module accounts.
Would you like help in implementing this migration step? I can assist in generating the necessary code or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/upgrade_test.go(4 hunks)app/upgrades/v8/pundix.go(1 hunks)app/upgrades/v8/upgrade.go(1 hunks)x/erc20/migrations/v8/migrate.go(3 hunks)
🔇 Additional comments (13)
x/erc20/migrations/v8/migrate.go (3)
12-13: LGTM: Import additions are appropriate.
The new imports for BSC and crosschain types are necessary for the added BSC bridge token functionality.
108-124: 🛠️ Refactor suggestion
Consider enhancing error handling and validation.
The implementation is solid, but could benefit from a few improvements:
- The error message could be more specific about why the bridge token wasn't found
- Consider validating the token's existence in the ERC20 registry before adding it as a bridge token
Consider applying these improvements:
func (m Migrator) addBscBridgePurse(ctx sdk.Context, newBaseDenom, base string) error {
for _, ck := range m.crosschainKeepers {
if ck.ModuleName() != bsctypes.ModuleName {
continue
}
legacyBridgeToken, found := ck.LegacyGetDenomBridgeToken(ctx, base)
if !found {
- return sdkerrors.ErrKeyNotFound.Wrapf("module %s bridge token: %s", ck.ModuleName(), base)
+ return sdkerrors.ErrKeyNotFound.Wrapf("BSC module bridge token not found for IBC denom: %s", base)
}
+ // Verify token exists in ERC20 registry
+ if _, err := m.keeper.GetERC20Token(ctx, newBaseDenom); err != nil {
+ return sdkerrors.Wrapf(err, "token %s not registered in ERC20 module", newBaseDenom)
+ }
bridgeDenom := crosschaintypes.NewBridgeDenom(ck.ModuleName(), legacyBridgeToken.Token)
ctx.Logger().Info("add bridge token", "base-denom", newBaseDenom, "alias", bridgeDenom, "module", ck.ModuleName(), "contract", legacyBridgeToken.Token)
if err := m.keeper.AddBridgeToken(ctx, newBaseDenom, ck.ModuleName(), legacyBridgeToken.Token, false); err != nil {
return err
}
}
return nil
}Let's verify the token validation pattern across the codebase:
#!/bin/bash
# Check how other migration functions validate tokens
ast-grep --pattern 'keeper.GetERC20Token($token)'44-50: LGTM: BSC bridge token migration logic is well-integrated.
The addition of BSC bridge token migration for IBC denoms is properly implemented with appropriate error handling.
Let's verify the IBC denom handling:
✅ Verification successful
These commands will help us verify the IBC denom handling patterns and BSC bridge token integration across the codebase.
LGTM: IBC denom handling for BSC bridge tokens is correctly implemented
The verification confirms that:
- The IBC denom prefix check (
ibctransfertypes.DenomPrefix+"/") is consistently used across the codebase for identifying IBC tokens - The
addBscBridgePursefunction is properly integrated into the migration flow and specifically handles BSC module tokens - The implementation aligns with existing IBC token handling patterns in the codebase, particularly in the transfer and crosschain modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other IBC denom handling patterns in the codebase
rg -A 3 "ibctransfertypes.DenomPrefix" --type go
Length of output: 137
Script:
#!/bin/bash
# Let's try a different approach to search for IBC denom handling
rg "DenomPrefix" --type go -A 3
Length of output: 3028
Script:
#!/bin/bash
# Search for BSC bridge token handling patterns
rg "addBscBridgePurse" --type go -A 3
Length of output: 591
Script:
#!/bin/bash
# Look for IBC transfer type imports and usages
rg "ibctransfertypes" --type go -A 3
Length of output: 10058
app/upgrades/v8/upgrade.go (1)
58-60: Verify migration sequence dependencies.
The Pundix migration is positioned between governance parameter migration and bridge balance migration. Please verify that this sequence is correct and that the Pundix migration:
- Doesn't depend on bridge balances that haven't been migrated yet
- Doesn't affect governance parameters that were just migrated
✅ Verification successful
Migration sequence is safe and correctly ordered
After analyzing the code, I can confirm the Pundix migration sequence is correctly positioned:
-
The Pundix migration does not depend on bridge balances:
- It only reads from the pundix genesis file and migrates PUNDIX/PURSE tokens
- The bridge-related operations are isolated to token denomination handling and don't depend on the bridge balance migration that happens later
-
The governance parameter migration is independent:
- The only governance interaction in Pundix migration is setting the ERC20 token owner to the governance module
- This is a one-way operation that doesn't read or depend on any governance parameters that were migrated earlier
The sequence in upgrade.go is logically ordered:
- Governance params migration
- Pundix migration
- Bridge balance migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependencies between Pundix migration and other components
# Check for bridge balance dependencies
rg -A 5 "bridge.*balance|balance.*bridge" "app/upgrades/v8/pundix.go"
# Check for governance parameter dependencies
rg -A 5 "gov.*param|param.*gov" "app/upgrades/v8/pundix.go"
Length of output: 129
Script:
#!/bin/bash
# Let's examine the pundix migration implementation and its dependencies
# First, check the content of pundix.go
cat app/upgrades/v8/pundix.go
# Also check the upgrade sequence in upgrade.go
cat app/upgrades/v8/upgrade.go
# Look for any bridge-related imports or references
rg -l "bridge" app/upgrades/v8/
# Look for any governance-related imports or references
rg -l "gov" app/upgrades/v8/
Length of output: 20339
app/upgrade_test.go (8)
4-5: Approved: Added necessary imports for hashing and JSON handling
The imports of "crypto/sha256" and "encoding/json" are appropriate and required for cryptographic hashing and JSON operations used later in the code.
28-28: Approved: Imported IBC transfer types
Adding ibctransfertypes import is essential for handling IBC denomination calculations in the code.
35-38: Approved: Imported Function X modules and contracts
The imports for contract, helpers, fxtypes, and bsctypes are necessary for the new functionalities introduced in the tests.
40-41: Approved: Imported ERC20 and Ethereum module types
The addition of erc20types and ethtypes imports is appropriate for ERC20 token handling and Ethereum module interactions.
125-125: Verify integration of checkPundixChainMigrate in upgrade tests
The call to checkPundixChainMigrate(t, ctx, myApp) integrates new migration checks into the upgrade test suite. Ensure this addition doesn't negatively impact existing tests and that all dependencies are properly managed.
177-185: Approved: Added checkPundixChainMigrate function
The new function checkPundixChainMigrate appropriately reads the genesis state and initiates bank state checks for the Pundix chain migration. The use of t.Helper() simplifies debugging in case of test failures.
187-236: Approved: Added checkPundixBank function for detailed bank state verification
The checkPundixBank function provides thorough verification of token balances and supply post-migration, ensuring consistency between the bank module and ERC20 token states. The comprehensive checks enhance test coverage for the migration process.
225-226: Verify correct Bech32 prefix when decoding addresses
Using "px" as the Bech32 prefix in sdk.GetFromBech32(bal.Address, "px") assumes all addresses have this prefix. If the Pundix chain uses a different prefix or supports multiple prefixes, this could cause address decoding errors. Confirm that "px" is the correct and only prefix for addresses in this context.
You can run the following script to check for any addresses with unexpected prefixes:
app/upgrades/v8/pundix.go (1)
193-193: Verify the Bech32 prefix "px" is correct.
In the function GetPxChannelEscrowAddr, the Bech32 prefix "px" is used. Please verify that this prefix is consistent with the expected address format in your application.
Run the following script to list all Bech32 prefixes used in the codebase:
Co-authored-by: nulnut <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/upgrade_test.go (1)
190-239: Consider decomposing the function for better maintainabilityThe function handles multiple responsibilities (purse token, pundix token, balance checks). Consider splitting it into smaller, focused functions:
validatePundixTokenvalidatePurseTokenvalidateAccountBalancesExample refactor:
func validatePundixToken(t *testing.T, ctx sdk.Context, myApp *app.App) (string, *erc20types.Token) { pundixToken, err := myApp.Erc20Keeper.GetERC20Token(ctx, "pundix") require.NoError(t, err) pundixBridgeToken, err := myApp.Erc20Keeper.GetBridgeToken(ctx, ethtypes.ModuleName, pundixToken.GetDenom()) require.NoError(t, err) pundixDenomHash := sha256.Sum256([]byte(fmt.Sprintf("%s/channel-0/%s", ibctransfertypes.ModuleName, pundixBridgeToken.BridgeDenom()))) pundixIBCDenom := fmt.Sprintf("%s/%X", ibctransfertypes.DenomPrefix, pundixDenomHash[:]) return pundixIBCDenom, pundixToken }app/upgrades/v8/pundix.go (3)
38-45: Add documentation for the Pundix struct.Consider adding godoc comments to describe the purpose of the struct and its fields. This will help future maintainers understand the role of each keeper.
+// Pundix handles the migration of PUNDIX and PURSE tokens during the v8 upgrade. type Pundix struct { + // accountKeeper manages account-related operations accountKeeper authkeeper.AccountKeeper + // erc20Keeper handles ERC20 token operations erc20Keeper erc20keeper.Keeper + // bankKeeper manages token balances bankKeeper bankkeeper.Keeper + // ibcTransferKeeper handles IBC transfer operations ibcTransferKeeper ibctransferkeeper.Keeper + // erc20TokenKeeper interacts with ERC20 contracts erc20TokenKeeper contract.ERC20TokenKeeper + // storeKey provides access to the IBC transfer store storeKey storetypes.StoreKey }
130-134: Address the TODO comment about staking and reward migration.The code skips module accounts with a TODO comment about staking and reward migration. This needs to be addressed to ensure complete migration.
Would you like me to help create a GitHub issue to track the implementation of staking and reward migration?
323-336: Enhance error handling in ReadGenesisState.The function should validate the genesis file content and handle edge cases better.
func ReadGenesisState(genesisPath string) (map[string]json.RawMessage, error) { genesisFile, err := os.ReadFile(genesisPath) if err != nil { return nil, err } + if len(genesisFile) == 0 { + return nil, fmt.Errorf("genesis file is empty: %s", genesisPath) + } genesisState := make(map[string]json.RawMessage) if err = tmjson.Unmarshal(genesisFile, &genesisState); err != nil { return nil, err } + if _, ok := genesisState["app_state"]; !ok { + return nil, fmt.Errorf("app_state not found in genesis file") + } appState := make(map[string]json.RawMessage) appStateBz := genesisState["app_state"] err = tmjson.Unmarshal(appStateBz, &appState) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal app_state: %w", err) + } return appState, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/upgrade_test.go(4 hunks)app/upgrades/v8/pundix.go(1 hunks)app/upgrades/v8/upgrade.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v8/upgrade.go
🔇 Additional comments (4)
app/upgrade_test.go (2)
211-219: LGTM: Thorough token supply validation
The implementation correctly validates:
- Total supply matching between ERC20 and bank module
- Module balance consistency
- Proper error handling for external calls
235-237:
Use exact balance comparison for precise validation
As noted in previous reviews, using GTE for balance comparison may mask issues where extra tokens are present. Consider using exact matching for more precise validation.
-require.True(t, allBal.AmountOf(pundixToken.GetDenom()).GTE(bal.Coins.AmountOf(pundixIBCDenom)))
-require.True(t, allBal.AmountOf(purseToken.GetDenom()).GTE(bal.Coins.AmountOf(purseBscBridgeToken.BridgeDenom())))
+require.Equal(t, allBal.AmountOf(pundixToken.GetDenom()), bal.Coins.AmountOf(pundixIBCDenom))
+require.Equal(t, allBal.AmountOf(purseToken.GetDenom()), bal.Coins.AmountOf(purseBscBridgeToken.BridgeDenom()))app/upgrades/v8/pundix.go (2)
58-70: 🛠️ Refactor suggestion
Consider parameterizing the genesis path.
The genesis path is hardcoded, which could cause issues in different environments or during testing. Consider making it configurable.
-func (m *Pundix) Migrate(ctx sdk.Context) error {
+func (m *Pundix) Migrate(ctx sdk.Context, genesisPath string) error {
- pundixGenesisPath := path.Join(fxtypes.GetDefaultNodeHome(), "config/pundix_genesis.json")
- appState, err := ReadGenesisState(pundixGenesisPath)
+ appState, err := ReadGenesisState(genesisPath)
if err != nil {
return err
}Likely invalid or redundant comment.
83-144: Verify token amounts and migrations in a test environment.
The migration involves complex token calculations and transfers. Please ensure:
- Token amounts are correctly calculated
- No tokens are lost during migration
- All balances match expected values post-migration
Consider adding integration tests and performing a dry run in a test environment before executing in production.
Also applies to: 146-180
Summary by CodeRabbit
New Features
Bug Fixes
Tests