refactor: add bridge token #912
Conversation
WalkthroughThis pull request introduces significant changes to token handling across multiple components of the cross-chain and ERC20 modules. The modifications focus on standardizing token metadata management, introducing new methods for checking token existence, and simplifying token registration processes. Key changes include updating method signatures to use metadata objects, adding methods to check for ERC20 token existence, and removing deprecated token deployment methods. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Keeper
participant EVMKeeper
participant TokenRegistry
Caller->>Keeper: AddERC20Token(metadata)
Keeper->>TokenRegistry: Check token existence
TokenRegistry-->>Keeper: Token status
alt Token does not exist
Keeper->>EVMKeeper: Deploy contract
EVMKeeper-->>Keeper: Contract address
Keeper->>TokenRegistry: Register token
end
Keeper-->>Caller: Confirmation
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
🧹 Nitpick comments (2)
x/crosschain/keeper/bridge_token.go (2)
22-22: Improve clarity of the error messageThe error message could be rephrased for better readability. Consider updating it to:
"%s token decimals do not match %d; expected %d"Apply this diff to adjust the error message:
return types.ErrInvalid.Wrapf("%s denom decimals not match %d, expect %d", +return types.ErrInvalid.Wrapf("%s token decimals do not match %d; expected %d", fxtypes.DefaultSymbol, claim.Decimals, fxtypes.DenomUnit)
32-39: Check the order of operations for token registrationCurrently,
AddBridgeTokenis called before checking if the ERC20 token exists withHasERC20Token. It might be more efficient to verify the token's existence first to avoid unnecessary processing if the token already exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
precompiles/bank/contract_test.go(1 hunks)x/crosschain/keeper/bridge_token.go(1 hunks)x/crosschain/mock/expected_keepers_mocks.go(2 hunks)x/crosschain/types/expected_keepers.go(1 hunks)x/erc20/keeper/contract.go(0 hunks)x/erc20/keeper/erc20_token.go(2 hunks)x/erc20/keeper/proposal.go(3 hunks)
💤 Files with no reviewable changes (1)
- x/erc20/keeper/contract.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (10)
x/crosschain/types/expected_keepers.go (1)
60-61: LGTM! Well-designed interface methods.The new methods
HasERC20TokenandRegisterNativeCoinare well-designed, following Go naming conventions and maintaining consistency with the existing interface methods.x/crosschain/mock/expected_keepers_mocks.go (2)
497-510: LGTM! Well-implemented mock method.The mock implementation for
HasERC20Tokencorrectly follows the gomock patterns and provides proper test helpers.
526-539: LGTM! Well-implemented mock method.The mock implementation for
RegisterNativeCoincorrectly follows the gomock patterns and provides proper test helpers.precompiles/bank/contract_test.go (1)
50-51: LGTM! Improved token metadata handling.The change improves token metadata handling by using
fxtypes.NewMetadatato create a structured metadata object, which is a better approach than passing individual parameters.x/erc20/keeper/erc20_token.go (2)
Line range hint
14-43: FunctionAddERC20Tokensuccessfully refactored to useMetadataThe method now accepts a
banktypes.Metadataobject, consolidating token metadata handling and simplifying the interface. The implementation properly validates the metadata and ensures that tokens are not duplicated.
47-49: New methodHasERC20Tokencorrectly implementedThe
HasERC20Tokenmethod provides a convenient way to check for the existence of an ERC20 token by its base denomination. This enhances the token management capabilities of theKeeper.x/crosschain/keeper/bridge_token.go (1)
18-24: Verify the correctness of the base denomination assignmentThe logic for assigning
baseDenombased onclaim.Symbolmay have implications for token handling:
- When
claim.Symbolmatchesfxtypes.DefaultSymbol,baseDenomis set tofxtypes.DefaultDenom.- Otherwise,
baseDenomis the lowercase ofclaim.Symbol.Ensure that this approach aligns with the intended token denomination mapping and does not cause conflicts with existing tokens.
x/erc20/keeper/proposal.go (3)
24-34: Refactored token registration to handle default and custom tokens appropriatelyThe introduction of
tokenContractandmetadatavariables enhances the flexibility ofRegisterNativeCoin. The conditional logic correctly differentiates between the default symbol and other tokens, using the appropriate contracts and metadata.
32-34: Ensure consistent handling ofdecimalstypeWhile
decimalsis of typeuint8, it is converted touint32when creatingmetadata. Confirm that this type conversion is intentional and that it does not lead to inconsistencies.
66-67: Properly creating metadata inRegisterNativeERC20The
metadatais correctly initialized usingfxtypes.NewMetadata, ensuring that the token's name, symbol, and decimals are accurately represented. This aligns with the updatedAddERC20Tokenmethod.
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
New Features
Refactoring
Chores
The changes focus on improving token management and cross-chain functionality with more robust metadata handling and error checking.