refactor: reimplement BridgeTokens gRPC query #748
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
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: 13
🧹 Outside diff range and nitpick comments (7)
x/erc20/keeper/ibc_token.go (1)
Line range hint
1-31: Summary: Parameter reordering as part of larger refactoringThe changes in this file are consistent with the PR objective to refactor and reimplement the BridgeTokens gRPC query. The parameter reordering in both
GetIBCTokenandAddIBCTokenfunctions appears to be part of a broader, consistent change across the codebase.While the changes themselves look good, it's crucial to ensure that these modifications are reflected throughout the entire codebase, particularly in areas that call these functions. This will prevent potential runtime errors due to mismatched function signatures.
Consider adding comprehensive tests that cover these functions with their new signatures to ensure they work as expected in various scenarios. Additionally, if not already done, update any relevant documentation or API references to reflect these changes.
x/erc20/types/msg_test.go (1)
14-14: LGTM: Test restructuring improves clarityThe conversion of
TestMsgConvertCointo a standalone function simplifies the test structure and improves readability. This approach is more straightforward and easier to maintain.Consider adding a brief comment describing the purpose of the
TestMsgConvertCoinfunction to improve documentation.x/crosschain/types/expected_keepers.go (2)
58-58: Approve addition ofGetBridgeTokensmethodThe new
GetBridgeTokensmethod is a valuable addition that allows fetching all bridge tokens for a specific chain. This complements the existingGetBridgeTokenmethod and can support bulk operations or queries.Consider adding a comment to document the purpose and usage of this new method. For example:
// GetBridgeTokens retrieves all bridge tokens associated with the specified chain. // It returns a slice of BridgeToken and an error if the operation fails. GetBridgeTokens(ctx context.Context, chainName string) ([]erc20types.BridgeToken, error)
57-60: Summary of changes to theErc20KeeperinterfaceThe modifications to the
Erc20Keeperinterface enhance its design and functionality:
- Parameter reordering in
GetBridgeTokenandGetIBCTokenimproves readability and consistency.- The new
GetBridgeTokensmethod allows for bulk retrieval of bridge tokens, expanding the interface's capabilities.These changes contribute to a more coherent and feature-rich interface. Ensure that all relevant parts of the codebase are updated to accommodate these changes.
Consider updating the documentation for the entire
Erc20Keeperinterface to reflect these changes and provide clear usage guidelines for developers working with this interface.x/crosschain/mock/expected_keepers_mocks.go (2)
404-417: LGTM. Consider adding documentation for the new method.The new
GetBridgeTokensmethod and its corresponding recorder are correctly implemented and follow the existing patterns in the struct. This addition likely reflects new functionality in the actual keeper.Consider adding a brief comment explaining the purpose of this new method, for example:
// GetBridgeTokens retrieves all bridge tokens for a given chain name. func (m *MockErc20Keeper) GetBridgeTokens(ctx context.Context, chainName string) ([]types4.BridgeToken, error) { // ... existing implementation ... }
Line range hint
1-447: Summary of changes and potential impactThe modifications in this file, particularly in the
MockErc20Keeperstruct, indicate a significant refactoring effort in how bridge and IBC tokens are handled:
- Parameter order changes in
GetBridgeTokenandGetIBCTokenmethods.- Addition of a new
GetBridgeTokensmethod.These changes suggest an evolution in the token handling logic of the system. While the changes are well-implemented and consistent within this file, they may have broader implications:
- Calls to
GetBridgeTokenandGetIBCTokenthroughout the codebase will need to be updated.- The new
GetBridgeTokensmethod implies new functionality that might require additional testing and documentation.Ensure that these changes are reflected in the main keeper implementation, tests, and any relevant documentation. Also, verify that all dependent code has been updated to match these new method signatures.
proto/fx/gravity/crosschain/v1/legacy.proto (1)
128-171: Provide documentation for deprecated messagesSince multiple messages are added and immediately deprecated, it's important to document the reasoning behind this approach. This will help other developers understand the context and prevent misuse.
Consider adding comments explaining why these messages are added as deprecated:
message BridgeToken { option deprecated = true; + // Explanation for deprecation string token = 1; // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/crosschain/types/genesis.pb.gois excluded by!**/*.pb.gox/crosschain/types/legacy.pb.gois excluded by!**/*.pb.gox/crosschain/types/query.pb.gois excluded by!**/*.pb.gox/crosschain/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
- api/fx/gravity/crosschain/v1/genesis.pulsar.go (39 hunks)
- app/genesis.json (0 hunks)
- docs/swagger-ui/swagger.yaml (0 hunks)
- proto/fx/gravity/crosschain/v1/genesis.proto (1 hunks)
- proto/fx/gravity/crosschain/v1/legacy.proto (1 hunks)
- proto/fx/gravity/crosschain/v1/query.proto (1 hunks)
- proto/fx/gravity/crosschain/v1/types.proto (0 hunks)
- x/crosschain/keeper/bridge_token.go (2 hunks)
- x/crosschain/keeper/grpc_query.go (3 hunks)
- x/crosschain/keeper/many_to_one.go (3 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (2 hunks)
- x/crosschain/types/expected_keepers.go (1 hunks)
- x/crosschain/types/types.go (0 hunks)
- x/crosschain/types/types_test.go (0 hunks)
- x/erc20/keeper/bridge_token.go (1 hunks)
- x/erc20/keeper/ibc_token.go (1 hunks)
- x/erc20/types/events.go (0 hunks)
- x/erc20/types/keys_test.go (0 hunks)
- x/erc20/types/msg_test.go (2 hunks)
💤 Files with no reviewable changes (7)
- app/genesis.json
- docs/swagger-ui/swagger.yaml
- proto/fx/gravity/crosschain/v1/types.proto
- x/crosschain/types/types.go
- x/crosschain/types/types_test.go
- x/erc20/types/events.go
- x/erc20/types/keys_test.go
🧰 Additional context used
🔇 Additional comments (24)
x/erc20/keeper/ibc_token.go (2)
Line range hint
15-31: LGTM! Verify usage across the codebase.The reordering of parameters in the
AddIBCTokenfunction looks good. The internal logic remains correct with the new parameter order, including error handling and the creation of the IBCToken struct. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.To verify the consistent usage of the new function signature, run the following script:
#!/bin/bash # Description: Verify all function calls to `AddIBCToken` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'AddIBCToken\s*\(\s*ctx\s*,\s*channel\s*,\s*baseDenom\s*,\s*ibcDenom\s*\)' # Test: Search for potential missed updates. Expect: No results. rg --type go -A 5 $'AddIBCToken\s*\(\s*ctx\s*,\s*baseDenom\s*,\s*channel\s*,\s*ibcDenom\s*\)'
11-13: LGTM! Verify usage across the codebase.The reordering of parameters in the
GetIBCTokenfunction looks good. The internal logic remains correct with the new parameter order. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.To verify the consistent usage of the new function signature, run the following script:
proto/fx/gravity/crosschain/v1/genesis.proto (1)
18-28: LGTM: Consistent field structure and optionsThe new structure of the
GenesisStatemessage maintains consistency in field numbering, types, and options. Thegogoproto.nullable = falseoption is appropriately applied where needed, ensuring that all fields are required.x/erc20/keeper/bridge_token.go (1)
15-17: LGTM: New GetBaseDenom function looks good.The new
GetBaseDenomfunction is a straightforward and correct implementation for retrieving the base denomination of a given token using theDenomIndex.x/erc20/types/msg_test.go (3)
8-8: LGTM: Import changes align with test restructuringThe removal of the
suiteimport and addition of therequirepackage are consistent with the shift from a suite-based testing approach to standalone tests. This change simplifies the test structure and directly utilizes therequireassertions for error checking.
84-86: LGTM: Assertion changes maintain test effectivenessThe switch to
require.NoErrorandrequire.Erroris consistent with the new standalone test structure. The assertion logic and error messages have been preserved, maintaining the test's effectiveness and debuggability.
Line range hint
1-89: Overall: Successful refactoring of test structureThe changes in this file represent a successful refactoring of the test structure from a suite-based approach to standalone tests. This refactoring:
- Simplifies the overall test structure
- Maintains the existing test cases and their logic
- Improves readability and maintainability
The modifications align well with modern testing practices in Go and should make the codebase easier to work with for developers.
x/crosschain/keeper/bridge_token.go (1)
52-52: LGTM. Verify consistent application of parameter reordering for both methods.The changes in the order of parameters for
k.erc20Keeper.GetIBCTokenandk.erc20Keeper.GetBridgeTokenlook good and align with the reported modifications. However, it's crucial to ensure these changes have been consistently applied across all calls to both methods in the codebase to prevent potential runtime errors.To verify the consistent application of these changes, run the following script:
#!/bin/bash # Description: Check for any remaining calls to GetIBCToken and GetBridgeToken with the old parameter order # Test 1: Search for GetIBCToken calls. Expect: All calls should have IBCChannel as the first parameter. echo "Checking GetIBCToken calls:" rg --type go 'GetIBCToken\s*\([^,]+,\s*[^,]+\)' -g '!vendor/' # Test 2: Search for GetBridgeToken calls. Expect: All calls should have moduleName as the second parameter. echo "Checking GetBridgeToken calls:" rg --type go 'GetBridgeToken\s*\([^,]+,\s*[^,]+,\s*[^,]+\)' -g '!vendor/'If the script returns any results, those might be instances where the parameter reordering hasn't been applied and should be addressed.
Also applies to: 58-58
x/crosschain/types/expected_keepers.go (1)
60-60: Approve parameter reordering forGetIBCTokenThe reordering of parameters from
(ctx context.Context, baseDenom, channel string)to(ctx context.Context, channel, baseDenom string)improves consistency with theGetBridgeTokenmethod. This change enhances the overall coherence of the interface.Please ensure all calls to this method throughout the codebase are updated to reflect this change. Run the following script to identify potential places that need updating:
#!/bin/bash # Search for calls to GetIBCToken that might need updating rg --type go 'GetIBCToken\s*\([^)]*\)' -g '!x/crosschain/types/expected_keepers.go'x/crosschain/keeper/many_to_one.go (3)
Line range hint
1-174: Summary: Parameter reordering refactoringThe changes in this file are part of a larger refactoring effort to standardize the order of parameters in function calls. Specifically:
GetBridgeTokencalls now havemoduleNameas the first parameter.GetIBCTokencalls now havechannelas the first parameter.These changes improve consistency across the codebase without altering the functionality. However, it's crucial to ensure that all related parts of the codebase have been updated accordingly to prevent any potential issues.
To ensure the refactoring is complete across the entire codebase, please run the following script:
#!/bin/bash # Description: Verify the completion of the refactoring across the codebase # Test 1: Check for any remaining GetBridgeToken calls with the old parameter order echo "Checking for old GetBridgeToken calls:" rg --type go 'GetBridgeToken\s*\([^,]+,\s*[^,]+\)' # Test 2: Check for any remaining GetIBCToken calls with the old parameter order echo "Checking for old GetIBCToken calls:" rg --type go 'GetIBCToken\s*\([^,]+,\s*[^,]+\)' # Test 3: Verify the updated function signatures echo "Verifying updated function signatures:" rg --type go -A 5 'func \(k Keeper\) GetBridgeToken' rg --type go -A 5 'func \(k Keeper\) GetIBCToken'This script will help identify any inconsistencies that may have been missed during the refactoring process.
109-109: LGTM. Verify consistency across the codebase.The change in parameter order for the
GetIBCTokencall looks good. This is consistent with the previous change and appears to be part of the same refactoring effort.To ensure consistency, please run the following script to check for any remaining instances of the old parameter order:
✅ Verification successful
Verified. No remaining instances of the old
GetIBCTokenparameter order found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of the old GetIBCToken parameter order # Test: Search for GetIBCToken calls with the old parameter order rg --type go 'GetIBCToken\s*\([^,]+,\s*[^,]+\)'Length of output: 2587
171-171: LGTM. Verify erc20Keeper.GetBridgeToken signature.The change in parameter order for the
GetBridgeTokencall is consistent with the previous changes and completes the refactoring in this file.To ensure the refactoring is complete, please verify the signature of the
erc20Keeper.GetBridgeTokenfunction:✅ Verification successful
Verified: The
erc20Keeper.GetBridgeTokenfunction signature has been correctly updated, and the parameter order in theGetBridgeTokencall is consistent with the updated definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the signature of erc20Keeper.GetBridgeToken # Test: Search for the GetBridgeToken function definition in the erc20 keeper rg --type go -A 5 'func \(k Keeper\) GetBridgeToken'Length of output: 2587
proto/fx/gravity/crosschain/v1/query.proto (1)
9-9: Approve the addition of the legacy import with a request for clarification.The addition of the import
"fx/gravity/crosschain/v1/legacy.proto"is noted. This suggests the introduction of legacy functionality or definitions, which could be part of a refactoring process to separate legacy code.Could you please clarify:
- The purpose of including this legacy import?
- Where and how the imported legacy types or messages are being used?
- If this is preparation for future changes, what are those changes?
Consider adding a comment above this import to document its purpose and usage, which will help future maintainers understand its significance.
x/crosschain/mock/expected_keepers_mocks.go (2)
436-447: LGTM. Verify usage ofGetIBCTokenin the codebase.The parameter order change in
GetIBCTokenfrom(ctx, baseDenom, channel)to(ctx, channel, baseDenom)is consistent and correctly implemented. This change aligns with the similar modification inGetBridgeToken, suggesting a broader refactoring effort.To ensure all calls to
GetIBCTokenhave been updated, run the following script:#!/bin/bash # Search for calls to GetIBCToken and verify the parameter order rg --type go 'GetIBCToken\s*\([^)]*\)' -g '!*_test.go' -g '!mock*.go'
391-403: LGTM. Verify usage ofGetBridgeTokenin the codebase.The parameter order change in
GetBridgeTokenfrom(ctx, baseDenom, chainName)to(ctx, chainName, baseDenom)is consistent and correctly implemented. However, this change may impact other parts of the codebase that call this method.To ensure all calls to
GetBridgeTokenhave been updated, run the following script:✅ Verification successful
LGTM. All calls to
GetBridgeTokenhave been updated to reflect the new parameter order.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to GetBridgeToken and verify the parameter order rg --type go 'GetBridgeToken\s*\([^)]*\)' -g '!*_test.go' -g '!mock*.go'Length of output: 947
Script:
#!/bin/bash # Search for calls to GetBridgeToken and verify the parameter order rg --type go 'GetBridgeToken\s*\([^)]*\)' -g '!*_test.go' -g '!mock*.go'Length of output: 1572
x/crosschain/keeper/grpc_query.go (3)
195-195: Adjusted parameter order inGetBridgeTokencall aligns with updated function signature.The call to
GetBridgeTokennow correctly passesreq.ChainNamefollowed byreq.Denom, matching the updated function definition.
210-210: Updated parameter order inGetBridgeTokencall is appropriate.In the
TokenToDenommethod, the parameters in theGetBridgeTokencall have been reordered toreq.ChainNameandbaseDenom, reflecting the updated function signature.
285-296: ImplementedBridgeTokensmethod enhances functionality and provides accurate responses.The
BridgeTokensmethod now effectively retrieves bridge tokens for a givenChainNameand constructs the response. The implementation is clear, and the loop correctly maps tokens to the expected response format.api/fx/gravity/crosschain/v1/genesis.pulsar.go (6)
Line range hint
121-161: Consistent Type Updates in List StructuresThe updates to list structures
_GenesisState_6_listthrough_GenesisState_10_listcorrectly reflect the new types:
_GenesisState_6_listuses*OutgoingTransferTx._GenesisState_7_listuses*OutgoingTxBatch._GenesisState_8_listuses*MsgOracleSetConfirm._GenesisState_9_listuses*MsgConfirmBatch._GenesisState_10_listuses*Attestation.These changes ensure that the list handling functions operate with the appropriate types.
Also applies to: 172-212, 223-263, 274-314, 325-369
506-530: Correct Handling of List Fields in GenesisState MethodsThe methods for getting, setting, and mutating list fields in the
GenesisStatestruct have been updated to accommodate the new types. The changes toGet,Set,Mutable, andNewFieldmethods ensure proper manipulation of:
UnbatchedTransfersBatchesOracleSetConfirmsBatchConfirmsAttestationsThese updates maintain the integrity of data operations within the
GenesisStatestruct.Also applies to: 685-711, 761-777, 833-857, 904-916
Line range hint
1095-1191: Updated Serialization Logic for New TypesThe serialization functions, including marshaling methods, have been updated to handle the new types within the
GenesisStatestruct. This ensures that data is correctly serialized and deserialized, maintaining compatibility with the Protobuf definitions.
Line range hint
1517-1744: Accurate Unmarshaling of Updated FieldsThe unmarshaling logic has been carefully adjusted to correctly parse the new types within
GenesisState. The changes ensure reliable deserialization of incoming data into the updated structure, preventing potential runtime errors.
1822-1830: Revised GenesisState Struct DefinitionThe
GenesisStatestruct has been updated:
- Removed the
BridgeTokensfield.- Updated types for
UnbatchedTransfers,Batches,OracleSetConfirms,BatchConfirms, andAttestations.These modifications align the struct with the new data model, reflecting the restructuring efforts described in the PR objectives.
Line range hint
1964-2051: Consistency in Protobuf DefinitionsThe changes in the Protobuf descriptors and message types correspond to the updates made in the
GenesisStatestruct. This ensures that the Go code remains consistent with the Protobuf definitions, maintaining protocol compatibility.
| func (k Keeper) GetBridgeTokens(ctx context.Context, chainName string) ([]types.BridgeToken, error) { | ||
| rng := collections.NewPrefixedPairRange[string, string](chainName) | ||
| iter, err := k.BridgeToken.Iterate(ctx, rng) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| kvs, err := iter.KeyValues() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tokens := make([]types.BridgeToken, 0, len(kvs)) | ||
| for _, kv := range kvs { | ||
| tokens = append(tokens, kv.Value) | ||
| } | ||
|
|
||
| return tokens, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Approve new GetBridgeTokens function with a suggestion for improved error handling.
The new GetBridgeTokens function is well-implemented and efficiently retrieves all bridge tokens for a given chain name. It effectively uses the collections package to iterate over the relevant subset of bridge tokens.
However, there's room for improvement in error handling:
Consider returning an error immediately if one is encountered during iteration. This can be achieved by modifying the function as follows:
func (k Keeper) GetBridgeTokens(ctx context.Context, chainName string) ([]types.BridgeToken, error) {
rng := collections.NewPrefixedPairRange[string, string](chainName)
iter, err := k.BridgeToken.Iterate(ctx, rng)
if err != nil {
return nil, err
}
- kvs, err := iter.KeyValues()
- if err != nil {
- return nil, err
- }
-
- tokens := make([]types.BridgeToken, 0, len(kvs))
- for _, kv := range kvs {
- tokens = append(tokens, kv.Value)
+ var tokens []types.BridgeToken
+ for iter.Next() {
+ kv, err := iter.KeyValue()
+ if err != nil {
+ return nil, err
+ }
+ tokens = append(tokens, kv.Value)
}
return tokens, nil
}This modification ensures that any error during iteration is immediately returned, preventing potential issues with partial or incorrect data.
📝 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.
| func (k Keeper) GetBridgeTokens(ctx context.Context, chainName string) ([]types.BridgeToken, error) { | |
| rng := collections.NewPrefixedPairRange[string, string](chainName) | |
| iter, err := k.BridgeToken.Iterate(ctx, rng) | |
| if err != nil { | |
| return nil, err | |
| } | |
| kvs, err := iter.KeyValues() | |
| if err != nil { | |
| return nil, err | |
| } | |
| tokens := make([]types.BridgeToken, 0, len(kvs)) | |
| for _, kv := range kvs { | |
| tokens = append(tokens, kv.Value) | |
| } | |
| return tokens, nil | |
| func (k Keeper) GetBridgeTokens(ctx context.Context, chainName string) ([]types.BridgeToken, error) { | |
| rng := collections.NewPrefixedPairRange[string, string](chainName) | |
| iter, err := k.BridgeToken.Iterate(ctx, rng) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var tokens []types.BridgeToken | |
| for iter.Next() { | |
| kv, err := iter.KeyValue() | |
| if err != nil { | |
| return nil, err | |
| } | |
| tokens = append(tokens, kv.Value) | |
| } | |
| return tokens, nil | |
| } |
| func (k Keeper) GetBridgeToken(ctx context.Context, chainName, baseDenom string) (types.BridgeToken, error) { | ||
| return k.BridgeToken.Get(ctx, collections.Join(baseDenom, chainName)) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of GetBridgeToken detected.
While most calls to GetBridgeToken have been updated to the new parameter order (ctx, chainName, baseDenom), some instances still use an older or different signature. Please verify and update all function calls to maintain consistency and prevent potential bugs.
🔗 Analysis chain
Verify impact of parameter order change in GetBridgeToken.
The updated GetBridgeToken function looks good. The implementation is correct and consistent with the new parameter order (chainName, baseDenom).
Please ensure that all existing calls to this function have been updated to match the new parameter order. Run the following script to verify the function usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `GetBridgeToken` match the new parameter order.
# Test: Search for the function usage. Expect: Only occurrences of the new parameter order.
rg --type go -A 5 $'GetBridgeToken\s*\('
Length of output: 5369
| @@ -28,7 +28,7 @@ func (k Keeper) AddBridgeTokenExecuted(ctx sdk.Context, claim *types.MsgBridgeTo | |||
| return types.ErrInvalid.Wrapf("%s denom decimals not match %d, expect %d", | |||
| fxtypes.DefaultDenom, claim.Decimals, fxtypes.DenomUnit) | |||
| } | |||
| bridgeToken, err := k.erc20Keeper.GetBridgeToken(ctx, claim.Symbol, k.moduleName) | |||
| bridgeToken, err := k.erc20Keeper.GetBridgeToken(ctx, k.moduleName, claim.Symbol) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent GetBridgeToken Parameter Order Detected
The verification script identified multiple calls to GetBridgeToken with varying second parameters. While some instances correctly use moduleName, others use chainName, which may lead to inconsistencies and potential runtime errors. Please ensure that all calls pass moduleName as the second parameter to maintain consistency across the codebase.
🔗 Analysis chain
LGTM. Verify consistent application of parameter reordering.
The change in the order of parameters for k.erc20Keeper.GetBridgeToken looks good and aligns with the reported modifications. However, it's crucial to ensure this change has been consistently applied across all calls to GetBridgeToken in the codebase to prevent potential runtime errors.
To verify the consistent application of this change, run the following script:
If the script returns any results, those might be instances where the parameter reordering hasn't been applied and should be addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining calls to GetBridgeToken with the old parameter order
# Test: Search for GetBridgeToken calls. Expect: All calls should have moduleName as the second parameter.
rg --type go 'GetBridgeToken\s*\([^,]+,\s*[^,]+,\s*[^,]+\)' -g '!vendor/'
Length of output: 1685
| @@ -54,9 +54,10 @@ type Erc20Keeper interface { | |||
|
|
|||
| GetERC20Token(ctx context.Context, baseDenom string) (erc20types.ERC20Token, error) | |||
|
|
|||
| GetBridgeToken(ctx context.Context, baseDenom, chainName string) (erc20types.BridgeToken, error) | |||
| GetBridgeToken(ctx context.Context, chainName, baseDenom string) (erc20types.BridgeToken, error) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent GetBridgeToken Method Signatures Detected
Multiple instances of GetBridgeToken have been found with differing parameter counts:
-
Two Parameters:
x/crosschain/keeper/many_to_one.go: GetBridgeToken(ctx, tokenAddr)x/crosschain/keeper/many_to_one.go: func (k Keeper) GetBridgeToken(ctx context.Context, tokenAddr string) (erc20types.BridgeToken, error)
-
Three Parameters (Updated Order):
- All other occurrences align with the updated signature
(ctx context.Context, chainName, baseDenom string)
- All other occurrences align with the updated signature
This inconsistency may lead to potential bugs or runtime errors.
🔗 Analysis chain
Approve parameter reordering for GetBridgeToken
The reordering of parameters from (ctx context.Context, baseDenom, chainName string) to (ctx context.Context, chainName, baseDenom string) improves readability and consistency. It follows a logical order from broader (chainName) to more specific (baseDenom).
Please ensure all calls to this method throughout the codebase are updated to reflect this change. Run the following script to identify potential places that need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to GetBridgeToken that might need updating
rg --type go 'GetBridgeToken\s*\([^)]*\)' -g '!x/crosschain/types/expected_keepers.go'
Length of output: 832
Script:
#!/bin/bash
# Search for calls to GetBridgeToken that might need updating
rg --type go 'GetBridgeToken\s*\([^)]*\)' -g '!x/crosschain/types/expected_keepers.go'
Length of output: 1449
Script:
#!/bin/bash
# Search for calls to GetBridgeToken that might need updating
rg --type go 'GetBridgeToken\s*\([^)]*\)' -g '!x/crosschain/types/expected_keepers.go'
Length of output: 1449
| option deprecated = true; | ||
| string token_contract = 1; | ||
| string base_fee = 2 [ | ||
| (cosmos_proto.scalar) = "cosmos.Int", | ||
| (gogoproto.customtype) = "cosmossdk.io/math.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Clarify the necessity of deprecated MinBatchFee message
The MinBatchFee message is also added and marked as deprecated immediately. Please verify whether this message is required. If it is included for backward compatibility or future use, provide appropriate documentation.
| option deprecated = true; | ||
| string chain_name = 1; | ||
| repeated MinBatchFee min_batch_fees = 2 [ (gogoproto.nullable) = false ]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Check the use of nullable option on repeated fields
In the QueryBatchFeeRequest message, the field min_batch_fees is defined as:
repeated MinBatchFee min_batch_fees = 2 [ (gogoproto.nullable) = false ];Setting (gogoproto.nullable) = false on a repeated field may be unnecessary, as repeated fields are lists and are not nullable by default in Protobuf. Consider removing this option to avoid potential issues with code generation and serialization.
| option deprecated = true; | ||
| repeated BatchFees batch_fees = 1; | ||
| } |
There was a problem hiding this comment.
Assess the immediate deprecation of QueryBatchFeeResponse
The QueryBatchFeeResponse message is added and marked as deprecated. If this message is no longer needed, consider excluding it entirely. If it must be included, ensure that its deprecation status and purpose are clearly documented to aid developers.
| message MsgBridgeCallConfirmResponse { option deprecated = true; } |
There was a problem hiding this comment.
💡 Codebase verification
Deprecated MsgBridgeCallConfirmResponse is Still Referenced
- Found reference in
proto/fx/gravity/crosschain/v1/tx.proto
🔗 Analysis chain
Verify usage of deprecated MsgBridgeCallConfirmResponse
The MsgBridgeCallConfirmResponse message is marked as deprecated. Please confirm that all references to this message in the codebase have been properly updated or handled to prevent unintended usage.
Run the following script to identify any remaining usages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to MsgBridgeCallConfirmResponse in .proto files
rg --type proto 'MsgBridgeCallConfirmResponse'
Length of output: 76
Script:
#!/bin/bash
# Description: Find all references to MsgBridgeCallConfirmResponse in .proto files
rg 'MsgBridgeCallConfirmResponse' --glob '*.proto'
Length of output: 247
| tokens, err := k.erc20Keeper.GetBridgeTokens(c, req.ChainName) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine error handling for GetBridgeTokens to provide more specific gRPC error codes.
Currently, any error from k.erc20Keeper.GetBridgeTokens(c, req.ChainName) results in an Internal error code. Consider returning more specific error codes based on the nature of the error (e.g., codes.NotFound if the chain name is not found, or codes.InvalidArgument if the chain name is invalid) to offer clearer feedback to clients.
Apply this diff to enhance error handling:
tokens, err := k.erc20Keeper.GetBridgeTokens(c, req.ChainName)
if err != nil {
- return nil, status.Error(codes.Internal, err.Error())
+ if errors.Is(err, types.ErrChainNotFound) {
+ return nil, status.Error(codes.NotFound, err.Error())
+ } else if errors.Is(err, types.ErrInvalidChainName) {
+ return nil, status.Error(codes.InvalidArgument, err.Error())
+ } else {
+ return nil, status.Error(codes.Internal, err.Error())
+ }
}Ensure that types.ErrChainNotFound and types.ErrInvalidChainName are the actual error variables used in your codebase to represent these error conditions.
📝 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.
| tokens, err := k.erc20Keeper.GetBridgeTokens(c, req.ChainName) | |
| if err != nil { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| } | |
| tokens, err := k.erc20Keeper.GetBridgeTokens(c, req.ChainName) | |
| if err != nil { | |
| if errors.Is(err, types.ErrChainNotFound) { | |
| return nil, status.Error(codes.NotFound, err.Error()) | |
| } else if errors.Is(err, types.ErrInvalidChainName) { | |
| return nil, status.Error(codes.InvalidArgument, err.Error()) | |
| } else { | |
| return nil, status.Error(codes.Internal, err.Error()) | |
| } | |
| } |
Summary by CodeRabbit
New Features
bridge_tokensfield from multiple blockchain configurations.Bug Fixes
GenesisState.Documentation
BridgeTokenreferences.Chores