fix: update FX bridge token #954
Conversation
WalkthroughThis pull request introduces significant changes to the bridge token and denomination management in the application's upgrade process. The modifications include renaming functions, updating contract deployment logic, and refactoring the bridge token denomination update mechanism. The changes primarily focus on enhancing the flexibility of bridge token handling across different network environments (testnet and mainnet) by centralizing and standardizing the upgrade and validation processes. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Keeper as AppKeepers
participant Contract as ContractManager
participant ERC20 as ERC20Keeper
App->>Keeper: Initiate Upgrade
Keeper->>Contract: updateContract()
Contract->>Contract: Deploy BridgeFee Contract
Contract->>Contract: Deploy AccessControl Contract
Contract->>Contract: Update WPUNDIAI Logic
Contract->>Contract: Update ERC20 Logic
App->>ERC20: updateFXBridgeDenom()
ERC20->>ERC20: Retrieve ERC20 Token
ERC20->>ERC20: Update Denomination Index
ERC20->>ERC20: Remove Legacy Denomination
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: 0
🧹 Nitpick comments (4)
app/upgrades/v8/upgrade.go (2)
58-60: Consider adding idempotency checks or logging.
This newly introducedupgradeTestnetfunction simply delegates the testnet upgrade toupdateFXBridgeDenom. If the upgrade is called multiple times, it may partially overwrite or remove existing records. For better maintainability and debugging, consider logging the outcome to confirm that the denom was successfully updated.
113-113: Add a success log for contract updates.
The call toupdateContractis critical, but there's no log statement indicating whether it succeeded. Consider adding a log line upon success to ease troubleshooting in production environments.app/upgrades/v8/erc20.go (1)
25-49: Consider logging and ensuring a complete rollback or consistent state.
While updating the FX bridge denom, this routine removes the old token record and index before fully registering the new denom. If any subsequent step fails, the old entry remains deleted. For transparency and easier troubleshooting, you could log the old → new denom transition and confirm that re-running (idempotency) won’t cause inconsistent data.app/upgrades/v8/contract.go (1)
18-40: Ensure partial failure handling or logging.
This newupdateContractfunction deploys multiple contracts and updates logic codes in sequence. If an error occurs mid-deployment (e.g., afterBridgeFeeContractbut beforeAccessControlContract), the partially deployed changes remain. Consider adding a success logger or clarifying if the entire flow is transactional at the chain level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/upgrade_test.go(4 hunks)app/upgrades/v8/contract.go(1 hunks)app/upgrades/v8/erc20.go(2 hunks)app/upgrades/v8/testnet.go(0 hunks)app/upgrades/v8/upgrade.go(2 hunks)
💤 Files with no reviewable changes (1)
- app/upgrades/v8/testnet.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (7)
app/upgrades/v8/upgrade.go (1)
122-124: Verify multiple calls toupdateFXBridgeDenom.
This call toupdateFXBridgeDenomis repeated for the mainnet scenario, while a similar call also exists for the testnet scenario. Please confirm if this duplication is intentional (e.g., if different chain IDs require the same denom update). Otherwise, consider consolidating the update to avoid unintended repeated operations.app/upgrades/v8/erc20.go (2)
4-4: Confirmed usage of collections import.
The import for"cosmossdk.io/collections"is valid and used for joining keys at line 37.
9-10: ERC20 and ETH types imports are valid.
Theerc20typesandethtypesimports are referenced in the new functionupdateFXBridgeDenom.app/upgrades/v8/contract.go (2)
8-8: Import of evmtypes is valid.
This addition is necessary for referencingevmtypes.ModuleNameand other EVM-related components.
10-10: New keepers import is appropriate.
The import fromgithub.com/pundiai/fx-core/v8/app/keepersis needed to accessAppKeepers.app/upgrade_test.go (2)
Line range hint
678-694: LGTM! Bridge token verification looks good.The function correctly verifies:
- Bridge token mapping for FX denom
- Removal of legacy FX denom from bridge tokens
- Bridge denom index mapping
- Removal of legacy FX denom from ERC20 tokens
695-699: LGTM! PundiAI ERC20 token verification is complete.The function properly verifies:
- PundiAI ERC20 token existence
- Correct denomination index mapping for the token
Summary by CodeRabbit
New Features
Refactor
Bug Fixes