refactor(precompile): migrate cross-chain logic to cosmos code #741
Conversation
WalkthroughThe changes in this pull request enhance cross-chain functionality and ERC20 token management across multiple files. Key modifications include updates 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: 20
🧹 Outside diff range and nitpick comments (17)
x/crosschain/precompile/router.go (1)
57-62: LGTM with suggestions: New methodGetRouteWithIBCadded correctly.The new
GetRouteWithIBCmethod enhances the router's functionality by handling IBC targets, which aligns with the PR objectives. However, consider the following improvements:
- Add a comment explaining the purpose and behavior of this method.
- Consider adding error handling for the case where
ParseFxTargetmight fail.Here's a suggested implementation with comments and error handling:
// GetRouteWithIBC retrieves the CrosschainKeeper for a given target, // handling IBC targets by mapping them to the eth module. func (rtr *Router) GetRouteWithIBC(target string) (CrosschainKeeper, bool) { parsedTarget, err := fxtypes.ParseFxTarget(target) if err != nil { // Handle the error appropriately, e.g., log it or return a default value return nil, false } if parsedTarget.IsIBC() { target = ethtypes.ModuleName } return rtr.GetRoute(target) }x/crosschain/keeper/keeper.go (2)
31-31: LGTM! Consider adding a comment for the new field.The addition of the
evmErc20Keeperfield aligns well with the PR objective of migrating cross-chain logic. It follows the existing naming conventions and uses an interface type, which is good for maintainability.Consider adding a brief comment explaining the purpose of this new keeper, similar to other fields in the struct. For example:
// evmErc20Keeper manages EVM ERC20 token interactions evmErc20Keeper types.EvmERC20Keeper
41-41: LGTM! Consider updating the comment for consistency.The update to the
NewKeeperfunction signature correctly incorporates the newevmErc20Keeperparameter. The placement and grouping of parameters improve readability.For consistency, consider updating the function comment to include the new parameter:
// NewKeeper returns a new instance of the crosschain keeper func NewKeeper(cdc codec.BinaryCodec, moduleName string, storeKey storetypes.StoreKey, stakingKeeper types.StakingKeeper, stakingMsgServer types.StakingMsgServer, distributionKeeper types.DistributionMsgServer, bankKeeper types.BankKeeper, ibcTransferKeeper types.IBCTransferKeeper, erc20Keeper types.Erc20Keeper, ak types.AccountKeeper, evmKeeper types.EVMKeeper, evmErc20Keeper types.EvmERC20Keeper, authority string, ) Keeper { // ... (existing implementation) }x/crosschain/precompile/bridge_coin_amount_test.go (1)
59-59: Improved error message, consider minor enhancement.The updated error message provides more specific information about the failure condition, which is a good improvement. It now clearly states that the token pair was not found and that the coins are invalid.
Consider standardizing the error message format across the codebase. For example:
fmt.Errorf("token pair not found for %s: invalid coins (evm transaction execution failed)", token.String())This format separates the main error message from additional context and the underlying cause, making it easier to parse and understand.
x/evm/keeper/erc20.go (1)
49-55: Approve changes with a minor suggestion for consistency.The implementation of the
TotalSupplymethod looks good and aligns well with the existing codebase. It correctly queries the ERC20 contract for the total supply and handles potential errors.For consistency with other methods in this file, consider initializing the return value to
big.NewInt(0)in case of an error. This would make it behave similarly to theERC20BalanceOfmethod.Here's a suggested minor modification:
func (k *Keeper) TotalSupply(ctx context.Context, contractAddr common.Address) (*big.Int, error) { var totalSupplyRes struct{ Value *big.Int } if err := k.QueryContract(sdk.UnwrapSDKContext(ctx), k.module, contractAddr, contract.GetFIP20().ABI, "totalSupply", &totalSupplyRes); err != nil { - return nil, err + return big.NewInt(0), err } return totalSupplyRes.Value, nil }x/crosschain/keeper/keeper_test.go (1)
52-52: LGTM! Consider adding a comment for clarity.The addition of
evmErc20Keeperto theKeeperMockSuitestruct is appropriate and consistent with the existing structure. This change enhances the test suite's capability to simulate interactions with the EVM ERC20 keeper.Consider adding a brief comment above this field to explain its purpose, maintaining consistency with any existing field documentation in the struct.
x/crosschain/keeper/grpc_query.go (1)
298-302: Approve changes with a suggestion for improved error handlingThe simplification of the
BridgeCoinByDenommethod is a good improvement. It reduces unnecessary checks and makes the code more concise. However, there's a potential issue with error handling.Consider improving the error handling to distinguish between "denom not found" and other potential errors:
supply, err := k.BridgeCoinSupply(sdk.UnwrapSDKContext(c), req.GetDenom(), req.GetChainName()) if err != nil { - return nil, status.Error(codes.NotFound, "denom") + if errors.Is(err, types.ErrUnknownDenom) { + return nil, status.Error(codes.NotFound, "denom not found") + } + return nil, status.Error(codes.Internal, fmt.Sprintf("failed to get bridge coin supply: %v", err)) }This change assumes that
BridgeCoinSupplyreturns a specific error type (types.ErrUnknownDenom) when the denomination is not found. If this is not the case, you may need to adjust the error checking accordingly.x/crosschain/precompile/keeper.go (2)
24-25: Handle Specific Errors fromGetBaseDenomByErc20Currently, any error from
crossChainKeeper.GetBaseDenomByErc20results in returning an emptysdk.Coinand the error. Consider handling specific error cases to provide more informative feedback to the caller, which can aid in debugging and improve user experience.
51-52: Add Logging for Error HandlingWhen sending coins from the module to the account, adding logging statements for errors can enhance observability and aid in debugging. This will provide more context in case of failures and make troubleshooting more efficient.
x/crosschain/precompile/bridge_coin_amount.go (1)
50-50: Provide a more descriptive error messageThe error message
"invalid router"may not offer enough context for debugging. Consider refining it to be more descriptive, such as"crosschain module not found in router".Apply this diff to improve the error message:
if !found { - return nil, errors.New("invalid router") + return nil, errors.New("crosschain module not found in router") }x/crosschain/precompile/bridge_call.go (1)
69-69: Enhance error message for better clarityConsider providing more context in the error message to aid in debugging. Including the destination chain can help identify issues quickly.
Apply this diff to improve the error message:
import ( "errors" + "fmt" "math/big" - return errors.New("invalid router") + return fmt.Errorf("invalid router for destination chain: %s", args.DstChain)x/crosschain/types/expected_keepers.go (2)
61-62: Re-evaluate the placement of IBC-related methods inErc20KeeperThe methods
SetIBCTransferRelationandDeleteIBCTransferRelationpertain to IBC transfer relations, which may not be directly related to the core responsibilities of theErc20Keeperinterface focused on ERC20 token operations.Suggestion:
Consider moving these methods to a dedicated interface that handles IBC transfer functionalities, such as an
IBCTransferKeeperor a new interface specific to cross-chain transfer relations. This will promote better separation of concerns and adhere to the Single Responsibility Principle.
93-95: Add documentation for the newEvmERC20KeeperinterfaceThe newly introduced
EvmERC20Keeperinterface lacks documentation comments.Suggestion:
Add GoDoc comments to the
EvmERC20Keeperinterface and itsTotalSupplymethod to enhance readability and maintainability. For example:// EvmERC20Keeper defines the expected interface for interacting with EVM ERC20 tokens. type EvmERC20Keeper interface { // TotalSupply retrieves the total supply of the ERC20 token at the specified contract address. TotalSupply(ctx context.Context, contractAddr common.Address) (*big.Int, error) }x/crosschain/keeper/bridge_token.go (2)
182-199: Add unit tests forBridgeCoinSupplyfunctionThe new
BridgeCoinSupplyfunction introduces critical functionality for managing bridge coin supplies. To ensure reliability and correctness across different scenarios (native ERC20 tokens, non-native tokens, error conditions), please consider adding comprehensive unit tests.Would you like assistance in writing unit tests for this function?
201-207: Add unit tests forGetBaseDenomByErc20functionTo validate the behavior of
GetBaseDenomByErc20in various scenarios—including cases where the token pair exists or does not exist—it's advisable to include unit tests. This will help prevent regressions and ensure the function handles all edge cases appropriately.x/crosschain/keeper/bridge_call_out.go (1)
300-308: Include underlying error details in address validation error messagesWhen validating the
toaddress in theIBCTransferfunction, the error messages currently do not include the underlying error details, which could aid in debugging by providing more context about why the address is invalid. Consider modifying the error messages to include the error information.Apply this diff to include the error details:
if strings.ToLower(fxTarget.Prefix) == contract.EthereumAddressPrefix { if err := contract.ValidateEthereumAddress(to); err != nil { - return 0, fmt.Errorf("invalid to address: %s", to) + return 0, fmt.Errorf("invalid to address '%s': %v", to, err) } } else { if _, err := sdk.GetFromBech32(to, fxTarget.Prefix); err != nil { - return 0, fmt.Errorf("invalid to address: %s", to) + return 0, fmt.Errorf("invalid to address '%s': %v", to, err) } }app/keepers/keepers.go (1)
Line range hint
380-487: Refactor repetitive cross-chain keeper initializationThe initialization code for the cross-chain keepers is repetitive. Consider refactoring this code to eliminate duplication, which will improve maintainability and reduce the risk of inconsistencies.
You can create a helper function to handle the initialization. Here's an example:
func initializeCrossChainKeeper( moduleName string, storeKey storetypes.StoreKey, appKeepers *AppKeepers, appCodec codec.Codec, authAddr string, ) crosschainkeeper.Keeper { return crosschainkeeper.NewKeeper( appCodec, moduleName, appKeepers.keys[storeKey], appKeepers.StakingKeeper, stakingkeeper.NewMsgServerImpl(appKeepers.StakingKeeper.Keeper), distrkeeper.NewMsgServerImpl(appKeepers.DistrKeeper), appKeepers.BankKeeper, appKeepers.IBCTransferKeeper, appKeepers.Erc20Keeper, appKeepers.AccountKeeper, appKeepers.EvmKeeper, authAddr, ) }Then initialize each keeper like:
appKeepers.BscKeeper = initializeCrossChainKeeper(bsctypes.ModuleName, bsctypes.StoreKey, appKeepers, appCodec, authAddr)This refactoring reduces code duplication and makes it easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- app/keepers/keepers.go (9 hunks)
- x/crosschain/keeper/bridge_call_out.go (2 hunks)
- x/crosschain/keeper/bridge_token.go (2 hunks)
- x/crosschain/keeper/grpc_query.go (1 hunks)
- x/crosschain/keeper/keeper.go (3 hunks)
- x/crosschain/keeper/keeper_test.go (3 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (4 hunks)
- x/crosschain/precompile/bridge_call.go (1 hunks)
- x/crosschain/precompile/bridge_coin_amount.go (2 hunks)
- x/crosschain/precompile/bridge_coin_amount_test.go (1 hunks)
- x/crosschain/precompile/contract.go (1 hunks)
- x/crosschain/precompile/crosschain.go (1 hunks)
- x/crosschain/precompile/expected_keepers.go (1 hunks)
- x/crosschain/precompile/keeper.go (1 hunks)
- x/crosschain/precompile/router.go (2 hunks)
- x/crosschain/types/expected_keepers.go (3 hunks)
- x/crosschain/types/key.go (1 hunks)
- x/evm/keeper/erc20.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (21)
x/crosschain/precompile/router.go (2)
8-8: LGTM: New imports added correctly.The new imports for
fxtypesandethtypesare correctly added and are necessary for the newGetRouteWithIBCmethod.Also applies to: 10-10
Line range hint
1-62: Overall assessment: Changes look good with minor suggestions for improvement.The modifications to this file successfully introduce IBC target handling in the router, aligning with the PR objectives of migrating cross-chain logic. The new imports and the
GetRouteWithIBCmethod are correctly implemented. Consider adding error handling and improving documentation as suggested in the previous comments to enhance the robustness and maintainability of the code.x/crosschain/keeper/keeper.go (1)
31-31: Verify the usage of the newevmErc20Keeperin other files.The changes to add the
evmErc20Keeperfield and update theNewKeeperfunction are well-implemented and align with the PR objective. However, I noticed that the new keeper is not used in any of the existing methods in this file.To ensure the new keeper is properly integrated, please run the following script to check its usage across the codebase:
This will help verify that the new keeper is being utilized as intended in other parts of the crosschain module.
Also applies to: 41-41, 60-60
x/crosschain/precompile/bridge_coin_amount_test.go (1)
Line range hint
1-108: Overall assessment: Minor improvement to error messaging.The change in this file is limited to updating an error message in one of the test cases. This modification enhances the clarity of the error reporting without altering the test logic or coverage. It's a positive change that contributes to better error handling practices in the codebase.
x/crosschain/keeper/keeper_test.go (2)
103-103: LGTM! Proper initialization of the new mock keeper.The
evmErc20Keeperis correctly initialized using the mock controller. This addition is consistent with the initialization of other mock keepers in theSetupTestmethod.
119-119: LGTM! Verify NewKeeper function signature.The addition of
s.evmErc20Keeperas a parameter to thecrosschainkeeper.NewKeeperfunction call is appropriate and consistent with the integration of EVM ERC20 functionality.To ensure this change is correctly implemented, let's verify the
NewKeeperfunction signature:✅ Verification successful
LGTM! Verified
NewKeeperfunction signature.The
NewKeeperfunction now includesevmErc20Keeperas a parameter, confirming the correct integration of EVM ERC20 functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the NewKeeper function signature in the crosschain keeper package. # Test: Search for the NewKeeper function definition rg --type go -A 5 'func NewKeeper\(' x/crosschain/keeper/Length of output: 732
x/crosschain/precompile/expected_keepers.go (3)
6-6: Import of 'sdkmath' is necessary and appropriateThe addition of the import
sdkmath "cosmossdk.io/math"is required for the use ofsdkmath.Intin theBridgeCallBaseCoinmethod.
21-21: Addition of 'SendCoins' method enhances 'BankKeeper' interfaceIncluding the
SendCoinsmethod provides direct coin transfer functionality between accounts, aligning with standard operations in the bank module.
29-29: Addition of 'GetBaseDenomByErc20' method is appropriateThe
GetBaseDenomByErc20method extends theCrosschainKeeperinterface, allowing retrieval of the base denomination using an ERC20 address, which enhances cross-chain compatibility.x/crosschain/precompile/keeper.go (1)
31-33:⚠️ Potential issueVerify Proper Initialization and Usage of
erc20CallEnsure that
erc20Callis correctly initialized with the appropriate parameters before callingBurn. Additionally, confirm that theBurnoperation aligns with the token's logic and that theholderhas the necessary permissions to burn tokens.To automate this verification, you can run the following script:
x/crosschain/precompile/bridge_coin_amount.go (2)
4-4: Importing the 'errors' package is necessaryThe addition of the
"errors"package import is appropriate and required for error handling in the updated code.
57-57: Return statement correctly packs the outputThe return statement properly packs the total supply amount into the expected output format using
m.PackOutput.x/crosschain/precompile/bridge_call.go (3)
62-63: Verify the source address inSendCoinsYou're using
crosschaintypes.GetAddress().Bytes()as the source address to send coins to the sender. Please ensure that this address is intended to be the source of the funds and that it holds sufficient balance for the transaction.
75-75: Ensure correct mapping of tokens to amountsWhen iterating over
args.Tokens, verify that each token corresponds accurately to its amount inargs.Amounts[i]. Any misalignment could lead to incorrect token conversions or transaction failures.
81-82: Confirm parameter order inBridgeCallBaseCoinDouble-check that the parameters passed to
crosschainKeeper.BridgeCallBaseCoinare in the correct order and type, especially with the addition oforiginTokenAmount. Incorrect parameter placement could cause unexpected behavior.x/crosschain/precompile/crosschain.go (3)
89-90: Confirm parameters inCrossChainBaseCoincallVerify that all parameters passed to
crosschainKeeper.CrossChainBaseCoinalign with its expected signature. Specifically, ensure thattargetis in the correct format and type as required by the function.
81-84:⚠️ Potential issueHandle potential nil values from
EvmTokenToBaseEnsure that
totalCoinreturned fromm.EvmTokenToBaseis not nil before using it. If there's a possibility of receiving a nil value without an error, add a check to prevent a nil pointer dereference.Consider adding a nil check:
if totalCoin.IsNil() { return errors.New("received nil totalCoin from EvmTokenToBase") }
74-75:⚠️ Potential issueVerify the direction of funds transfer in
SendCoinsIn the
SendCoinscall, funds are being sent from the cross-chain module account to the sender:if err = m.bankKeeper.SendCoins(ctx, crosschaintypes.GetAddress().Bytes(), sender.Bytes(), sdk.NewCoins(totalCoin)); err != nil { return err }Please verify if this is the intended behavior. Typically, the sender should be transferring funds to the module account when initiating a cross-chain transaction. This could be a potential logic error.
To confirm if the
SendCoinsfunction is used correctly, you can run the following script:✅ Verification successful
Direction of funds transfer in
SendCoinsconfirmed as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of SendCoins where funds are sent from the module account to the sender. # Expected result: Verify if this pattern is intentional or deviates from standard practices. rg --type go 'SendCoins\([^,]+,\s*crosschaintypes\.GetAddress\(\)\.Bytes\(\),\s*sender\.Bytes\(\)' -A 5Length of output: 1041
Script:
#!/bin/bash # Description: Retrieve all usages of SendCoins to analyze fund transfer directions. rg --type go 'SendCoins\(' -A 5Length of output: 8645
x/crosschain/keeper/bridge_call_out.go (2)
261-286: Verify the necessity of callingBuildOutgoingTxBatchwhenoriginTokenistrueIn the
CrossChainBaseCoinfunction,BuildOutgoingTxBatchis invoked regardless of theoriginTokenvalue:batchNonce, err := k.BuildOutgoingTxBatch(ctx, from, receipt, amount, fee) if err != nil { return err }However,
batchNonceis only used when!originToken:if !originToken { k.erc20Keeper.SetOutgoingTransferRelation(ctx, fxTarget.GetTarget(), batchNonce) }Please verify whether it's necessary to build the outgoing transaction batch when
originTokenistrue, or if the call toBuildOutgoingTxBatchshould be conditioned on!originTokento avoid unnecessary processing.
237-259:⚠️ Potential issueUnused 'originTokenAmount' parameter in non-IBC transfers
In the
BridgeCallBaseCoinfunction, the parameteroriginTokenAmountis only used in the IBC transfer path (fxTarget.IsIBC()istrue), where it's checked usingoriginTokenAmount.IsZero(). However, in the non-IBC transfer path (fxTarget.IsIBC()isfalse), this parameter is not utilized, and there's a TODO comment acknowledging this:// todo record origin amountPlease ensure that
originTokenAmountis appropriately handled in the non-IBC transfer scenario to avoid potential issues or confusion.app/keepers/keepers.go (1)
341-341: EnsureprecompileRouteris initialized before useThe
precompileRouteris being passed tocrosschainprecompile.NewPrecompiledContract. Ensure thatprecompileRouteris properly initialized before this call to prevent any potential nil pointer dereferences at runtime.Run the following script to verify that
precompileRouteris initialized before use:✅ Verification successful
precompileRouterInitialization ConfirmedThe
precompileRouteris properly initialized at line 326 before its usage at line 341, preventing any potential nil pointer dereferences.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `precompileRouter` is initialized before being used in NewPrecompiledContract # Expected: Initialization of precompileRouter occurs before its usage rg -n 'precompileRouter\s*:=\s*crosschainprecompile\.NewRouter\(\)' app/keepers/keepers.goLength of output: 150
Summary by CodeRabbit
New Features
TotalSupplymethod for querying total ERC20 token supply.Bug Fixes
BridgeCoinByDenommethod for improved efficiency.Testing Enhancements
Deprecations
CrossChainMethodstruct in favor ofBridgeCallMethod.