fix: remove deprecated tokens from testnet #856
Conversation
WalkthroughThe pull request introduces modifications to the upgrade process, focusing on testnet-specific operations and balance validation. Changes are made across three files: Changes
Possibly related PRs
Poem
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: 0
🧹 Nitpick comments (3)
app/upgrades/v8/upgrade.go (3)
161-161: Clarify operator precedence.
The condition combines||and&&. To avoid confusion, consider adding parentheses for readability:-if md.Base == fxtypes.DefaultDenom || (len(md.DenomUnits) == 0 || len(md.DenomUnits[0].Aliases) == 0) && md.Symbol != pundixSymbol { +if md.Base == fxtypes.DefaultDenom || ((len(md.DenomUnits) == 0 || len(md.DenomUnits[0].Aliases) == 0) && md.Symbol != pundixSymbol) {
Line range hint
233-270: updateMetadata function considerations.
- The type-assertion to
bankkeeper.BaseKeepertightly couples the code to a specific implementation. If possible, use an interface-based approach for better extensibility.- Removing metadata only if it still exists is prudent. Error handling is correct.
- Consider additional logs or comments explaining why removing old metadata is safe and desired.
403-432: removeTestnetDeprecatedCoins approach.
- The usage of
slices.Containsfor each balance is convenient. However, for large sets of deprecated denominations, a set/dictionary lookup might be more performant.- The flow to burn coins by first sending to the
erc20types.ModuleNameis consistent with the removal logic in other modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/upgrade_test.go(2 hunks)app/upgrades/v8/constants.go(1 hunks)app/upgrades/v8/upgrade.go(8 hunks)
🔇 Additional comments (12)
app/upgrades/v8/upgrade.go (8)
6-7: Imports look consistent.
No issues found with these newly introduced imports (fmt,slices, etc.). They align with usage in the code (e.g.,slices.Contains) and follow standard Go practices.Also applies to: 10-10, 18-18, 20-21
54-58: Use additional logs if necessary.
The code properly retrieves the list of removed denominations and returns on error. If you need more visibility (e.g., to handle edge cases whereremoveDenomsmight be empty), consider adding a debug log statement. Otherwise, this looks fine.
65-68: Error handling is correct here.
The call to remove deprecated coins properly returns an error if anything fails. No concerns.
87-89: Metadata update call is straightforward.
Error handling is consistent. Good job integrating the metadata update into the upgrade flow.
97-100: Consistent error handling for ERC20 coin removal.
The removal of testnet ERC20-deprecated coins is structurally similar to other removal functions. No issues found.
149-153: BaseAccount type casting.
Creating a module account from aBaseAccountis correct unless you expect non-BaseAccount subtypes (e.g., vesting accounts). Confirm that crosschain accounts can never be vesting accounts. Otherwise, you might need a more flexible approach.
364-401: removeTestnetDeprecatedDenom logic.
- Properly returns nil if not on testnet.
- Halts and returns an error when token pair is not found (line 394). Verify whether this is expected or if some deprecated tokens may not have a token pair at all.
- Thorough logging is good.
434-450: removeTestnetERC20DeprecatedCoins function clarity.
- Checks if metadata display indicates an IBC token. This is a neat logic trick but might benefit from an explanatory code comment for future maintainers.
- Burning coins is consistent with the overall approach to deprecate testnet tokens.
app/upgrades/v8/constants.go (1)
63-66: New constants are clearly named.
DefiningpundixSymbolandpurseSymbolcentrally promotes consistent usage throughout the code. Great addition.app/upgrade_test.go (3)
193-193: Lenient check of account balances.
Replacing an equality check withGreaterOrEqualbroadens the acceptable range if some accounts lose all tokens (e.g., deprecated tokens were removed). Confirm this is intentional.
199-199: Removed chain ID restriction.
Now this logic always runs for the ERC20 module, potentially changing test coverage for non-Mainnet chains. Make sure this aligns with your desired coverage scope.
231-231: Zero or exact match check.
Allowing zero balances can mask unintended balance wipes if you expect specific amounts. Confirm that zero is an acceptable outcome before skipping the rest of the checks.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes