feat: migrate WFX to WPUNDIAI #917
Conversation
WalkthroughThis pull request introduces a comprehensive upgrade mechanism for a wrapped token called WPUNDIAI (Wrapped Pundi AIFX Token). The changes span multiple files, including a new migration function in Changes
Sequence DiagramsequenceDiagram
participant App as Upgrade Process
participant Keeper as EVM Keeper
participant Token as Wrap Token Migration
App->>Keeper: Initiate Token Migration
Keeper->>Token: migrationWFXToWPUNDIAI()
Token-->>Token: Scale Down Token Supply
Token-->>Token: Update Token Metadata
Token-->>Keeper: Migration Complete
Keeper-->>App: Upgrade Successful
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: 4
🧹 Nitpick comments (3)
app/upgrades/v8/wrap_token.go (2)
20-25: Document the purpose of storage slot numbers.The storage slot numbers appear to be magic numbers without clear documentation explaining their significance or how they were chosen.
Add comments explaining the purpose and derivation of each slot number:
const ( + // Slot for storing the token name in EVM storage nameSlot = 201 + // Slot for storing the token symbol in EVM storage symbolSlot = 202 + // Slot for storing the total supply in EVM storage totalSupplySlot = 204 + // Last slot used in storage layout lastSlot = 300 )
89-93: Improve function naming and documentation.The function name hardcodes the scaling factor, making it less flexible for future changes.
Consider making the scaling factor configurable:
-func scaleDownBy100(valueByte []byte) []byte { +func scaleDown(valueByte []byte, factor int64) []byte { value := big.NewInt(0).SetBytes(valueByte) - newValue := value.Div(value, big.NewInt(100)) + newValue := value.Div(value, big.NewInt(factor)) return common.BigToHash(newValue).Bytes() }app/upgrade_test.go (1)
190-209: Enhance test coverage for token migration.The test verifies basic token properties but lacks validation of the scaled-down values and state migration.
Add tests for scaled-down values:
func checkWrapToken(t *testing.T, ctx sdk.Context, myApp *app.App) { t.Helper() erc20TokenKeeper := contract.NewERC20TokenKeeper(myApp.EvmKeeper) wrapTokenAddress := nextversion.GetWFXAddress(ctx.ChainID()) + + // Test basic properties name, err := erc20TokenKeeper.Name(ctx, wrapTokenAddress) require.NoError(t, err) assert.EqualValues(t, nextversion.WrapTokenName, name) symbol, err := erc20TokenKeeper.Symbol(ctx, wrapTokenAddress) require.NoError(t, err) assert.EqualValues(t, nextversion.WrapTokenSymbol, symbol) decimals, err := erc20TokenKeeper.Decimals(ctx, wrapTokenAddress) require.NoError(t, err) assert.EqualValues(t, 18, decimals) owner, err := erc20TokenKeeper.Owner(ctx, wrapTokenAddress) require.NoError(t, err) assert.EqualValues(t, common.BytesToAddress(myApp.AccountKeeper.GetModuleAddress(erc20types.ModuleName).Bytes()), owner) supply, err := erc20TokenKeeper.TotalSupply(ctx, wrapTokenAddress) require.NoError(t, err) assert.EqualValues(t, 1, supply.Cmp(big.NewInt(0))) + + // Test scaled-down values + testBalance := big.NewInt(1000) + testAddress := common.HexToAddress("0x1234567890123456789012345678901234567890") + err = erc20TokenKeeper.SetBalance(ctx, wrapTokenAddress, testAddress, testBalance) + require.NoError(t, err) + + balance, err := erc20TokenKeeper.BalanceOf(ctx, wrapTokenAddress, testAddress) + require.NoError(t, err) + expectedBalance := new(big.Int).Div(testBalance, big.NewInt(100)) + assert.Zero(t, balance.Cmp(expectedBalance)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/upgrade_test.go(3 hunks)app/upgrades/v8/upgrade.go(2 hunks)app/upgrades/v8/wrap_token.go(1 hunks)scripts/linter.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
scripts/linter.sh (1)
7-7: LGTM!The increase in #nosec limit is justified by the addition of the new security-related code in wrap_token.go.
|
|
||
| migrationWFXToWPUNDIAI(ctx, app.EvmKeeper) | ||
|
|
There was a problem hiding this comment.
Add error handling for migration function.
The migration function is called without error handling, which could lead to incomplete upgrades if it fails.
Add error handling:
- migrationWFXToWPUNDIAI(ctx, app.EvmKeeper)
+ if err := migrationWFXToWPUNDIAI(ctx, app.EvmKeeper); err != nil {
+ return err
+ }📝 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.
| migrationWFXToWPUNDIAI(ctx, app.EvmKeeper) | |
| if err := migrationWFXToWPUNDIAI(ctx, app.EvmKeeper); err != nil { | |
| return err | |
| } | |
Summary by CodeRabbit
New Features
Tests
Chores
#nosecpattern occurrence