refactor: convert token in bridgecall #724
Conversation
WalkthroughThe pull request introduces significant modifications across multiple files, primarily enhancing 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: 8
🧹 Outside diff range and nitpick comments (9)
x/crosschain/types/expected_keepers.go (1)
53-57: LGTM! Consider reordering methods for better grouping.The changes to the
Erc20Keeperinterface look good. The addition of theConvertERC20method complements the existingConvertCoinmethod, enhancing the token conversion capabilities. The repositioning ofGetTokenPairimproves the logical grouping of related methods.To further improve readability and maintain a consistent structure, consider grouping related methods together. For example:
type Erc20Keeper interface { GetTokenPair(ctx sdk.Context, tokenOrDenom string) (erc20types.TokenPair, bool) ConvertCoin(ctx context.Context, msg *erc20types.MsgConvertCoin) (*erc20types.MsgConvertCoinResponse, error) ConvertERC20(goCtx context.Context, msg *erc20types.MsgConvertERC20) (*erc20types.MsgConvertERC20Response, error) ConvertDenomToTarget(ctx sdk.Context, from sdk.AccAddress, coin sdk.Coin, fxTarget fxtypes.FxTarget) (sdk.Coin, error) TransferAfter(ctx sdk.Context, sender sdk.AccAddress, receive string, coin, fee sdk.Coin, _ bool) error // ... other methods ... }This grouping places all conversion-related methods together, followed by transfer-related methods.
x/crosschain/keeper/outgoing_pool.go (2)
29-29: LGTM! Consider adding a comment for clarity.The change simplifies the
BaseCoinToBridgeTokenfunction call by removing an unused return value, which is a good practice. The core functionality remains intact.Consider adding a brief comment explaining why only one return value is now used, to provide context for future developers:
// Only the tokenContract is needed; error is the second return value tokenContract, err := k.BaseCoinToBridgeToken(ctx, amount.Add(fee), sender)
221-224: LGTM! Consider enhancing error handling.The change simplifies the code by directly using the
sdkmath.Inttype forrefundAmountwithout conversion, which improves clarity and maintainability.Consider wrapping the error returned by
BridgeTokenToBaseCoinwith additional context:baseCoin, err := k.BridgeTokenToBaseCoin(ctx, tokenContract, refundAmount, sender) if err != nil { return sdk.Coin{}, fmt.Errorf("failed to convert bridge token to base coin: %w", err) }This will provide more informative error messages for debugging purposes.
x/crosschain/types/msgs.go (1)
486-488: Summary of changes and recommendationsThe addition of the
IsMemoSendCallTomethod toMsgBridgeCallClaimenhances the functionality for checking memo contents in bridge call claims. This change appears to be part of a larger feature or refactoring effort related to memo handling in cross-chain operations.Recommendations:
- Ensure that the
IsMemoSendCallTofunction is properly implemented and documented in the appropriate file.- Update or add unit tests to cover this new functionality.
- Review any other parts of the codebase that might benefit from using this new method.
- Consider adding comments to explain the purpose and expected behavior of this method, especially if it's part of a new feature.
As this change might be part of a larger feature, it would be beneficial to provide some context in the form of comments or documentation about the overall design and purpose of this new functionality.
x/crosschain/keeper/send_to_fx.go (1)
Line range hint
105-105: Return 'nil' explicitly when no error has occurredAt the end of the
transferIBCHandlerfunction, it's clearer to returnnilexplicitly rather than returningerr, which should benilat this point. This enhances code readability and reduces potential confusion.Apply this diff to return
nilexplicitly:- return err + return niltypes/target.go (2)
36-41: Consider simplifying theisHexparameterThe
isHexparameter is declared as a variadic boolean (isHex ...bool), but it seems to be used as an optional flag with at most one value. Using a variadic parameter in this context may lead to confusion. Consider changing it to a single boolean parameter with a default value to improve code readability and clarity.Apply this diff:
-func ParseFxTarget(targetStr string, isHex ...bool) FxTarget { - if len(isHex) > 0 && isHex[0] { +func ParseFxTarget(targetStr string, isHex bool) FxTarget { + if isHex { // ignore hex decode error targetByte, _ := hex.DecodeString(targetStr) targetStr = string(targetByte) }
128-130: Ensure consistent case when comparing prefixesIn the comparison
strings.ToLower(i.Prefix) == contract.EthereumAddressPrefix, onlyi.Prefixis converted to lowercase. Ifcontract.EthereumAddressPrefixcontains uppercase letters, this comparison might fail unexpectedly. For robustness, consider converting both strings to lowercase to ensure a case-insensitive comparison.Apply this diff:
func (i FxTarget) ReceiveAddrToStr(receive sdk.AccAddress) (receiveAddrStr string, err error) { - if strings.ToLower(i.Prefix) == contract.EthereumAddressPrefix { + if strings.ToLower(i.Prefix) == strings.ToLower(contract.EthereumAddressPrefix) { return common.BytesToAddress(receive.Bytes()).String(), nil } return bech32.ConvertAndEncode(i.Prefix, receive) }x/crosschain/keeper/many_to_one.go (2)
20-51: Consider Wrapping Errors with Additional ContextIn both
EvmToBaseCoinandBaseCoinToEvmfunctions, errors returned fromConvertERC20andConvertCoinare propagated without additional context. Wrapping these errors can provide more informative messages, aiding in debugging.Apply this diff to wrap errors with context:
if err != nil { - return sdk.Coin{}, err + return sdk.Coin{}, sdkerrors.Wrap(err, "failed to convert ERC20 to base coin") } --- if err != nil { - return "", err + return "", sdkerrors.Wrap(err, "failed to convert base coin to ERC20") }
Line range hint
58-68: Check for Possible Redundant Conversion inBridgeTokenToBaseCoinIn the
BridgeTokenToBaseCoinfunction, after depositing the bridge token and callingManyToOne, theConversionCoinfunction is called withbaseDenomas both thebaseDenomandtargetDenom. This may be redundant if no actual conversion is needed between the denominations.Consider adding a condition to bypass the conversion when
baseDenomandtargetDenomare the same.if err = k.ConversionCoin(ctx, holder, bridgeToken, baseDenom, baseDenom); err != nil { return sdk.Coin{}, err } +// Skip conversion if baseDenom and targetDenom are the same +if baseDenom != bridgeToken.Denom { + if err = k.ConversionCoin(ctx, holder, bridgeToken, baseDenom, baseDenom); err != nil { + return sdk.Coin{}, err + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- types/target.go (3 hunks)
- x/crosschain/keeper/bridge_call_in.go (3 hunks)
- x/crosschain/keeper/bridge_call_out.go (1 hunks)
- x/crosschain/keeper/grpc_query_v1_test.go (1 hunks)
- x/crosschain/keeper/hook.go (0 hunks)
- x/crosschain/keeper/many_to_one.go (2 hunks)
- x/crosschain/keeper/many_to_one_test.go (2 hunks)
- x/crosschain/keeper/msg_server.go (1 hunks)
- x/crosschain/keeper/outgoing_pool.go (2 hunks)
- x/crosschain/keeper/send_to_fx.go (1 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (1 hunks)
- x/crosschain/precompile/bridge_call.go (1 hunks)
- x/crosschain/precompile/expected_keepers.go (2 hunks)
- x/crosschain/types/expected_keepers.go (1 hunks)
- x/crosschain/types/msgs.go (1 hunks)
- x/crosschain/types/types.go (0 hunks)
💤 Files with no reviewable changes (2)
- x/crosschain/keeper/hook.go
- x/crosschain/types/types.go
🧰 Additional context used
🔇 Additional comments (27)
x/crosschain/keeper/bridge_call_out.go (1)
Line range hint
1-48: Summary of changes in bridge_call_out.goThe main change in this file is the modification of the
AddOutgoingBridgeCallfunction. The function now acceptssdk.Coinsinstead of[]types.ERC20Token, which improves its flexibility and aligns better with Cosmos SDK conventions. A conversion loop has been added to maintain compatibility with the existing logic.These changes are well-implemented and don't introduce any apparent issues. However, it's important to ensure that all callers of this function throughout the codebase have been updated to pass
sdk.Coinsinstead of[]types.ERC20Token.To ensure a smooth transition, please verify that all callers of
AddOutgoingBridgeCallhave been updated accordingly. You may use the script provided in the previous comment to assist with this verification.x/crosschain/keeper/outgoing_pool.go (1)
Line range hint
1-262: Overall, the changes improve code clarity and maintainability.The modifications in this file focus on simplifying function calls and improving type handling. These changes align well with the pull request's objective of refactoring token handling in bridge calls. The core functionality of the affected functions remains intact, while unnecessary complexity has been removed.
Key improvements:
- Simplified
BaseCoinToBridgeTokenfunction call inaddToOutgoingPool.- Direct use of
sdkmath.IntinhandleCancelRefundwithout type conversion.These changes contribute to a more streamlined and maintainable codebase.
x/crosschain/keeper/many_to_one_test.go (3)
373-373: LGTM: Simplified function call improves readabilityThe modification to
BridgeTokenToBaseCoincall, changingamount.BigInt()toamount, aligns with the updated method signature. This change simplifies the function call, improves code readability, and reduces unnecessary type conversions. The consistency of this change across all test cases in theTestBridgeTokenToBaseCoinfunction is commendable.
Line range hint
1-452: Overall: Positive changes improving test suite clarity and alignmentThe modifications in this file are part of a larger refactoring effort to simplify token conversion processes. These changes:
- Improve code readability by simplifying function calls.
- Align the test cases with updated method signatures.
- Potentially streamline internal processes for token conversion.
The consistency of these changes across multiple test functions is commendable. These updates enhance the clarity of the test suite and ensure it accurately reflects the current implementation of the token conversion functions.
440-440: LGTM: Simplified function call, verify impact on related codeThe modification to
BaseCoinToBridgeTokencall, removing thesuite.chainNameparameter, aligns with the updated method signature. This change simplifies the function call and potentially indicates a more streamlined internal process for token conversion. The consistency of this change across all test cases in theTestBaseCoinToBridgeTokenfunction is positive.To ensure this change doesn't have unintended consequences, please run the following script to check for any remaining uses of
chainNamein related functions:✅ Verification successful
Verified:
chainNameparameter successfully removed without residual usagesThe
suite.chainNameparameter has been removed from theBaseCoinToBridgeTokenfunction call, and verification confirms that there are no remaining usages ofchainNamein thex/crosschainpackage. This change is correctly implemented and does not impact other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining uses of chainName in BaseCoinToBridgeToken and related functions # Search for chainName usage in BaseCoinToBridgeToken function echo "Checking BaseCoinToBridgeToken function:" rg --type go "func.*BaseCoinToBridgeToken.*chainName" ./x/crosschain # Search for chainName usage in related functions echo "Checking related functions:" rg --type go "func.*(BaseCoinTo|ToBridgeToken).*chainName" ./x/crosschainLength of output: 302
x/crosschain/keeper/msg_server.go (1)
468-468: Approve simplification of token handling in BridgeCallThe change simplifies the
BridgeCallmethod by directly passingmsg.CoinstoAddOutgoingBridgeCall, removing the intermediate step of converting to ERC20 tokens. This approach is more flexible and reduces complexity.Please ensure that the
AddOutgoingBridgeCallfunction can handle all types of coins that might be passed. Run the following script to verify the implementation:x/crosschain/types/msgs.go (2)
486-488: New method added to MsgBridgeCallClaimThe
IsMemoSendCallTomethod has been added to theMsgBridgeCallClaimstruct. This method checks if the memo associated with the message indicates a "send call to" action.A few observations:
- The method uses
MustMemo()to get the memo data, which will panic if the memo is invalid. This is consistent with otherMust*methods in the file.- The
IsMemoSendCallTofunction is called with the result ofMustMemo(), but we don't see the implementation ofIsMemoSendCallToin this file.To ensure the
IsMemoSendCallTofunction is properly implemented, let's check for its definition:#!/bin/bash # Search for the IsMemoSendCallTo function definition rg --type go "func IsMemoSendCallTo\("
486-488: Verify usage and test coverage for new methodThe addition of the
IsMemoSendCallTomethod toMsgBridgeCallClaimis a new feature that should be properly tested and potentially used in other parts of the codebase.Let's check for any usage of this new method and ensure that appropriate tests have been added:
x/crosschain/mock/expected_keepers_mocks.go (3)
431-438: LGTM: ConvertERC20 method implementation looks correct.The
ConvertERC20method is properly implemented for theMockErc20Keeper. It follows the same pattern as other methods in this mock, using thegomocklibrary correctly.
440-444: LGTM: ConvertERC20 recorder method implementation is correct.The
ConvertERC20recorder method is properly implemented for theMockErc20KeeperMockRecorder. It follows the same pattern as other recorder methods in this mock, using thegomocklibrary correctly.
431-444: Summary: New ConvertERC20 mock method added correctly.The changes in this file add a new
ConvertERC20method to theMockErc20Keeperand its corresponding recorder method toMockErc20KeeperMockRecorder. These additions are consistent with the existing code structure and correctly implement the mocking functionality using thegomocklibrary. The new methods likely support testing of ERC20 conversion functionality in the system.x/crosschain/keeper/send_to_fx.go (3)
Line range hint
78-80: Confirm the modification of IBC transfer timeout height calculationThe timeout height for IBC transfers is now calculated by multiplying the result of
k.GetIbcTransferTimeoutHeight(ctx)by 5:ibcTransferTimeoutHeight := k.GetIbcTransferTimeoutHeight(ctx) * 5Please confirm that increasing the timeout height by this factor is intentional and aligns with the desired timeout behavior for IBC transfers.
72-75: Ensure 'ReceiveAddrToStr' correctly converts receiver addressesThe function now uses
target.ReceiveAddrToStr(receive)to obtainibcReceiveAddress. Please verify thatReceiveAddrToStrcorrectly handles the conversion of thereceiveaddress to the appropriate format required for IBC transfers, especially when dealing with different address prefixes or formats.
47-50: Verify that 'claim.TargetIbc' is correctly parsed without hex decodingThe updated code parses
claim.TargetIbcdirectly usingfxtypes.ParseFxTarget(claim.TargetIbc, true)without prior hex decoding. Previously, there might have been a hex decoding step fortargetHex. Please ensure thatclaim.TargetIbcis in the expected format and that omitting hex decoding does not introduce any issues with address parsing.You can verify the usage of
ParseFxTargetand ensure thatclaim.TargetIbcis properly handled:x/crosschain/precompile/expected_keepers.go (2)
5-5: Importingmath/bigThe addition of the
"math/big"import is appropriate as it's required for the use of*big.Intin theEvmToBaseCoinmethod.
53-53: Addition ofAddOutgoingBridgeCallmethodThe new method
AddOutgoingBridgeCalladded to theCrosschainKeeperinterface aligns with the interface's responsibilities and extends its functionality appropriately.x/crosschain/precompile/bridge_call.go (2)
60-60: RenamingcoinstobaseCoinsImproves ClarityRenaming the variable from
coinstobaseCoinsenhances readability by clearly indicating that the coins are base coins resulting from token conversion.
81-81: Update toAddOutgoingBridgeCallReflects Correct Method UsageThe change to use
route.AddOutgoingBridgeCallwith the additional parameter0aligns with the updated bridge call processing logic.types/target.go (1)
131-131: Confirm proper error handling forbech32.ConvertAndEncodeThe
bech32.ConvertAndEncodefunction returns a string and an error. While the methodReceiveAddrToStrincludes an error return value, ensure that any errors frombech32.ConvertAndEncodeare properly handled and propagated to avoid unexpected issues when encoding addresses.To verify that errors are properly handled downstream, run the following script to find all usages of
ReceiveAddrToStrand check if the returned error is being handled:✅ Verification successful
Error Handling for
bech32.ConvertAndEncodeConfirmedAll usages of
ReceiveAddrToStrproperly handle and propagate errors frombech32.ConvertAndEncode. No issues were found in the current implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of ReceiveAddrToStr and check for error handling. rg --type go 'ReceiveAddrToStr\(' -A 3Length of output: 563
x/crosschain/keeper/bridge_call_in.go (5)
27-30: Receiver address assignment logic is validThe updated code correctly assigns
receiverAddrbased on theisMemoSendCallToflag. This change ensures that whenisMemoSendCallTois true, thereceiverAddris set to the sender's address, aligning with the intended functionality.
43-45: Updated parameters in BridgeCallEvm align with function signatureThe call to
k.BridgeCallEvmnow includesreceiverAddrandbaseCoins, matching the updated function signature. This change ensures that the correct parameters are passed and that the function operates as intended.
60-60: BridgeCallFailedRefund now uses baseCoins correctlyThe
k.BridgeCallFailedRefundfunction is updated to acceptbaseCoinsinstead oferc20Token, reflecting the refactored token handling logic. This modification aligns with the overall changes in the codebase.
63-72: Conversion from baseCoins to tokens and amounts is handled properlyIn
BridgeCallEvm, the loop convertsbaseCoinsto correspondingtokensandamounts. The use ofk.BaseCoinToEvmwithreceiverAddrensures that each base coin is correctly mapped to its EVM token representation. This approach maintains consistency in the token conversion process.
104-105: BridgeCallFailedRefund function parameters updated appropriatelyThe
BridgeCallFailedRefundfunction now acceptsbaseCoinsas an argument. This change is consistent with the shift from handlingERC20Tokento usingsdk.Coins, streamlining the refund process.x/crosschain/keeper/many_to_one.go (3)
72-87: EnsuremoduleNameUsage Is Appropriate After Parameter RemovalIn the
BaseCoinToBridgeTokenfunction, themoduleparameter has been removed, andk.moduleNameis used directly when callingk.ManyToOne. Confirm that this change aligns with the intended functionality and thatk.moduleNameis the correct value to use in all contexts.To verify, ensure that
k.moduleNamecorrectly represents the target module in all scenarios whereBaseCoinToBridgeTokenis used.
37-51: Confirm Sender and Receiver Addresses Are Correctly AssignedIn the
BaseCoinToEvmfunction, theSenderis assigned assdk.AccAddress(holder.Bytes()).String(), and theReceiverisholder.String(). Ensure that theSenderrepresents the correct Cosmos SDK address and theReceiveris the correct Ethereum address. Misassignment may cause transaction failures or misdirected funds.To ensure correctness, verify that:
sdk.AccAddress(holder.Bytes()).String()accurately converts the Ethereumholderaddress to a Cosmos SDK address.holder.String()provides the correct Ethereum address format for the receiver.You can use the following script to search for inconsistent address conversions:
#!/bin/bash # Description: Identify inconsistent address conversions between Ethereum and Cosmos SDK addresses. # Test: Search for address conversions and assignments in the codebase. # Expect: Correct and consistent assignments. rg --type go 'Sender:\s+sdk\.AccAddress\([^)]+\.Bytes\(\)\)\.String\(\)|Receiver:\s+[^,]+' x/crosschain/
Line range hint
53-68: Update Function Calls Due to Parameter ChangeThe
BridgeTokenToBaseCoinfunction signature has changed, replacingamount *big.Intwithamount sdkmath.Int. Ensure that all calls to this function throughout the codebase are updated to passsdkmath.Intinstead of*big.Int.To verify, check for all usages of
BridgeTokenToBaseCoin:
Summary by CodeRabbit
Release Notes
New Features
FxTargetstruct with improved parsing and address conversion capabilities.ConvertERC20method in the mock keeper for testing ERC20 token conversions.Improvements
Bug Fixes
Refactor