feat: add token registration and update #870
Conversation
WalkthroughThis pull request introduces significant refactoring across multiple modules, primarily focusing on consolidating and standardizing the handling of external addresses, chain types, and message registrations. The changes involve updating import paths, modifying function calls, and introducing new message types for ERC20 token operations. The modifications span various packages, including crosschain, erc20, eth, and other chain-specific modules, with a consistent theme of centralizing type definitions and utility functions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgServer
participant Keeper
participant TypeRegistry
Client->>MsgServer: RegisterNativeCoin/ERC20
MsgServer->>MsgServer: Validate Authority
MsgServer->>Keeper: RegisterNativeCoin/ERC20
Keeper->>TypeRegistry: Validate and Register Token
TypeRegistry-->>Keeper: Token Registration Result
Keeper-->>MsgServer: Return Registered Token
MsgServer-->>Client: Response with Token Details
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: 7
🧹 Nitpick comments (4)
x/layer2/types/key.go (1)
1-1: Consider architectural implications of eth types coupling.While the refactoring to centralize external address handling in
fxtypesis good, the introduction ofethtypesdependency across all chain modules creates tight coupling. Consider moving theEthereumAddresstype to a more neutral package to maintain better separation of concerns.x/avalanche/types/key.go (1)
3-6: LGTM! Completes the consistent refactoring across chain modules.The changes maintain the systematic approach seen across all chain implementations.
The consistent refactoring across all chain modules improves the codebase by:
- Centralizing common types under
fxtypes- Isolating Ethereum-specific types in the
ethmodule- Maintaining a uniform pattern for external address registration
This architectural change should make it easier to:
- Add new chain implementations
- Maintain common functionality
- Update Ethereum-specific features
Also applies to: 17-17
types/external_address.go (1)
48-51: LGTM! New IsSupportChain function provides clean chain validation.The function provides a clean way to check chain support. However, consider enhancing error handling for consistency:
Consider returning an error instead of a boolean to align with the error handling pattern used in other functions like
ValidateExternalAddr:-func IsSupportChain(chainName string) bool { - _, ok := externalAddressRouter[chainName] - return ok +func IsSupportChain(chainName string) error { + if _, ok := externalAddressRouter[chainName]; !ok { + return fmt.Errorf("unrecognized crosschain name: %s", chainName) + } + return nil +}This would provide more detailed feedback and maintain consistency with the error handling pattern in the codebase.
x/crosschain/types/msgs.go (1)
15-16: Consider adding package documentation.Since this file contains many message types and validation logic, consider adding package documentation to explain the relationship between
fxtypesand the message validation.package types +// Package types provides message definitions and validation logic for the crosschain module. +// It relies on fxtypes for standardized chain support and address validation functionality. + import (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/fx/erc20/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/erc20/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (45)
app/encoding_test.go(1 hunks)app/interface_registry.json(3 hunks)app/upgrades/v8/upgrade.go(1 hunks)cmd/root.go(2 hunks)precompiles/bank/contract_test.go(2 hunks)precompiles/crosschain/contract_test.go(1 hunks)precompiles/crosschain/has_oracle.go(2 hunks)precompiles/crosschain/has_oracle_test.go(1 hunks)precompiles/crosschain/is_oracle_online.go(2 hunks)precompiles/crosschain/is_oracle_online_test.go(1 hunks)proto/fx/erc20/v1/tx.proto(2 hunks)tests/integration/crosschain_suite.go(5 hunks)tests/integration/crosschain_test.go(2 hunks)testutil/helpers/key.go(1 hunks)types/external_address.go(1 hunks)types/external_address_test.go(1 hunks)x/arbitrum/types/key.go(2 hunks)x/avalanche/types/key.go(2 hunks)x/bsc/types/key.go(2 hunks)x/crosschain/client/cli/query.go(3 hunks)x/crosschain/client/cli/tx.go(3 hunks)x/crosschain/keeper/bridge_account.go(1 hunks)x/crosschain/keeper/bridge_call_out.go(2 hunks)x/crosschain/keeper/confirm.go(2 hunks)x/crosschain/keeper/grpc_query.go(6 hunks)x/crosschain/keeper/keeper_test.go(2 hunks)x/crosschain/keeper/keeper_v1_test.go(2 hunks)x/crosschain/keeper/msg_server_test.go(2 hunks)x/crosschain/types/legacy.go(2 hunks)x/crosschain/types/msgs.go(27 hunks)x/crosschain/types/msgs_test.go(2 hunks)x/crosschain/types/target.go(2 hunks)x/erc20/keeper/erc20_token.go(5 hunks)x/erc20/keeper/msg_server.go(1 hunks)x/erc20/types/codec.go(2 hunks)x/erc20/types/msg.go(3 hunks)x/eth/types/address.go(1 hunks)x/eth/types/key.go(2 hunks)x/eth/types/signer.go(3 hunks)x/eth/types/signer_test.go(1 hunks)x/layer2/types/key.go(2 hunks)x/optimism/types/key.go(2 hunks)x/polygon/types/key.go(2 hunks)x/tron/types/address.go(1 hunks)x/tron/types/key.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/eth/types/signer_test.go
- x/crosschain/keeper/msg_server_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (47)
x/erc20/types/msg.go (1)
95-97: Verify if lowercasing the symbol during validation is appropriateIn the
ValidateBasicmethod ofMsgRegisterNativeCoin, the symbol is converted to lowercase before validation. Please verify if converting the symbol to lowercase is intended, as token symbols are often case-sensitive and typically displayed in uppercase (e.g., 'ETH', 'USDT').x/erc20/keeper/erc20_token.go (2)
73-75: Good addition of metadata validationAdding validation for the token metadata ensures that only valid tokens are registered, enhancing the robustness of the token registration process.
56-57: Ensure consistent context handling after changing context typeThe method
RegisterNativeERC20now usescontext.Contextinstead ofsdk.Contextand unwraps it usingsdk.UnwrapSDKContext(ctx). Please verify that this change is consistent across the codebase and does not introduce any unintended side effects, particularly regarding event emission and transaction context.x/tron/types/key.go (1)
3-5: Verify compatibility after replacingcrosschaintypeswithfxtypesThe import of
crosschaintypeshas been replaced withfxtypes, andRegisterExternalAddressis now called fromfxtypes. Ensure that this change maintains compatibility with existing functionality and that external address registrations continue to operate as expected.Also applies to: 16-16
x/eth/types/key.go (2)
16-16: LGTM! Consistent with other chain modules.The update to use
fxtypes.RegisterExternalAddressaligns with similar changes in other chain-specific modules.
3-5: Verify import path update impact.The migration from
crosschaintypestofxtypespackage is part of a larger refactoring effort.✅ Verification successful
Import path update is correctly implemented
The migration from
crosschaintypestofxtypesforRegisterExternalAddressis consistent across all chain modules and maintains the same functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old package rg "crosschaintypes" --type go # Verify all modules using RegisterExternalAddress have been updated rg "RegisterExternalAddress" --type goLength of output: 21122
x/crosschain/keeper/bridge_account.go (1)
6-6: Verify backward compatibility of ExternalAddrToAccAddr.The migration to core types package could affect account address generation.
✅ Verification successful
Import change is safe and maintains backward compatibility
The migration to core types package is verified to be safe as the ExternalAddrToAccAddr implementation maintains its routing mechanism and chain-specific address conversion logic. The function continues to delegate to appropriate chain-specific implementations through the established router pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any changes in ExternalAddrToAccAddr implementation ast-grep --pattern 'func ExternalAddrToAccAddr($_, $_) $_ { $$$ }'Length of output: 70
Script:
#!/bin/bash # Search for ExternalAddrToAccAddr in the codebase rg "ExternalAddrToAccAddr" -A 5 # Also check for the types package usage rg "types\.External" -A 2Length of output: 15748
x/bsc/types/key.go (2)
17-17: LGTM! Consistent with eth module implementation.The registration of external addresses follows the same pattern as the eth module.
3-6: Verify dependency on eth types package.The introduction of
ethtypesdependency creates a direct coupling with the eth module.✅ Verification successful
Dependency on eth types package is appropriate
The dependency on
eth/typesis justified as it provides common types for EVM-compatible chains. This is a clean, one-way dependency with no circular references, following the same pattern as other L2 modules (Arbitrum, Avalanche, Optimism, Polygon).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for circular dependencies rg -l "github.com/pundiai/fx-core/v8/x/eth/types" | xargs -I {} dirname {} | sort | uniqLength of output: 343
Script:
#!/bin/bash # Check if eth module imports anything from bsc rg -l "github.com/pundiai/fx-core/v8/x/bsc" x/ethLength of output: 49
x/layer2/types/key.go (1)
Line range hint
3-17: LGTM! Maintains consistency across chain modules.The changes follow the established pattern seen in other chain modules:
- Migration to
fxtypes- Introduction of
ethtypesdependency- Updated external address registration
x/polygon/types/key.go (2)
3-6: LGTM! Import changes align with the architectural refactoring.The replacement of
crosschaintypeswithfxtypesandethtypesfollows the new pattern of centralizing common types underfxtypeswhile keeping Ethereum-specific types in theethmodule.
17-17: Verify backward compatibility of the registration change.While the change looks correct, moving from
crosschaintypestofxtypesfor external address registration could potentially impact existing chain integrations.Run this script to check for any other modules still using the old pattern:
✅ Verification successful
Registration change is safe and complete
The migration from
crosschaintypestofxtypesfor external address registration is complete and consistent across all chain modules. Each module properly registers its address type using the new pattern, and the implementation is centralized in thefxtypespackage. The change maintains backward compatibility ascrosschaintypescontinues to handle other core functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of crosschaintypes.RegisterExternalAddress # to ensure complete migration rg "crosschaintypes\.RegisterExternalAddress" # Search for any custom implementations that might be affected ast-grep --pattern 'RegisterExternalAddress($_, $_)'Length of output: 102
Script:
#!/bin/bash # Search for imports of both packages echo "=== Package Imports ===" rg "\".*crosschaintypes\"" rg "\".*fxtypes\"" # Search for any RegisterExternalAddress calls echo -e "\n=== RegisterExternalAddress Usage ===" rg "RegisterExternalAddress" # Search for any remaining crosschaintypes references echo -e "\n=== Crosschaintypes References ===" rg "crosschaintypes\." -A 2Length of output: 34612
Script:
#!/bin/bash # Check the fxtypes package implementation echo "=== fxtypes Implementation ===" rg -A 5 "RegisterExternalAddress.*=.*func" rg -A 5 "type.*ExternalAddress.*interface" # Check for any imports of the registration function echo -e "\n=== Registration Imports ===" rg "RegisterExternalAddress" --type go -g "*/types/key.go"Length of output: 812
Script:
#!/bin/bash # Search for key.go files and their registration patterns echo "=== Module Registration Patterns ===" fd "key.go$" | xargs rg "RegisterExternalAddress" -A 2 # Verify the registration function implementation echo -e "\n=== Registration Implementation ===" rg -g "external_address.go" "func RegisterExternalAddress"Length of output: 1335
x/arbitrum/types/key.go (1)
3-6: LGTM! Changes maintain consistency with other modules.The modifications mirror those in the polygon module, maintaining a consistent approach across different chain implementations.
Also applies to: 17-17
x/optimism/types/key.go (1)
3-6: LGTM! Systematic refactoring pattern maintained.The changes follow the same refactoring pattern seen in other chain modules, ensuring consistency across the codebase.
Also applies to: 17-17
tests/integration/crosschain_test.go (1)
4-4: Verify behavior consistency after package migrationThe change moves
GetSupportChains()fromcrosschaintypestotypespackage as part of type consolidation. The implementation looks correct.Let's verify that the function maintains the same behavior after the move:
Also applies to: 13-13
✅ Verification successful
Function successfully migrated to types package
The
GetSupportChains()function has been properly migrated fromcrosschaintypestotypespackage, with all references updated. The implementation remains consistent, using the same logic to return a sorted list of supported chains.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the implementation of GetSupportChains() in both packages # Search for the old implementation rg -A 10 "func GetSupportChains" x/crosschain/types/ # Search for the new implementation rg -A 10 "func GetSupportChains" types/Length of output: 663
Script:
#!/bin/bash # Check if there are any remaining references to GetSupportChains in x/crosschain/types rg "GetSupportChains" x/crosschain/types/ # Check if the externalAddressRouter is properly migrated rg "externalAddressRouter" x/crosschain/types/Length of output: 159
x/eth/types/address.go (1)
8-8: Verify interface implementation completenessThe change updates the type assertion to use
fxtypes.ExternalAddress. The implementation looks correct.Let's verify that
EthereumAddressstill implements all methods of theExternalAddressinterface:Also applies to: 11-11
✅ Verification successful
Interface implementation verified successfully
The
EthereumAddresstype correctly implements all four methods required by thefxtypes.ExternalAddressinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare interface methods with implementation # Search for the interface definition ast-grep --pattern 'type ExternalAddress interface { $$$ }' # Search for the implementation ast-grep --pattern 'type EthereumAddress struct { $$$ } func (b EthereumAddress) $$$'Length of output: 543
Script:
#!/bin/bash # Search for individual method implementations ast-grep --pattern 'func (b EthereumAddress) ValidateExternalAddr(addr string) error { $$$ }' ast-grep --pattern 'func (b EthereumAddress) ExternalAddrToAccAddr(addr string) $_ { $$$ }' ast-grep --pattern 'func (b EthereumAddress) ExternalAddrToHexAddr(addr string) $_ { $$$ }' ast-grep --pattern 'func (b EthereumAddress) ExternalAddrToStr(bz []byte) string { $$$ }'Length of output: 1153
types/external_address_test.go (1)
9-9: LGTM!The import changes align with the package consolidation effort, and the test functionality remains intact.
precompiles/bank/contract_test.go (1)
4-4: Verify impact of enforcing uppercase token namesThe change enforces uppercase token names by converting the name parameter using
strings.ToUpper(). While this ensures consistency, it could affect existing token registrations if they rely on case-sensitive names.Let's verify the impact on existing tokens:
Also applies to: 50-50
✅ Verification successful
✓ Token name case conversion is consistent with existing behavior
The change aligns with the existing codebase behavior where token symbols are already handled in uppercase. This modification only affects test code and maintains consistency with the production implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing token registrations that might be affected # Search for AddERC20Token calls to analyze existing usage rg -A 5 "AddERC20Token.*name.*" # Search for potential test cases that might rely on case-sensitive names rg -A 5 "SetErc20Token.*name.*"Length of output: 1984
x/tron/types/address.go (1)
12-12: LGTM! Interface implementation remains unchanged.The change from
crosschaintypestofxtypesaligns with the broader refactoring effort to consolidate type definitions. The implementation of theExternalAddressinterface remains functionally unchanged.Also applies to: 15-15
precompiles/crosschain/has_oracle_test.go (1)
12-12: LGTM! Test coverage remains intact.The import path change from
x/crosschain/typestotypesaligns with the broader refactoring effort. All test cases continue to validate the same functionality.app/encoding_test.go (1)
47-47: LGTM! Interface count updated for new token operations.The increase in interface implementation count from 293 to 299 correctly reflects the addition of new message types for ERC20 token operations, aligning with the PR objectives.
precompiles/crosschain/has_oracle.go (1)
10-10: LGTM! Function call remains unchanged.The change from
crosschaintypestofxtypesaligns with the broader refactoring effort. TheExternalAddrToStrfunction call remains functionally unchanged.Also applies to: 49-49
precompiles/crosschain/is_oracle_online_test.go (1)
12-12: LGTM! Import path update aligns with type consolidation.The change to use the consolidated
typespackage is consistent with the broader refactoring effort.precompiles/crosschain/is_oracle_online.go (1)
10-10: LGTM! Import and function call updates are consistent.The changes to use
fxtypesinstead ofcrosschaintypesmaintain the same functionality while aligning with the type consolidation effort.Also applies to: 50-50
x/eth/types/signer.go (1)
16-16: LGTM! Error handling standardization improves consistency.The changes standardize error handling by consistently using
crosschaintypes.ErrInvalidwith descriptive error messages.Also applies to: 24-24, 47-47, 62-62
x/erc20/types/codec.go (1)
26-28: LGTM! Verify message handlers implementation.The new message types are properly registered in both
RegisterInterfacesandRegisterLegacyAminoCodec.Let's verify the implementation of message handlers for these new types:
Also applies to: 50-52
✅ Verification successful
Message handlers are properly implemented and follow best practices ✅
All three message handlers are fully implemented with:
- Proper authority checks
- Comprehensive error handling
- Event emission for state changes
- Clean separation between message handling and business logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for message handler implementations ast-grep --pattern 'func (k Keeper) $_(ctx sdk.Context, msg *types.$_) (*types.$_, error) { $$$ }'Length of output: 102
Script:
#!/bin/bash # First locate relevant keeper files echo "=== Searching for keeper files ===" rg -l "RegisterNativeCoin|RegisterNativeERC20|UpdateBridgeToken" x/erc20/keeper/ echo -e "\n=== Searching for message handlers ===" # Search for handler implementations with context rg -A 5 "func \(k .*Keeper\).*\(.*Context.*Msg(RegisterNativeCoin|RegisterNativeERC20|UpdateBridgeToken)" x/erc20/keeper/ echo -e "\n=== Searching with ast-grep for structural pattern ===" ast-grep --pattern 'func (k Keeper) $_($_) $_ { $$$ }' x/erc20/keeper/msg_server.goLength of output: 676
Script:
#!/bin/bash echo "=== Checking msg_server.go content ===" cat x/erc20/keeper/msg_server.go echo -e "\n=== Searching for interface definitions ===" rg "type.*Msg.*Server.*interface" x/erc20/types/ echo -e "\n=== Checking erc20_token.go for related functions ===" cat x/erc20/keeper/erc20_token.goLength of output: 8621
x/crosschain/types/legacy.go (1)
11-11: LGTM! Chain validation logic centralized.The change appropriately moves chain validation to the centralized
fxtypespackage, improving maintainability.Let's verify the supported chains implementation:
Also applies to: 63-65
precompiles/crosschain/contract_test.go (1)
55-55: LGTM! Test setup updated for centralized chain support.The change correctly uses the centralized
fxtypes.GetSupportChains()function.x/crosschain/types/target.go (1)
14-14: LGTM! Address validation logic centralized.The change appropriately moves external address validation to the centralized
fxtypespackage, maintaining error context.Let's verify the address validation implementation:
Also applies to: 113-113
✅ Verification successful
Verification successful: Address validation is properly centralized
The codebase shows a well-structured address validation system with:
- Central validation through
fxtypes.ValidateExternalAddr- Chain-specific implementations properly registered
- Consistent usage across crosschain messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for address validation implementation ast-grep --pattern 'func ValidateExternalAddr(chainName string, addr string) error { $$$ }'Length of output: 95
Script:
#!/bin/bash # Search for address validation related functions rg -i "func.*validate.*addr" --type go # Also search for any address validation related code rg -i "validate.*address" --type goLength of output: 6463
testutil/helpers/key.go (1)
18-18: LGTM! Import path update aligns with centralized types package.The change to use the centralized
typespackage improves code organization and maintainability.x/crosschain/keeper/confirm.go (1)
10-10: LGTM! Ethereum-specific functionality moved to eth package.The change improves code organization by moving Ethereum-specific signature validation to the eth package.
Also applies to: 137-137
x/crosschain/keeper/keeper_v1_test.go (1)
42-42: LGTM! Package organization improvements.The changes improve code organization by:
- Centralizing chain support under fxtypes
- Moving Ethereum-specific functionality to ethtypes
Also applies to: 89-89, 97-97
x/crosschain/client/cli/tx.go (1)
111-111: LGTM! Good refactoring of Ethereum signature creation.Moving the signature creation to the eth-specific types package improves code organization by consolidating Ethereum-related functionality.
Let's verify that the signature creation is consistently using the new package across the codebase:
Also applies to: 181-181
✅ Verification successful
Verification confirms consistent usage of new Ethereum signature package
The signature creation has been consistently moved to the eth/types package across the codebase. Different import aliases (types2, ethtypes) all reference the same implementation at
github.com/pundiai/fx-core/v8/x/eth/types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of the old signature creation method # and verify consistency of the new implementation # Check for old usage rg "types\.NewEthereumSignature" # Check new usage pattern rg "types2\.NewEthereumSignature"Length of output: 1011
Script:
#!/bin/bash # Check import statements in files using NewEthereumSignature echo "=== client/cli/tx.go imports ===" rg "import.*types2" x/crosschain/client/cli/tx.go -A 5 -B 5 echo -e "\n=== keeper test imports ===" rg "import.*ethtypes" x/crosschain/keeper/keeper_v1_test.go x/crosschain/keeper/msg_server_test.go -A 5 -B 5 echo -e "\n=== integration test imports ===" rg "import.*types" tests/integration/crosschain_suite.go -A 5 -B 5Length of output: 468
Script:
#!/bin/bash # Check complete import blocks in all relevant files echo "=== client/cli/tx.go imports ===" rg "^import \(" x/crosschain/client/cli/tx.go -A 20 echo -e "\n=== keeper test imports ===" rg "^import \(" x/crosschain/keeper/keeper_v1_test.go x/crosschain/keeper/msg_server_test.go -A 20 echo -e "\n=== integration test imports ===" rg "^import \(" tests/integration/crosschain_suite.go -A 20Length of output: 4771
x/crosschain/client/cli/query.go (2)
17-17: LGTM! Good addition of centralized types package.The import of
fxtypesaligns with the refactoring goal of centralizing common functionality.
141-141: LGTM! Consistent use of centralized address validation.Moving address validation to
fxtypesimproves maintainability by centralizing this common functionality.Let's verify the consistent usage of the new address validation across the codebase:
Also applies to: 244-244
✅ Verification successful
Address validation centralization verified successfully
All instances of external address validation consistently use the centralized
fxtypes.ValidateExternalAddrimplementation across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of the old address validation # and verify consistency of the new implementation # Check for old usage rg "types\.ValidateExternalAddr" # Check new usage pattern rg "fxtypes\.ValidateExternalAddr"Length of output: 5583
cmd/root.go (1)
202-202: LGTM! Consistent use of centralized chain support queries.Moving chain support queries to
fxtypesimproves maintainability by centralizing this common functionality.Let's verify the consistent usage of the new chain support queries across the codebase:
Also applies to: 229-229
✅ Verification successful
Verification successful: Chain support queries consistently use centralized implementation
The codebase shows complete adoption of
fxtypes.GetSupportChains()across all relevant components including CLI, core logic, tests, and upgrades, with no remnants of the old implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of the old chain support queries # and verify consistency of the new implementation # Check for old usage rg "crosschaintypes\.GetSupportChains" # Check new usage pattern rg "fxtypes\.GetSupportChains"Length of output: 762
x/crosschain/keeper/bridge_call_out.go (1)
92-93: LGTM! Consistent use of centralized address conversion in BuildOutgoingBridgeCall.Moving address conversion to
fxtypesimproves maintainability by centralizing this common functionality.Also applies to: 95-95
app/upgrades/v8/upgrade.go (1)
331-331: LGTM! Verify chain list consistency after package change.The change to use
fxtypes.GetSupportChains()aligns with the broader refactoring to centralize chain support functionality.Run this script to verify that the chain lists are identical between the old and new implementations:
✅ Verification successful
Verified: Chain support implementation is robust and properly centralized
The change to use
fxtypes.GetSupportChains()is verified as a safe improvement that:
- Centralizes chain support in a single package
- Uses a robust registration system with validation
- Maintains consistency through sorted chain lists
- Prevents duplicate chain registrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare chain lists between old and new implementations # Note: This helps ensure that the migration to fxtypes maintains the same supported chains # Search for all GetSupportChains usage echo "=== Chain support implementations ===" rg --type go "func.*GetSupportChains.*\[\]string" # Search for chain list definitions echo -e "\n=== Chain list definitions ===" rg --type go "SupportChains.*=.*\["Length of output: 312
Script:
#!/bin/bash # Broader search for chain implementations and definitions # Find the old implementation in crosschaintypes echo "=== Old implementation (crosschaintypes) ===" fd -e go . | grep -i "crosschain" | xargs cat echo -e "\n=== New implementation (fxtypes) ===" cat types/external_address.go echo -e "\n=== Any other chain-related constants ===" rg --type go -i "chain.*=.*\[" -A 5Length of output: 66122
Script:
#!/bin/bash # Search for GetSupportChains implementations echo "=== GetSupportChains implementations ===" ast-grep --pattern 'func GetSupportChains() []string { $$$ }' echo -e "\n=== Chain-related variables and constants ===" # Look for chain-related constants with smaller context rg --type go -i "var.*chain.*=.*\[" -A 2 rg --type go -i "const.*chain.*=.*\[" -A 2Length of output: 7762
Script:
#!/bin/bash # Find externalAddressRouter definition and usage echo "=== Router definition and initialization ===" rg --type go "externalAddressRouter" -B 2 -A 5 echo -e "\n=== Router package initialization ===" rg --type go "func init\(\)" -B 2 -A 10 types/external_address.goLength of output: 4010
tests/integration/crosschain_suite.go (2)
71-71: LGTM! Address formatting changes align with centralization effort.The changes to use
fxtypes.ExternalAddrToStrfor address formatting are consistent with the refactoring to centralize address handling.Also applies to: 79-79
241-243: LGTM! Signature validation changes maintain security.The switch to use
types.NewEthereumSignatureandtypes.ValidateEthereumSignaturefrom the eth-specific package improves code organization while maintaining security checks.Also applies to: 446-448
x/crosschain/keeper/grpc_query.go (2)
139-141: LGTM! Consistent validation pattern across query handlers.The changes standardize external address validation using
fxtypes.ValidateExternalAddracross all query handlers, maintaining the same error handling pattern.Also applies to: 167-169, 205-207, 241-243
314-314: LGTM! Chain list query updated.The change to use
fxtypes.GetSupportChains()in the chain list query maintains consistency with the rest of the refactoring.x/crosschain/types/msgs.go (1)
Line range hint
79-90: LGTM! Comprehensive validation updates across message types.The changes standardize chain support and address validation using
fxtypespackage functions. The address conversion methods are updated to use the centralized implementations while maintaining the same functionality.Also applies to: 435-445, 475-477
proto/fx/erc20/v1/tx.proto (1)
28-35: LGTM! Well-structured RPC method additions.The new RPC methods follow proper proto3 syntax and are well-integrated into the existing Msg service.
app/interface_registry.json (1)
183-190: LGTM! Proper registration of new message types.The new message types are correctly registered in all required sections (TypeURLMap, Msgs, and ProposalMsgs) following the existing pattern.
Also applies to: 411-414, 503-507
x/crosschain/keeper/keeper_test.go (1)
20-20: LGTM! Consistent use of fxtypes package.The changes align with the broader refactoring to consolidate chain support functionality under the fxtypes package.
Also applies to: 54-54
x/crosschain/types/msgs_test.go (1)
20-20: LGTM! Consistent use of fxtypes package.The changes align with the broader refactoring to consolidate chain support functionality under the fxtypes package.
Also applies to: 1708-1708
Co-authored-by: nulnut <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
proto/fx/erc20/v1/tx.proto (1)
112-112: Use snake_case for field names in proto files.The field name
contractAddressshould follow proto3 naming conventions using snake_case.- string contractAddress = 2; + string contract_address = 2;x/erc20/types/msg.go (1)
86-104: Add empty symbol validation.While the method validates the symbol format using
sdk.ValidateDenom, it should also explicitly check for empty strings like it does for the name field.func (m *MsgRegisterNativeCoin) ValidateBasic() error { if _, err := sdk.AccAddressFromBech32(m.Authority); err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("authority address: %s", err.Error()) } if strings.TrimSpace(m.Name) == "" { return sdkerrors.ErrInvalidRequest.Wrapf("name field cannot be blank") } + if strings.TrimSpace(m.Symbol) == "" { + return sdkerrors.ErrInvalidRequest.Wrapf("symbol field cannot be blank") + } if err := sdk.ValidateDenom(strings.ToLower(m.Symbol)); err != nil { return sdkerrors.ErrInvalidCoins.Wrapf("symbol: %s", err.Error()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/erc20/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/fx/erc20/v1/tx.proto(2 hunks)x/erc20/types/msg.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (5)
proto/fx/erc20/v1/tx.proto (3)
28-35: LGTM! Well-structured RPC method additions.The new RPC methods follow a consistent pattern and are well-integrated into the existing Msg service.
89-103: LGTM! Well-defined message structure.The message definition includes all necessary fields for native coin registration with proper options and field types.
119-134: LGTM! Comprehensive bridge token update message.The message definition includes all necessary fields with proper naming conventions and covers both IBC and external chain scenarios.
x/erc20/types/msg.go (2)
4-14: LGTM! Proper setup of imports and interface implementations.All necessary imports are included and new message types correctly implement the sdk.Msg interface.
Also applies to: 26-28
106-114: LGTM! Thorough validation implementation.The validation method properly checks both the authority and contract addresses using appropriate validation functions.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
crosschaintypestofxtypes.