refactor(crosschain): simplify interface references for the erc20 module #731
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing error handling, restructuring method signatures, and removing redundant functions within 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: 7
🧹 Outside diff range and nitpick comments (9)
x/crosschain/keeper/attestation_handler.go (1)
Line range hint
9-26: Consider consistent error handling in AttestationHandlerWhile the
AttestationHandlerfunction is well-structured, it could benefit from consistent error handling across all cases. Currently, only some cases return errors, while others don't.Consider modifying the function to return errors for all cases, similar to the
ExecuteClaimfunction. This would provide more consistent behavior and potentially improve error tracking. Here's a suggested modification:func (k Keeper) AttestationHandler(ctx sdk.Context, externalClaim types.ExternalClaim) error { switch claim := externalClaim.(type) { case *types.MsgSendToFxClaim, *types.MsgBridgeCallClaim, *types.MsgBridgeCallResultClaim: - k.SavePendingExecuteClaim(ctx, externalClaim) + return k.SavePendingExecuteClaim(ctx, externalClaim) case *types.MsgSendToExternalClaim: - k.OutgoingTxBatchExecuted(ctx, claim.TokenContract, claim.BatchNonce) + return k.OutgoingTxBatchExecuted(ctx, claim.TokenContract, claim.BatchNonce) case *types.MsgBridgeTokenClaim: return k.AddBridgeTokenExecuted(ctx, claim) case *types.MsgOracleSetUpdatedClaim: return k.UpdateOracleSetExecuted(ctx, claim) default: return types.ErrInvalid.Wrapf("event type: %s", claim.GetType()) } - return nil }This change would require updating the
SavePendingExecuteClaimandOutgoingTxBatchExecutedmethods to return an error if they don't already do so.x/crosschain/keeper/bridge_call_out_test.go (1)
Line range hint
1-80: Consider expanding test coverage and maintaining consistency.While the changes improve error handling, there are a couple of suggestions to enhance the overall test suite:
The
TestKeeper_BridgeCallResultHandlerfunction has atestsslice with only one test case. Consider adding more test cases to cover different scenarios, such as failure cases or edge cases.For consistency, you might want to apply similar error handling improvements to other test functions in this suite, like
TestKeeper_DeleteOutgoingBridgeCall.Would you like assistance in generating additional test cases or applying consistent error handling across the test suite?
x/crosschain/keeper/bridge_token.go (2)
Line range hint
105-119: LGTM with a minor suggestion for error handling.The
GetBridgeDenomsfunction is well-implemented and handles various error cases appropriately. It correctly retrieves the metadata for a given denomination and checks for the presence of aliases.Consider using a custom error type or wrapping errors to provide more context. For example:
if !ok { return nil, fmt.Errorf("%w: denom %s not found", types.ErrInvalidDenom, denom) }This approach would allow callers to easily check for specific error types and provide more context about where the error occurred.
120-131: LGTM with suggestions for improvements.The
GetBridgeDenomfunction is implemented correctly and achieves its purpose of finding a bridge denomination that matches a given chain name. However, there are a few areas where it could be improved:
Error handling: Consider wrapping the error returned by
GetBridgeDenomsto provide more context.Matching logic: The current implementation assumes that the
chainNameis always a prefix of the bridge denomination. This might not always be the case, depending on your naming conventions. Consider if a more flexible matching strategy is needed.Multiple matches: The function doesn't handle the case where multiple bridge denominations might start with the same
chainName. Depending on your requirements, you might want to handle this case explicitly.Here's a suggested refactoring that addresses these points:
func (k Keeper) GetBridgeDenom(ctx context.Context, denom, chainName string) (string, error) { bridgeDenoms, err := k.GetBridgeDenoms(ctx, denom) if err != nil { return "", fmt.Errorf("failed to get bridge denoms: %w", err) } var matches []string for _, bridgeDenom := range bridgeDenoms { if strings.Contains(bridgeDenom, chainName) { matches = append(matches, bridgeDenom) } } switch len(matches) { case 0: return "", fmt.Errorf("no bridge denom found for chain %s and denom %s", chainName, denom) case 1: return matches[0], nil default: return "", fmt.Errorf("multiple bridge denoms found for chain %s and denom %s: %v", chainName, denom, matches) } }This refactored version provides more detailed error messages, uses a more flexible matching strategy (
Containsinstead ofHasPrefix), and explicitly handles the case of multiple matches.x/crosschain/keeper/abci.go (1)
185-187: Improved error handling for bridge call refunds.The changes enhance the error handling for the
RefundOutgoingBridgeCalloperation, which aligns with the PR objective of simplifying interface references. The explicit error check and panic with a detailed message improve the debugging process in case of failures.However, consider logging the error before panicking to ensure the error details are captured in the application logs.
Consider adding a log statement before the panic:
if err := k.RefundOutgoingBridgeCall(ctx, data); err != nil { + k.Logger(ctx).Error("failed to refund outgoing bridge call", "nonce", data.Nonce, "error", err) panic(fmt.Sprintf("failed cancel out bridge call %d while trying to execute failed: %s", data.Nonce, err)) }x/crosschain/types/expected_keepers.go (2)
59-59: Add missing documentation forDeleteIBCTransferRelationThe method
DeleteIBCTransferRelationlacks a documentation comment. Adding a comment explaining its purpose and return value will enhance maintainability and readability.
84-85: Add documentation forGetBridgeDenomsandGetBridgeDenomThe new methods lack documentation comments. Providing clear descriptions of their functionality, parameters, and return values will aid other developers in understanding and using these methods correctly.
x/crosschain/keeper/bridge_call_out.go (1)
124-127: Include missing attribute in refund eventThe refund event does not include the
BridgeCallNonce, which might be useful for tracking and logging purposes. Consider adding it to the emitted event.Apply this diff to include the
BridgeCallNonceattribute:ctx.EventManager().EmitEvent(sdk.NewEvent( types.EventTypeBridgeCallRefund, sdk.NewAttribute(types.AttributeKeyRefund, refund.String()), + sdk.NewAttribute(types.AttributeKeyBridgeCallNonce, fmt.Sprint(data.Nonce)), ))x/crosschain/keeper/many_to_one.go (1)
Line range hint
170-186: Inconsistent use of context types in function signatureThe function
BaseDenomToBridgeDenomusescontext.Contextas its context parameter, while other functions have been updated to usesdk.Context. For consistency and to reduce unnecessary unwrapping of the context, consider changing the parameter type tosdk.Context.Apply the following diff to update the function signature and adjust the context usage:
-func (k Keeper) BaseDenomToBridgeDenom(ctx context.Context, baseDenom, target string) (string, error) { +func (k Keeper) BaseDenomToBridgeDenom(ctx sdk.Context, baseDenom, target string) (string, error) { ... - denomTrace, found := k.ibcTransferKeeper.GetDenomTrace(sdk.UnwrapSDKContext(ctx), hash) + denomTrace, found := k.ibcTransferKeeper.GetDenomTrace(ctx, hash)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- x/crosschain/keeper/abci.go (1 hunks)
- x/crosschain/keeper/attestation_handler.go (1 hunks)
- x/crosschain/keeper/bridge_call_in.go (2 hunks)
- x/crosschain/keeper/bridge_call_in_test.go (0 hunks)
- x/crosschain/keeper/bridge_call_out.go (2 hunks)
- x/crosschain/keeper/bridge_call_out_test.go (1 hunks)
- x/crosschain/keeper/bridge_call_refund.go (0 hunks)
- x/crosschain/keeper/bridge_call_refund_test.go (0 hunks)
- x/crosschain/keeper/bridge_token.go (2 hunks)
- x/crosschain/keeper/grpc_query.go (1 hunks)
- x/crosschain/keeper/grpc_query_v1_test.go (0 hunks)
- x/crosschain/keeper/keeper_test.go (0 hunks)
- x/crosschain/keeper/keeper_v1_test.go (0 hunks)
- x/crosschain/keeper/many_to_one.go (2 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (5 hunks)
- x/crosschain/types/expected_keepers.go (2 hunks)
- x/ibc/middleware/types/expected_keepers.go (1 hunks)
💤 Files with no reviewable changes (6)
- x/crosschain/keeper/bridge_call_in_test.go
- x/crosschain/keeper/bridge_call_refund.go
- x/crosschain/keeper/bridge_call_refund_test.go
- x/crosschain/keeper/grpc_query_v1_test.go
- x/crosschain/keeper/keeper_test.go
- x/crosschain/keeper/keeper_v1_test.go
🧰 Additional context used
🔇 Additional comments (19)
x/ibc/middleware/types/expected_keepers.go (1)
18-18: Approve the context type change and verify its impact.The change from
context.Contexttosdk.Contextin theIBCCoinRefundmethod signature is appropriate and aligns with the Cosmos SDK conventions. This change provides access to additional SDK-specific functionality and maintains consistency with other SDK methods.To ensure this change doesn't introduce any issues, please run the following verification script:
This script will help identify any inconsistencies or potential issues related to the context type change. Please review the results and make any necessary adjustments to ensure all implementations and calls are updated correctly.
✅ Verification successful
Context type change verified successfully.
The change from
context.Contexttosdk.Contextin theIBCCoinRefundmethod signature has been verified. No instances ofcontext.Contextusage remain inIBCCoinRefundimplementations, and all calls correctly utilizesdk.Context. Additionally, there are no detected issues related to context cancellation or timeouts within the scope of this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing context.Context to sdk.Context in IBCCoinRefund # Test 1: Check for any remaining uses of context.Context in IBCCoinRefund implementations echo "Checking for context.Context usage in IBCCoinRefund implementations:" rg --type go "func.*IBCCoinRefund.*context\.Context" # Test 2: Verify that all calls to IBCCoinRefund now use sdk.Context echo "Verifying IBCCoinRefund calls use sdk.Context:" rg --type go "IBCCoinRefund\s*\(\s*\w+\s*sdk\.Context" # Test 3: Check for any potential issues with context cancellation or timeouts echo "Checking for potential context cancellation or timeout issues:" rg --type go "WithCancel|WithTimeout|WithDeadline" -C 5Length of output: 97278
x/crosschain/keeper/attestation_handler.go (1)
43-43: Approve change and verify error handlingThe modification to return the result of
k.BridgeCallResultHandler(ctx, claim)is a good improvement. It ensures proper error propagation and makes the handling ofMsgBridgeCallResultClaimconsistent with other claim types in the switch statement.To ensure this change doesn't introduce any unexpected behavior, please verify the error handling in functions that call
ExecuteClaim. Run the following script to find these caller functions:Review the output to ensure that these calling functions properly handle any errors that may now be returned from
ExecuteClaim.✅ Verification successful
Error handling verified
No additional callers to
ExecuteClaimwere found. The modification ensures that errors fromBridgeCallResultHandlerare properly propagated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find functions that call ExecuteClaim and check their error handling # Search for ExecuteClaim function calls ast-grep --lang go --pattern 'k.ExecuteClaim($$$)' | rg -A 10 'ExecuteClaim'Length of output: 76
Script:
#!/bin/bash # Description: Find all invocations of ExecuteClaim in the codebase # Use ast-grep to find all calls to ExecuteClaim with any receiver ast-grep --lang go --pattern '$_.ExecuteClaim($$$)' # Additionally, use ripgrep to search for ExecuteClaim calls rg 'ExecuteClaim\s*\('Length of output: 5054
x/crosschain/keeper/bridge_call_out_test.go (1)
51-52: Improved error handling in the test case.The addition of error checking for the
BridgeCallResultHandlermethod call enhances the robustness of the test. This change ensures that the method executes without errors, which is a good practice in unit testing.x/crosschain/keeper/bridge_token.go (1)
Line range hint
105-131: Summary of changes in bridge_token.goThe additions to this file, namely
GetBridgeDenomsandGetBridgeDenom, contribute to the PR's objective of simplifying interface references for the erc20 module. These functions provide a more flexible and robust way of handling bridge denominations, which aligns with the refactoring goals.The new implementation allows for:
- Retrieval of multiple bridge denominations for a given denom.
- Filtering of bridge denominations based on chain name.
These changes should make it easier to work with bridge tokens across different chains, potentially simplifying cross-chain operations in the erc20 module.
To fully achieve the PR's objectives, ensure that these new functions are utilized consistently throughout the codebase, replacing any older, less flexible methods of handling bridge denominations.
x/crosschain/keeper/abci.go (1)
185-187: Summary: Focused improvement in error handlingThe changes in this file are minimal and well-focused, improving the error handling in the
cleanupTimeOutBridgeCallfunction without altering its core logic. This enhancement aligns with the PR objective of simplifying interface references for the erc20 module by making the error handling more explicit and informative.The localized nature of these changes minimizes the risk of unintended side effects while improving the overall robustness of the code.
x/crosschain/types/expected_keepers.go (2)
59-59:⚠️ Potential issueClarify the necessity of the boolean return value
DeleteIBCTransferRelationreturns abool, but it's unclear what the return value signifies. Ensure that the boolean indicates something meaningful (e.g., success status) and consider documenting it for clarity.
84-85:⚠️ Potential issueEnsure consistent error handling in
GetBridgeDenomsandGetBridgeDenomVerify that both methods handle errors consistently, especially in cases where the provided
denomorchainNamedoes not exist. This will prevent unexpected behavior and make debugging easier.x/crosschain/keeper/bridge_call_out.go (4)
Line range hint
90-111: Improved error handling inBridgeCallResultHandlerThe updates to
BridgeCallResultHandlerenhance its robustness by replacing the panic mechanism with proper error returns. This allows for graceful error handling and better control flow management throughout the application.
113-136: Correct implementation ofRefundOutgoingBridgeCallThe new
RefundOutgoingBridgeCallfunction appropriately handles the refund process, including token conversion and event emission. Error handling is correctly implemented, ensuring that issues are propagated upstream.
138-143: Effective encapsulation inDeleteOutgoingBridgeCallRecordThe new
DeleteOutgoingBridgeCallRecordfunction neatly encapsulates the deletion logic for bridge calls and their confirmations, improving code readability and maintainability.
130-134: Handle potential issues with address conversionWhen converting the refund address to an EVM-compatible address, ensure that the address conversion is valid and consider handling any potential edge cases.
Run the following script to verify the address conversions:
x/crosschain/keeper/many_to_one.go (1)
289-298: Verify all calls toIBCCoinRefundare updated to usesdk.ContextThe
IBCCoinRefundfunction's context parameter type has been changed fromcontext.Contexttosdk.Context. Ensure that all calls to this function across the codebase are updated accordingly to prevent potential type mismatches.Run the following script to identify any calls still using the old context type:
✅ Verification successful
All calls to
IBCCoinRefundare updated to usesdk.Context.
- Verified that all instances of
IBCCoinRefundpasssdk.Contextas the context parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `IBCCoinRefund` that may still use `context.Context`. # Test: Search for usages of `IBCCoinRefund` with `context.Context`. rg --type go 'IBCCoinRefund\s*\(\s*context\.Context' -A 2Length of output: 59
Script:
#!/bin/bash # Description: Find all calls to `IBCCoinRefund` and display the context parameter type. rg --type go 'IBCCoinRefund\s*\(' -A 2Length of output: 847
x/crosschain/mock/expected_keepers_mocks.go (5)
25-25: Import path update aligns with package restructuringThe import path for
types5has been updated togithub.com/functionx/fx-core/v8/x/erc20/types, reflecting the new package organization.
401-404: MethodConvertCoinupdated correctlyThe method
ConvertCoinnow usestypes5.MsgConvertCoinandtypes5.MsgConvertCoinResponse, which aligns with the updated types package.
416-419: MethodConvertERC20updated correctlyThe method
ConvertERC20has been updated to usetypes5.MsgConvertERC20andtypes5.MsgConvertERC20Response, reflecting the changes in the types package.
457-460: Return type forGetTokenPairupdated appropriatelyThe
GetTokenPairmethod now returnstypes5.TokenPair, consistent with the package updates.
745-757: UpdatedGetBridgeDenommethod signatureThe signature of
GetBridgeDenomhas been modified to include an additional parameterchainName string, and this change is correctly reflected in both the method and its mock recorder.x/crosschain/keeper/bridge_call_in.go (2)
40-41: Improved error handling enhances code clarityAssigning the result of
k.BridgeCallEvmdirectly toerrand checking it immediately improves the readability and clarity of the error handling logic. This ensures that any errors from the EVM call are properly captured and handled.
52-69: Consolidated refund logic simplifies the codebaseIntegrating the refund logic directly into the
BridgeCallHandlerfunction eliminates the need for the separateBridgeCallFailedRefundfunction. This consolidation reduces complexity and enhances maintainability by keeping related logic within a single function.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests