refactor(bridgecall): token conversion logic #717
Conversation
WalkthroughThe pull request introduces significant modifications across several files in the codebase, primarily focusing on enhancing the handling of token pairs and bridge calls. Key changes include updates to method signatures, the introduction of new methods for better error handling, and restructuring of test cases for improved clarity. 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: 6
🧹 Outside diff range and nitpick comments (4)
x/crosschain/keeper/bridge_call_in_test.go (3)
25-52: Explicitly setCallContractin test cases for clarityIn the test cases, the
CallContractfield is not set, so it defaults tofalse. To improve clarity and avoid potential confusion in the future, consider explicitly settingCallContractin each test case.
19-19: Ensure consistent receiver names and types across test functionsThe test functions use different receiver names and types:
(suite *KeeperTestSuite)inTestBridgeCallHandlerandBridgeCallClaimInitialize(s *KeeperMockSuite)inTest_CoinsToBridgeCallTokensandMockBridgeCallTokenFor consistency and better readability, consider using the same receiver name and struct type across all test functions, unless there is a specific reason for the difference.
Also applies to: 95-129
74-94: Consider renamingBridgeCallClaimInitializetoInitializeBridgeCallClaimfor better readabilityRenaming the function to
InitializeBridgeCallClaimaligns with Go naming conventions by starting with an action verb, improving code readability.Apply this diff to rename the function:
-func (suite *KeeperTestSuite) BridgeCallClaimInitialize(msg types.MsgBridgeCallClaim, tokenIsNativeCoin []bool) (baseDenoms, bridgeDenoms []string, erc20Addrs []common.Address) { +func (suite *KeeperTestSuite) InitializeBridgeCallClaim(msg types.MsgBridgeCallClaim, tokenIsNativeCoin []bool) (baseDenoms, bridgeDenoms []string, erc20Addrs []common.Address) {And update the function call accordingly:
-_, _, erc20Addrs := suite.BridgeCallClaimInitialize(tc.Msg, tc.TokenIsNativeCoin) +_, _, erc20Addrs := suite.InitializeBridgeCallClaim(tc.Msg, tc.TokenIsNativeCoin)x/crosschain/keeper/bridge_call_in.go (1)
81-81: Address the TODO comment regarding coin conversionThere's a TODO comment:
// todo replace convert coin, return token pair. This indicates that the current implementation is a placeholder. Please consider completing this task to improve the code's functionality and maintainability.Would you like assistance in implementing the replacement of
ConvertCoinwith a method that returns the token pair? I can help draft the code or open a new GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- testutil/helpers/suite.go (3 hunks)
- x/crosschain/keeper/bridge_call_in.go (3 hunks)
- x/crosschain/keeper/bridge_call_in_test.go (2 hunks)
- x/crosschain/keeper/keeper.go (0 hunks)
💤 Files with no reviewable changes (1)
- x/crosschain/keeper/keeper.go
🧰 Additional context used
🔇 Additional comments (1)
testutil/helpers/suite.go (1)
216-216: Confirm use ofstrings.ToUpper(denom)In the function call to
DeployUpgradableToken,denomis converted to uppercase usingstrings.ToUpper(denom). Ensure this is intentional, as changing the case ofdenommay lead to inconsistencies if the original case is significant elsewhere in the codebase or in external systems.Run the following script to verify how
denomis used elsewhere and whether case sensitivity matters:#!/bin/bash # Description: Check usage of `denom` and its case sensitivity in the codebase. # Find all occurrences of `denom` variable. rg --type go '\bdenom\b' # Check if `denom` is used in a case-sensitive context.
Co-authored-by: nulnut <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
x/crosschain/keeper/bridge_call_in_test.go (2)
55-70: Improved test execution and error handlingThe changes to the test execution loop are well-structured and improve the overall quality of the tests:
- Using
suite.Runfor each test case enhances isolation and reporting.- The introduction of
BridgeCallClaimInitializeimproves modularity in test setup.- The simplified error checking makes the tests more concise and readable.
Consider adding more descriptive error messages to the
suite.Require().Error(err)check. This can help in quickly identifying which specific assertion failed when a test case doesn't succeed. For example:suite.Require().Error(err, "Expected an error for test case: %s", tc.Name)
Line range hint
98-150: Consider refactoring remaining mock-based testsThe
Test_CoinsToBridgeCallTokensandMockBridgeCallTokenmethods are still part of theKeeperMockSuite, while the refactored tests useKeeperTestSuite. To maintain consistency across the test file:
- Consider refactoring these methods to use the
KeeperTestSuiteapproach.- If mock-based testing is still necessary for these specific cases, consider moving them to a separate file dedicated to mock-based tests.
This refactoring would improve the overall consistency of the test suite and make it easier to maintain in the future.
x/crosschain/keeper/bridge_call_in.go (5)
28-32: Approved: Flexible receiver token address determinationThe new logic for determining the receiver token address based on the memo content adds flexibility to the system. This change aligns well with the intended functionality.
Consider adding a brief comment explaining the significance of
isMemoSendCallTofor better code readability:// If memo indicates a send call, use sender address as receiver token address isMemoSendCallTo := types.IsMemoSendCallTo(msg.MustMemo())
34-41: Approved: Improved token handling and error managementThe token creation and conversion process has been implemented correctly. The error handling has been enhanced, which is a good practice.
Consider wrapping the error returned from
BridgeTokenToERC20with additional context:if err != nil { return fmt.Errorf("failed to bridge token to ERC20: %w", err) }This will provide more informative error messages for debugging.
Line range hint
44-61: Approved: Enhanced bridge call execution and error handlingThe use of a cache context for the bridge call execution is a good practice for ensuring atomic operations. The improved error handling and telemetry enhancements will greatly benefit system observability.
Consider adding a log statement before the telemetry call to provide more context in the logs:
if !ctx.IsCheckTx() { k.Logger(ctx).Info("Bridge call execution result", "success", err == nil, "module", k.moduleName) telemetry.IncrCounterWithLabels( // ... (existing code) ) }This will provide more detailed logging information alongside the telemetry data.
96-124: Approved: Improved BridgeCallEvm function with enhanced flexibilityThe updated
BridgeCallEvmfunction now effectively handles both memo send calls and regular bridge calls. The addition of the contract check for the target address enhances security.Consider adding a comment to explain the difference between memo send calls and regular bridge calls for better code readability:
// Handle memo send calls differently from regular bridge calls if isMemoSendCallTo { // Memo send call logic args = data callEvmSender = sender } else { // Regular bridge call logic var err error args, err = types.PackBridgeCallback(sender, refundAddr, tokens, amounts, data, memo) if err != nil { return err } callEvmSender = k.GetCallbackFrom() }This will help future developers understand the purpose of this conditional logic more easily.
126-136: Approved: New BridgeCallFailedRefund function for handling failed bridge callsThe addition of the
BridgeCallFailedRefundfunction provides a crucial mechanism for handling failed bridge calls, improving user experience and system reliability. The event emission enhances the observability of refund operations.Consider wrapping the error returned from
AddOutgoingBridgeCallwith additional context:outCallNonce, err := k.AddOutgoingBridgeCall(ctx, refundAddr, refundAddr, erc20Token, common.Address{}, nil, nil, eventNonce) if err != nil { return fmt.Errorf("failed to add outgoing bridge call for refund: %w", err) }This will provide more informative error messages for debugging purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- testutil/helpers/suite.go (3 hunks)
- x/crosschain/keeper/bridge_call_in.go (3 hunks)
- x/crosschain/keeper/bridge_call_in_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
x/crosschain/keeper/bridge_call_in_test.go (3)
19-52: Improved test structure and readabilityThe refactoring of the
TestBridgeCallHandlerfunction has significantly improved the test structure:
- The change from
KeeperMockSuitetoKeeperTestSuitesuggests a move towards more integrated testing.- The new test case structure using a slice of structs with named fields enhances readability and maintainability.
- Additional fields in the test cases allow for more comprehensive test scenarios.
These changes will make it easier to add new test cases and understand existing ones.
74-96: Well-structured test initialization methodThe new
BridgeCallClaimInitializemethod is a great addition to the test suite:
- It encapsulates complex setup logic, improving test readability and maintainability.
- The length check on line 75 addresses the potential index out of range issue raised in a previous review comment.
- The method covers all necessary setup steps for the test cases, including handling of native and non-native tokens.
This method effectively addresses the previous concern about potential index out of range errors and provides a clean, reusable way to initialize test data.
Line range hint
1-150: Overall improvement in test structure with opportunity for further refinementThe changes to this test file represent a significant improvement in test structure, readability, and maintainability:
- The refactoring of
TestBridgeCallHandlerand the introduction ofBridgeCallClaimInitializeenhance the organization and reusability of test code.- The new approach using
KeeperTestSuiteseems to move towards more integrated testing, which can provide better coverage.- The changes address previous concerns about potential errors and improve error handling.
However, there's an opportunity to further refine the test suite by consistently applying the new approach across all test methods. Consider completing the transition from
KeeperMockSuitetoKeeperTestSuitefor the remaining methods to achieve full consistency throughout the file.These changes significantly improve the quality of the test suite, and with a bit more refactoring, it will be even more robust and maintainable.
x/crosschain/keeper/bridge_call_in.go (2)
23-27: Excellent implementation of the module account checkThe addition of this check ensures that module accounts cannot be used as senders, which is a good security practice. The error message has also been improved as per the previous review suggestion.
64-93: Approved: New BridgeTokenToERC20 function with suggestionsThe new
BridgeTokenToERC20function effectively handles the conversion of tokens to base coins and retrieves the corresponding ERC20 addresses and amounts.
Address the TODO comment:
// TODO: Replace convert coin operation with a more efficient method // that returns the token pair directlyConsider creating a GitHub issue to track this improvement.
Improve error handling when getting the token pair:
pair, found := k.erc20Keeper.GetTokenPair(sdk.UnwrapSDKContext(ctx), coin.Denom) if !found { return nil, nil, nil, fmt.Errorf("token pair not found for denom: %s", coin.Denom) }This will prevent potential panics if the token pair is not found.
Run the following script to check for other occurrences of ignored errors from
GetTokenPair:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
BridgeCallHandlerto improve clarity and organization.