refactor: rename OriginalFXDenom to LegacyFXDenom #926
Conversation
WalkthroughThe pull request introduces a comprehensive change in handling token denominations across multiple files. The primary modification involves replacing Changes
Sequence DiagramsequenceDiagram
participant Types as Token Types
participant Migrator as Token Migrator
participant Middleware as IBC Middleware
Types->>Migrator: Provide LegacyFXDenom
Migrator->>Migrator: Process token migration
Migrator->>Middleware: Transfer tokens
Middleware->>Middleware: Validate denomination
Middleware-->>Migrator: Confirm transfer
Possibly related PRs
Poem
✨ Finishing Touches
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: 1
🔭 Outside diff range comments (2)
app/upgrades/v8/upgrade.go (2)
Line range hint
365-371: Consider adding validation for metadata updates.The metadata update logic skips
LegacyFXDenombut doesn't validate that the new base denomination doesn't already exist. This could potentially lead to conflicts.if md.Base == fxtypes.LegacyFXDenom || (len(md.DenomUnits) == 0 || len(md.DenomUnits[0].Aliases) == 0) && md.Symbol != pundixSymbol { continue } +// Validate that the new base denom doesn't exist +newBase := strings.ToLower(md.Symbol) +if bankKeeper.HasDenomMetaData(ctx, newBase) && newBase != md.Base { + return fmt.Errorf("metadata for denom %s already exists", newBase) +}
Line range hint
704-744: Add transaction boundary for bank module migration.The bank module migration performs multiple state changes without a clear transaction boundary. If any operation fails mid-way, it could leave the system in an inconsistent state.
Consider wrapping the entire migration in a transaction:
+func migrateBankModule(ctx sdk.Context, bankKeeper bankkeeper.Keeper) error { + // Create a cached context + cacheCtx, commit := ctx.CacheContext() + sendEnabledEntry, found := bankKeeper.GetSendEnabledEntry(ctx, fxtypes.LegacyFXDenom) if found { bankKeeper.DeleteSendEnabled(ctx, fxtypes.LegacyFXDenom) bankKeeper.SetSendEnabled(ctx, fxtypes.DefaultDenom, sendEnabledEntry.Enabled) } // ... rest of the migration code ... + // If we get here, commit all changes + commit() + return nil +}
🧹 Nitpick comments (3)
types/constant.go (1)
23-24: LGTM! Consider adding documentation.The addition of
LegacyFXDenomand its relationship withFXDenomis clear. Consider adding comments to document the purpose and distinction between these constants.+// LegacyFXDenom represents the legacy FX token denomination in uppercase LegacyFXDenom = "FX" +// FXDenom represents the current FX token denomination in lowercase FXDenom = "fx"x/erc20/migrations/v8/keys.go (1)
76-76: LGTM! Consider extracting the condition for better readability.The migration logic correctly handles both
LegacyFXDenomand lowercase symbol cases. Consider extracting the condition into a helper function for better readability.+func isSpecialDenom(md banktypes.Metadata) bool { + return md.Base == fxtypes.LegacyFXDenom || md.Base == strings.ToLower(md.Symbol) +} + func (m Migrator) migrateTokenPair(ctx sdk.Context, store storetypes.KVStore) error { // ... - if md.Base == fxtypes.LegacyFXDenom || md.Base == strings.ToLower(md.Symbol) { + if isSpecialDenom(md) { // ... } // ... }app/upgrades/v8/upgrade.go (1)
Line range hint
749-757: Add validation for chain ID in escrow denoms migration.The function assumes the chain ID will be either mainnet or testnet but doesn't handle invalid chain IDs.
func GetMigrateEscrowDenoms(chainID string) map[string]string { + if chainID != fxtypes.MainnetChainId && chainID != fxtypes.TestnetChainId { + panic(fmt.Sprintf("unsupported chain ID: %s", chainID)) + } result := make(map[string]string, 2) result[fxtypes.LegacyFXDenom] = fxtypes.DefaultDenom // ... rest of the function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/upgrade_test.go(3 hunks)app/upgrades/v8/upgrade.go(6 hunks)types/constant.go(1 hunks)types/denom_wrap.go(2 hunks)types/denom_wrap_test.go(2 hunks)x/erc20/migrations/v8/keys.go(1 hunks)x/erc20/migrations/v8/migrate.go(2 hunks)x/ibc/middleware/keeper/relay.go(1 hunks)x/ibc/middleware/keeper/relay_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (7)
types/denom_wrap.go (1)
28-29: LGTM! Consistent denomination updates.The replacement of
IBCFXDenomwithLegacyFXDenomin the denomination mapping is consistent with the PR's objective.Also applies to: 40-41
types/denom_wrap_test.go (1)
32-32: LGTM! Test cases updated consistently.The test cases have been properly updated to use
LegacyFXDenom, maintaining test coverage for the denomination changes.Also applies to: 40-40, 56-56, 64-64
x/erc20/migrations/v8/migrate.go (1)
20-20: LGTM! Consistent replacement of denomination source.The changes correctly replace the function call
OriginalFXDenom()with the constantLegacyFXDenom, maintaining the same logic while improving code consistency.Also applies to: 27-27
x/ibc/middleware/keeper/relay.go (1)
142-142: LGTM! Consistent denomination handling in IBC packet processing.The change correctly updates the denomination check from
IBCFXDenomtoLegacyFXDenom, maintaining consistency with the broader refactoring effort.x/ibc/middleware/keeper/relay_test.go (1)
151-151: LGTM! Test data updated to match implementation changes.The test case correctly uses
LegacyFXDenomto align with the changes in the implementation code.app/upgrade_test.go (1)
645-647: LGTM! Test validations updated consistently.The changes correctly update all test validations to use
LegacyFXDenom, maintaining consistency across:
- ERC20 token existence checks
- Balance denomination validations
- Send-enabled entry validations
Also applies to: 672-672, 683-683
app/upgrades/v8/upgrade.go (1)
Line range hint
227-233: Verify the token amount conversion during escrow migration.The code swaps the amount for
LegacyFXDenomtokens in escrow. Ensure that:
- The swap ratio is correctly applied
- No precision is lost during the conversion
- All escrow balances are properly accounted for
✅ Verification successful
Token amount conversion during escrow migration is correctly implemented
The conversion implementation is safe and precise:
- Uses integer division (QuoRaw) for 100:1 ratio
- Properly zeroes out old denomination before setting new one
- Maintains precision without floating-point operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify escrow balances and conversion ratios # Check for any remaining LegacyFXDenom in escrow rg -A 5 "SetTotalEscrowForDenom.*LegacyFXDenom" # Check for SwapAmount implementation ast-grep --pattern 'func SwapAmount($_) $_'Length of output: 237
| erc20Token, err := keeper.GetERC20Token(ctx, fxtypes.LegacyFXDenom) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| erc20Token.Denom = fxtypes.DefaultDenom | ||
| if err = keeper.ERC20Token.Set(ctx, erc20Token.Denom, erc20Token); err != nil { | ||
| return err | ||
| } | ||
| return keeper.ERC20Token.Remove(ctx, fxDenom) | ||
| return keeper.ERC20Token.Remove(ctx, fxtypes.LegacyFXDenom) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for token removal.
The function removes the old ERC20 token after setting the new one, but there's no rollback mechanism if the removal fails.
if err = keeper.ERC20Token.Set(ctx, erc20Token.Denom, erc20Token); err != nil {
return err
}
-return keeper.ERC20Token.Remove(ctx, fxtypes.LegacyFXDenom)
+if err = keeper.ERC20Token.Remove(ctx, fxtypes.LegacyFXDenom); err != nil {
+ // Rollback the previous set operation
+ keeper.ERC20Token.Remove(ctx, erc20Token.Denom)
+ return fmt.Errorf("failed to remove legacy token: %w", err)
+}
+return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| erc20Token, err := keeper.GetERC20Token(ctx, fxtypes.LegacyFXDenom) | |
| if err != nil { | |
| return err | |
| } | |
| erc20Token.Denom = fxtypes.DefaultDenom | |
| if err = keeper.ERC20Token.Set(ctx, erc20Token.Denom, erc20Token); err != nil { | |
| return err | |
| } | |
| return keeper.ERC20Token.Remove(ctx, fxDenom) | |
| return keeper.ERC20Token.Remove(ctx, fxtypes.LegacyFXDenom) | |
| erc20Token, err := keeper.GetERC20Token(ctx, fxtypes.LegacyFXDenom) | |
| if err != nil { | |
| return err | |
| } | |
| erc20Token.Denom = fxtypes.DefaultDenom | |
| if err = keeper.ERC20Token.Set(ctx, erc20Token.Denom, erc20Token); err != nil { | |
| return err | |
| } | |
| if err = keeper.ERC20Token.Remove(ctx, fxtypes.LegacyFXDenom); err != nil { | |
| // Rollback the previous set operation | |
| keeper.ERC20Token.Remove(ctx, erc20Token.Denom) | |
| return fmt.Errorf("failed to remove legacy token: %w", err) | |
| } | |
| return nil |
Summary by CodeRabbit
Release Notes
New Features
LegacyFXDenomRefactor
IBCFXDenomandOriginalFXDenom()withLegacyFXDenomTests
The changes primarily focus on standardizing token denomination handling across the system.