refactor(upgrade): split migration logic into multiple files #950
Conversation
WalkthroughThis pull request introduces a comprehensive upgrade package (v8) for a blockchain system, focusing on migrating and updating various module parameters, denominations, and smart contract configurations. The changes span multiple modules including bank, crosschain, ERC20, EVM, governance, and metadata management. The upgrade process involves renaming denominations, updating parameters, migrating token balances, and adjusting logging mechanisms across different components of the blockchain application. Changes
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for oldDenom, newDenom := range escrowDenoms { | ||
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | ||
| newAmount := totalEscrow.Amount | ||
| if oldDenom == fxtypes.LegacyFXDenom { | ||
| newAmount = fxtypes.SwapAmount(newAmount) | ||
| } | ||
| // first remove old denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | ||
| // then add new denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (16)
app/upgrades/v8/evm.go (1)
11-16: Add explanatory documentation and consider event logging.The function itself is straightforward, but consider:
- Adding a docstring to explain the reasoning behind overriding
EvmDenomandHeaderHashNum.- Emitting events or logs to indicate a parameter migration occurred for tracking and troubleshooting.
+// migrateEvmParams updates the EVM module parameters, setting the default +// EVM denomination and HeaderHashNum. Events/logs may be useful for auditing. func migrateEvmParams(ctx sdk.Context, evmKeeper *fxevmkeeper.Keeper) error { params := evmKeeper.GetParams(ctx) params.EvmDenom = fxtypes.DefaultDenom params.HeaderHashNum = evmtypes.DefaultHeaderHashNum return evmKeeper.SetParams(ctx, params) }app/upgrades/v8/bank.go (3)
14-19: Clarify SendEnabled logic for migrated denominations.Although the logic to transfer the "send enabled" flag from
LegacyFXDenomtoDefaultDenomlooks correct, consider adding a clear in-code comment explaining why these steps are necessary. It will help maintainers understand the rationale behind toggling send capabilities after removing the legacy entry.
25-28: Improve error message for type assertion.The error “bank keeper not implement bank.BaseKeeper” might be too terse. Consider including context on which keeper was expected and at which step, so administrators can quickly pinpoint misconfigurations if the type assertion fails.
- return errors.New("bank keeper not implement bank.BaseKeeper") + return errors.New("bankKeeper does not implement bank.BaseKeeper. Please ensure it is correctly configured for v8 migration.")
29-46: Validate coincident supply changes during iteration.While iterating over all balances, multiple coin updates occur. If other modules also read or mutate balances in the same block, consider carefully reviewing concurrency assumptions or adding locks if applicable.
app/upgrades/v8/crosschain.go (2)
30-46: Handle absent module account more gracefully.Currently, if the crosschain module account or its permissions are missing, an error is returned. Consider whether a fallback or an auto-creation mechanism might be appropriate, or at least log additional details to guide remediation steps in production.
48-56: Maintain backward compatibility checks for Oracle's DelegateAmount.Swapping the oracle delegate amount in place is straightforward. However, validate that any consumers of the
DelegateAmount(e.g., other chain modules or external APIs) can handle the newly swapped values without disruption.app/upgrades/v8/metadata.go (1)
68-86: Validate adjusted display fields across IBC tokens.The new logic sets
md.Displayto the first non-baseDenomUnit. This is convenient, but certain IBC tokens might use additional denominations or non-standard display names. Test thoroughly to ensure correct display for chains with unconventional denom unit structures.app/upgrades/v8/contract.go (2)
18-42: Validate usage oferrors.Newreturn paths.
The function correctly returns early on error conditions (e.g., no oracles found). For enhanced observability, consider wrapping errors to provide contextual details (e.g.fmt.Errorf("deployBridgeFeeContract: %w", err)).
75-77: Deployment logic for access control contract.
Function is succinct and reads well. You might add logging for success or failure outcomes, mirroring the pattern used in other deploy/update contract functions.app/upgrades/v8/bridge.go (3)
18-33: Consider logging faced errors inmigrateERC20TokenToCrosschain.
Errors are returned immediately, but adding logs before returning could make diagnosing migration failures easier.
54-81: Potentially revise complex condition checks inmigrateBridgeBalance.
Nested conditions for aliases, base denominations, and symbol checks might become harder to maintain as new denominations or edge cases appear. Consider extracting the logic into smaller helper functions for clarity.
83-110: Evaluate partial migration failures.
If a single balance migration fails, the iteration stops immediately. This prevents migrating other valid balances. Consider collecting errors or applying partial reverts if feasible.app/upgrades/v8/upgrade.go (1)
221-234:fixPundixCoin: Expose the rationale for minting.
Minting additional coins based on ERC20 supply is logically consistent but might merit clarifying logs or comments to explain the reason behind the minted amounts.app/upgrades/v8/constants.go (1)
88-98: Validate chain-specific denomination mapping.
The newGetMigrateEscrowDenomsfunction hardcodes logic for testnet vs. mainnet. This is fine for now, but consider centralizing chain-specific logic to avoid duplication.app/upgrades/v8/staking.go (1)
70-70: Return nil explicitly when no error is found.
Returningerrdirectly works, but returningnilfor the success path can be clearer.- return err + if err != nil { + return err + } + return nilapp/upgrades/v8/wrap_token.go (1)
56-56: Add more contextual fields to logs for enhanced observability.Including
"module", "upgrade"is beneficial, but consider adding details like the current block height or chain ID for better troubleshooting in production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/upgrades/store/store.go(1 hunks)app/upgrades/v8/bank.go(1 hunks)app/upgrades/v8/bridge.go(1 hunks)app/upgrades/v8/constants.go(2 hunks)app/upgrades/v8/contract.go(1 hunks)app/upgrades/v8/crisis.go(1 hunks)app/upgrades/v8/crosschain.go(1 hunks)app/upgrades/v8/erc20.go(1 hunks)app/upgrades/v8/evm.go(1 hunks)app/upgrades/v8/feegrant.go(0 hunks)app/upgrades/v8/feematket.go(1 hunks)app/upgrades/v8/gov.go(1 hunks)app/upgrades/v8/ibc.go(1 hunks)app/upgrades/v8/metadata.go(1 hunks)app/upgrades/v8/staking.go(1 hunks)app/upgrades/v8/upgrade.go(2 hunks)app/upgrades/v8/wrap_token.go(2 hunks)
💤 Files with no reviewable changes (1)
- app/upgrades/v8/feegrant.go
🧰 Additional context used
🪛 GitHub Check: CodeQL
app/upgrades/v8/ibc.go
[warning] 13-23: Iteration over map
Iteration over map may be a possible source of non-determinism
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (24)
app/upgrades/v8/evm.go (2)
1-2: No issues with the package name.Using a versioned package (
v8) for a significant upgrade is a clear and organized approach that helps keep the code modular.
3-9: Imports look correct and consistent.All required external packages are properly imported. Just ensure the references to
fx-core/v8are appropriately set in your go.mod to avoid version conflicts.app/upgrades/v8/bank.go (2)
21-24: Verify zero or negative supply edge cases.When retrieving and storing the supply for
LegacyFXDenom, confirm you have test coverage for zero or negative supply scenarios. This prevents unexpected panics or unhandled errors during migration if the supply is already empty or inadvertently set to a negative value in upstream logic.
51-55: Confirm final supply reconciliation.After removing
LegacyFXDenomand setting the new supply forDefaultDenom, verify that no corner cases exist where the new supply is miscalculated. For instance, if supply bridging or external calls to the supply are triggered within the same migration block, ensure consistent state across modules.app/upgrades/v8/crosschain.go (1)
14-28: Check boundary conditions on new DelegateThreshold.When adjusting
DelegateThresholdto the new default denomination and swapped amount, confirm it's tested with minimal and maximal threshold values to catch any precision or rounding issues.app/upgrades/v8/metadata.go (2)
39-53: Confirm dictionary-based metadata removal approach.You gather
removeMetadataand then remove them from theBaseViewKeeper. Double-check concurrency assumptions and indexing, as well as whether any immediate re-creation of metadata might conflict with these removals during the same block.
55-66: Ensure updated default metadata for the new denom is sufficiently comprehensive.When calling
NewDefaultMetadata()forpundiai, confirm it includes all the required fields (e.g., display, symbol, description, and any necessary denom units). Incomplete metadata can degrade UX for explorers or wallet integrations.app/upgrades/v8/contract.go (3)
1-2: Package and file structure look good.
No issues found with the package declaration and overall file structure.
4-16: Imports appear consistent and necessary.
All imports are used and relevant to the file context.
44-55: Confirm viability of forcibly deleting accounts.
evmKeeper.DeleteAccountis invoked for the contract and oracle addresses. Ensure there's no scenario where prior state or balances are needed post-upgrade. If partial refunds or record retention is required, you may want to handle it before deletion.app/upgrades/v8/bridge.go (1)
35-52: Ensure minted supply doesn’t exceed expected constraints.
The function calculates new mints based on IBC supply differences, which is correct, but double-check any existing supply constraints, especially for test environments or total supply ceilings.app/upgrades/v8/upgrade.go (2)
115-117: Flow check inupgradeMainnet: Migrate Gov params earlier or later?
You’re runningmigrateGovDefaultParamsaftermigrateGovCustomParam. Verify that default param overrides don’t unintentionally override or conflict with custom param migrations.
240-280:fixPurseCoin: Carefully unify multi-chain supplies.
The function merges supply from multiple chains (ETH, BSC, IBC) into a base coin. Confirm that each chain’s crosschain minted coins remain in sync with underlying contract logic and that modules handle new minted coins properly.app/upgrades/v8/feematket.go (2)
1-9: No issues with package declaration and imports
Everything here seems appropriate for the new migration logic.
11-16: Verify default gas price for production
Confirm thatfxtypes.DefaultGasPricealigns with your desired baseline. If it's too high/low, it could significantly impact transaction costs.app/upgrades/v8/crisis.go (2)
1-9: Imports are correct
All imported packages are consistent with the file’s purpose.
11-19: Check correctness for crisis module’s constant fee
Assigning133 * 1e18to the fee requires validating that it aligns with your chain’s supply and desired penalty level. Otherwise, it could become too large or too small.app/upgrades/v8/erc20.go (2)
1-9: No issues with package and imports
They align with the intended functionality for ERC20 migration.
10-20: Ensure robust handling of legacy token retrieval
Consider verifying that the old token actually exists. IfLegacyFXDenomdoesn’t exist, handle it gracefully rather than proceeding with an empty token.app/upgrades/store/store.go (1)
20-20: Improved logging context
Including"module", "upgrade"in the log statement enhances clarity for debugging migration operations.app/upgrades/v8/gov.go (2)
29-31: Ensure large deposit amount is correct.
A minimum deposit of 30×10^18 may be extremely high for certain governance proposals. Verify that this aligns with community expectations and usage patterns.
17-17: Double-check removed keys.
Ensure the removed store keys are no longer in use elsewhere in the codebase. This helps prevent accidental data loss or mismatch.✅ Verification successful
Verified: Store key removal is part of planned v8 migration
The removed keys (FxBaseParamsKeyPrefix,FxEGFParamsKey) are being replaced with new custom params as part of a coordinated v8 upgrade. The immediate initialization of new params and presence of tests confirm this is a safe, planned migration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that none of the removed keys are still in use. rg -A 3 -B 3 'fxgovv8.GetRemovedStoreKeys'Length of output: 1045
Script:
#!/bin/bash # Find the implementation of GetRemovedStoreKeys ast-grep --pattern 'func GetRemovedStoreKeys() []string { $$$ }' # In case it's not found with ast-grep, try ripgrep rg -A 5 "func GetRemovedStoreKeys\(\)"Length of output: 1196
app/upgrades/v8/constants.go (1)
64-66: Renamed constants improve clarity.
Thank you for renaming topundixBaseDenomandpurseBaseDenom—this is clearer.app/upgrades/v8/wrap_token.go (1)
68-68: Maintain consistent logging style across different upgrade actions.This log statement aligns well with the earlier log change. Ensuring consistent key-value pairs across all upgrade logs will help simplify log parsing and correlation.
| func updateMetadata(ctx sdk.Context, bankKeeper bankkeeper.Keeper) error { | ||
| mds := bankKeeper.GetAllDenomMetaData(ctx) | ||
|
|
||
| removeMetadata := make([]string, 0, 2) | ||
| for _, md := range mds { | ||
| if md.Base == fxtypes.LegacyFXDenom || (len(md.DenomUnits) == 0 || len(md.DenomUnits[0].Aliases) == 0) && md.Symbol != pundixSymbol { | ||
| continue | ||
| } | ||
| // remove alias | ||
| md.DenomUnits[0].Aliases = []string{} | ||
|
|
||
| newBase := strings.ToLower(md.Symbol) | ||
| // update pundix/purse base denom | ||
| if md.Base != newBase && !strings.Contains(md.Base, newBase) && !strings.HasPrefix(md.Display, ibctransfertypes.ModuleName+"/"+ibcchanneltypes.ChannelPrefix) { | ||
| removeMetadata = append(removeMetadata, md.Base) | ||
|
|
||
| md.Base = newBase | ||
| md.Display = newBase | ||
| md.DenomUnits[0].Denom = newBase | ||
| } | ||
|
|
||
| bankKeeper.SetDenomMetaData(ctx, md) | ||
| } | ||
|
|
There was a problem hiding this comment.
Refine alias removal logic.
Aliasing is stripped from each DenomUnits[0] array. While removing them outright seems intentional, ensure this action doesn’t break external references reliant on these aliases (e.g., wallets or blockchain explorers).
| func updateERC20LogicCode(ctx sdk.Context, keeper *fxevmkeeper.Keeper) { | ||
| erc20 := contract.GetERC20() | ||
| if err := keeper.UpdateContractCode(ctx, erc20.Address, erc20.Code); err != nil { | ||
| ctx.Logger().Error("update ERC20 contract", "module", "upgrade", "err", err.Error()) | ||
| } else { | ||
| ctx.Logger().Info("update ERC20 contract", "module", "upgrade", "codeHash", erc20.CodeHash()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the error-handling approach from WPUNDIAI to ERC20.
Same rationale applies: returning an error on failure ensures that upgrade processes can cleanly fail when a contract update is unsuccessful.
| func updateWPUNDIAILogicCode(ctx sdk.Context, keeper *fxevmkeeper.Keeper) { | ||
| wpundiai := contract.GetWPUNDIAI() | ||
| if err := keeper.UpdateContractCode(ctx, wpundiai.Address, wpundiai.Code); err != nil { | ||
| ctx.Logger().Error("update WPUNDIAI contract", "module", "upgrade", "err", err.Error()) | ||
| } else { | ||
| ctx.Logger().Info("update WPUNDIAI contract", "module", "upgrade", "codeHash", wpundiai.CodeHash()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning error after logging contract update failures.
Both success and error paths are appropriately logged, but the function silently swallows errors. This could mask issues during upgrades. Returning an error might help detect failures earlier.
func updateWPUNDIAILogicCode(ctx sdk.Context, keeper *fxevmkeeper.Keeper) error {
wpundiai := contract.GetWPUNDIAI()
- if err := keeper.UpdateContractCode(ctx, wpundiai.Address, wpundiai.Code); err != nil {
+ err := keeper.UpdateContractCode(ctx, wpundiai.Address, wpundiai.Code)
+ if err != nil {
ctx.Logger().Error("update WPUNDIAI contract", "module", "upgrade", "err", err.Error())
+ return err
} else {
ctx.Logger().Info("update WPUNDIAI contract", "module", "upgrade", "codeHash", wpundiai.CodeHash())
}
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.
| func updateWPUNDIAILogicCode(ctx sdk.Context, keeper *fxevmkeeper.Keeper) { | |
| wpundiai := contract.GetWPUNDIAI() | |
| if err := keeper.UpdateContractCode(ctx, wpundiai.Address, wpundiai.Code); err != nil { | |
| ctx.Logger().Error("update WPUNDIAI contract", "module", "upgrade", "err", err.Error()) | |
| } else { | |
| ctx.Logger().Info("update WPUNDIAI contract", "module", "upgrade", "codeHash", wpundiai.CodeHash()) | |
| } | |
| } | |
| func updateWPUNDIAILogicCode(ctx sdk.Context, keeper *fxevmkeeper.Keeper) error { | |
| wpundiai := contract.GetWPUNDIAI() | |
| err := keeper.UpdateContractCode(ctx, wpundiai.Address, wpundiai.Code) | |
| if err != nil { | |
| ctx.Logger().Error("update WPUNDIAI contract", "module", "upgrade", "err", err.Error()) | |
| return err | |
| } else { | |
| ctx.Logger().Info("update WPUNDIAI contract", "module", "upgrade", "codeHash", wpundiai.CodeHash()) | |
| } | |
| return nil | |
| } |
| for oldDenom, newDenom := range escrowDenoms { | ||
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | ||
| newAmount := totalEscrow.Amount | ||
| if oldDenom == fxtypes.LegacyFXDenom { | ||
| newAmount = fxtypes.SwapAmount(newAmount) | ||
| } | ||
| // first remove old denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | ||
| // then add new denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address non-deterministic iteration over map.
In a blockchain environment, iterating over a map directly can introduce non-determinism. Consider sorting keys before processing them to ensure a deterministic sequence of migration.
- for oldDenom, newDenom := range escrowDenoms {
+ sortedDenoms := make([]string, 0, len(escrowDenoms))
+ for denom := range escrowDenoms {
+ sortedDenoms = append(sortedDenoms, denom)
+ }
+ sort.Strings(sortedDenoms)
+
+ for _, oldDenom := range sortedDenoms {
+ newDenom := escrowDenoms[oldDenom]
totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom)
...📝 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.
| for oldDenom, newDenom := range escrowDenoms { | |
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | |
| newAmount := totalEscrow.Amount | |
| if oldDenom == fxtypes.LegacyFXDenom { | |
| newAmount = fxtypes.SwapAmount(newAmount) | |
| } | |
| // first remove old denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | |
| // then add new denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | |
| } | |
| sortedDenoms := make([]string, 0, len(escrowDenoms)) | |
| for denom := range escrowDenoms { | |
| sortedDenoms = append(sortedDenoms, denom) | |
| } | |
| sort.Strings(sortedDenoms) | |
| for _, oldDenom := range sortedDenoms { | |
| newDenom := escrowDenoms[oldDenom] | |
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | |
| newAmount := totalEscrow.Amount | |
| if oldDenom == fxtypes.LegacyFXDenom { | |
| newAmount = fxtypes.SwapAmount(newAmount) | |
| } | |
| // first remove old denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | |
| // then add new denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 13-23: Iteration over map
Iteration over map may be a possible source of non-determinism
Summary by CodeRabbit
Release Notes for v8 Upgrade
New Features
Improvements
Migrations
Technical Updates