-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: crosschain precmpile contract #579
Conversation
WalkthroughThe recent changes introduce a significant refactor to the precompile methods in the cross-chain package of the Cosmos SDK. A new interface, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PrecompileContract
participant BridgeCallMethod
participant CrossChainMethod
User ->> PrecompileContract: Initiate transaction
PrecompileContract ->> BridgeCallMethod: Execute BridgeCall
BridgeCallMethod -->> PrecompileContract: Return results
PrecompileContract ->> CrossChainMethod: Process CrossChain
CrossChainMethod -->> PrecompileContract: Return confirmation
PrecompileContract -->> User: Transaction complete
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
Outside diff range and nitpick comments (5)
x/crosschain/types/contract.go (4)
Line range hint
35-41
: Improve Error Handling ConsistencyThe error message for an invalid transaction ID in
CancelSendToExternalArgs
is good, but consider using similar descriptive error messages for other validation failures across the file for consistency.- return errors.New("invalid tx id") + return errors.New("transaction ID must be positive")
Line range hint
67-73
: Standardize Error Messages for ConsistencyThe error handling in
CrossChainArgs
validation should be consistent with other validations in terms of message clarity and format.- return errors.New("empty receipt") + return errors.New("receipt field must not be empty") - return errors.New("invalid amount") + return errors.New("amount must be positive") - return errors.New("invalid fee") + return errors.New("fee must be non-negative")
Line range hint
83-89
: Clarify Error Messages inIncreaseBridgeFeeArgs
ValidationThe error messages in
IncreaseBridgeFeeArgs
are inconsistent with other parts of the file. Using clear and consistent error messaging helps maintain the code's readability and maintainability.- return errors.New("invalid tx id") + return errors.New("transaction ID must be positive") - return errors.New("invalid add bridge fee") + return errors.New("bridge fee must be positive")
Line range hint
97-103
: Validate Array Lengths inBridgeCallArgs
The validation in
BridgeCallArgs
is crucial for the integrity of the operation. It might be beneficial to add checks for the lengths of theTokens
andAmounts
arrays to ensure they are not excessively large which could lead to performance issues.+ if len(args.Tokens) > 100 { + return errors.New("tokens array size exceeds maximum limit") + } + if len(args.Amounts) > 100 { + return errors.New("amounts array size exceeds maximum limit") + }x/crosschain/precompile/crosschain.go (1)
Line range hint
46-96
: Refactor Complex Method for Better ReadabilityThe
Run
method inCrossChainMethod
is complex and handles multiple scenarios. Consider refactoring it to improve readability and maintainability.- // Original complex method + // Refactored method with helper functions
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- contract/precompile.go (1 hunks)
- tests/precompile_suite.go (3 hunks)
- x/crosschain/legacy/precompile_events.go (1 hunks)
- x/crosschain/precompile/bridge_call.go (4 hunks)
- x/crosschain/precompile/bridge_call_test.go (3 hunks)
- x/crosschain/precompile/bridge_coin_amount.go (1 hunks)
- x/crosschain/precompile/bridge_coin_amount_test.go (1 hunks)
- x/crosschain/precompile/cancel_send_to_external.go (1 hunks)
- x/crosschain/precompile/cancel_send_to_external_test.go (4 hunks)
- x/crosschain/precompile/contract.go (3 hunks)
- x/crosschain/precompile/contract_test.go (2 hunks)
- x/crosschain/precompile/crosschain.go (2 hunks)
- x/crosschain/precompile/crosschain_test.go (4 hunks)
- x/crosschain/precompile/fip20_crosschain.go (1 hunks)
- x/crosschain/precompile/fip20crosschain_test.go (8 hunks)
- x/crosschain/precompile/increase_bridge_fee.go (1 hunks)
- x/crosschain/precompile/increase_bridgefee_test.go (5 hunks)
- x/crosschain/precompile/keeper.go (1 hunks)
- x/crosschain/types/contract.go (7 hunks)
- x/crosschain/types/events.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/crosschain/precompile/contract_test.go
- x/crosschain/types/events.go
Additional comments not posted (25)
contract/precompile.go (1)
8-13
: Well-defined interface for precompiled contractsThe
PrecompileMethod
interface is correctly defined with appropriate methods for handling precompiled contracts in the Cosmos SDK using EVM. This should facilitate better modularization and reusability of contract methods.x/crosschain/precompile/contract.go (3)
17-18
: RefactoredContract
struct with improved modularityThe
Contract
struct now correctly uses a slice ofPrecompileMethod
, enhancing modularity and simplifying method management. This is a positive change in line with best practices for maintainability.
61-67
: Efficient gas calculation methodThe
RequiredGas
method efficiently determines the gas required by comparing method IDs. This implementation is clear and makes good use of the modular design of precompiled methods.
75-94
: Robust contract execution method with proper state managementThe
Run
method in theContract
class is well-implemented with excellent error handling and state management. Using snapshots for state rollback on errors ensures transactional integrity, which is crucial for contract execution.x/crosschain/precompile/bridge_call.go (2)
16-20
: Well-structuredBridgeCallMethod
classThe
BridgeCallMethod
class is well-structured, encapsulating the ABI method and event necessary for bridge calls. This encapsulation enhances modularity and clarity in the contract's functionality.
Line range hint
42-108
: Robust implementation of bridge call executionThe
Run
method in theBridgeCallMethod
class is implemented robustly, with detailed error handling and resource management. The method's structure facilitates clear understanding and maintenance.x/crosschain/precompile/bridge_coin_amount_test.go (1)
20-24
: Correct ABI method testingThe test
TestBridgeCoinAmountMethod_ABI
correctly checks the number of inputs and outputs for the ABI method, ensuring it meets the expected specifications. This is essential for ensuring the method's interface is correctly implemented.x/crosschain/types/contract.go (1)
Line range hint
51-57
: Refine Error Messages for Better ClarityThe error messages in
FIP20CrossChainArgs
validation could be more specific. This will help in understanding exactly what went wrong without needing to delve into the code.
[ISSE]- return errors.New("empty receipt") + return errors.New("receipt field must not be empty") - return errors.New("invalid amount") + return errors.New("amount must be positive") - return errors.New("invalid fee") + return errors.New("fee must be non-negative")x/crosschain/precompile/increase_bridge_fee.go (3)
20-24
: Struct definition for IncreaseBridgeFeeMethod: Looks good.
The structIncreaseBridgeFeeMethod
is well-defined and properly encapsulates the necessary components (Keeper
,Method
, andEvent
from the ABI).
26-31
: Constructor function for IncreaseBridgeFeeMethod: Properly initializes struct.
The constructor functionNewIncreaseBridgeFeeMethod
initializes theIncreaseBridgeFeeMethod
struct with appropriate ABI methods and events. This is crucial for ensuring that the correct ABI definitions are used during runtime.
34-36
: Method implementations for interface compliance: Correct and efficient.
The implementations ofIsReadonly
,GetMethodId
, andRequiredGas
are straightforward and comply with the expected interface contracts. These methods ensure that the contract's metadata is accessible and correctly used.Also applies to: 38-40, 42-44
x/crosschain/precompile/fip20_crosschain.go (3)
18-21
: Struct definition for FIP20CrossChainMethod: Well-structured.
TheFIP20CrossChainMethod
struct extendsCrossChainMethod
and includes an additionalMethod
field. This design leverages inheritance to reuse functionality and extend capabilities, which is a good OOP practice.
23-27
: Constructor function for FIP20CrossChainMethod: Correctly initializes struct.
The constructor function properly initializes the base struct and sets up the ABI method. This ensures that the method-specific ABI is correctly linked.
30-32
: Simple method implementations: Correctly implemented.
The methodsIsReadonly
,GetMethodId
, andRequiredGas
are implemented correctly, providing necessary contract metadata efficiently.Also applies to: 34-36, 38-40
x/crosschain/precompile/keeper.go (1)
23-29
: Keeper struct definition: Appropriately structured.
TheKeeper
struct is well-defined with fields that are essential for handling various aspects of cross-chain operations, such as routing, token management, and account management.tests/precompile_suite.go (1)
138-138
: New test methods for CancelSendToExternal and IncreaseBridgeFee: Properly implemented.
The new test methods forCancelSendToExternal
andIncreaseBridgeShee
are correctly implemented and use the newly introduced precompile methods. These tests are essential for ensuring that the new functionality works as expected.Also applies to: 172-172
x/crosschain/precompile/cancel_send_to_external_test.go (3)
32-35
: Good validation of ABI configurations.The assertions correctly validate the expected number of inputs and outputs for both the method and the associated event, ensuring that the ABI is configured as expected.
447-451
: Correct event processing in test cases.The test correctly identifies and processes the
CancelSendToExternal
event by matching the event ID and unpacking the event data. This ensures that the event is being emitted correctly and that the data is consistent with the expected format.
1-1
: Review the package and import statements.The import statements are correctly organized and relevant to the test scenarios being implemented. This setup ensures that all necessary modules and dependencies are available for the tests.
x/crosschain/precompile/increase_bridgefee_test.go (2)
23-23
: Refactor import to consolidate.Consider consolidating the import statements for better organization and to follow Go conventions.
- "github.com/functionx/fx-core/v7/x/crosschain/precompile" - crosschaintypes "github.com/functionx/fx-core/v7/x/crosschain/types" + "github.com/functionx/fx-core/v7/x/crosschain/precompile" + "github.com/functionx/fx-core/v7/x/crosschain/types"
486-491
: Verify event data extraction and handling.Ensure that the event data extraction from logs is correctly implemented. This code assumes that the log data structure matches expected formats, which should be validated in test scenarios.
Verification successful
Verify event data extraction and handling.
The broader search for patterns related to event data extraction and handling indicates that the
event.Inputs.NonIndexed().Unpack
method is used consistently across multiple test files. The usage appears to be consistent and includes error handling.
x/crosschain/precompile/increase_bridgefee_test.go
x/crosschain/precompile/crosschain_test.go
x/crosschain/precompile/cancel_send_to_external_test.go
x/evm/precompiles/staking/approve_shares_test.go
x/evm/precompiles/staking/delegation_test.go
x/evm/precompiles/staking/undelegate_test.go
x/evm/precompiles/staking/redelegate_test.go
x/evm/precompiles/staking/delegate_test.go
x/evm/precompiles/staking/allowance_shares_test.go
x/evm/precompiles/staking/withdraw_test.go
x/evm/precompiles/staking/delegation_rewards_test.go
x/evm/precompiles/staking/transfer_shares_test.go
Given this, the implementation in the snippet provided appears to be consistent with the rest of the codebase and includes appropriate error handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm the log data structure matches expected formats. # Test: Search for the usage of log data extraction in the entire codebase. Expect: Consistent usage and error handling. rg --type go 'event.Inputs.NonIndexed().Unpack'Length of output: 49
Script:
#!/bin/bash # Description: Confirm the log data structure matches expected formats by searching for broader patterns related to event data extraction and handling. # Test: Search for the usage of `event.Inputs` in the entire codebase. Expect: Consistent usage and error handling. rg --type go 'event.Inputs' # Test: Search for the usage of `Unpack` method in the entire codebase. Expect: Consistent usage and error handling. rg --type go 'Unpack'Length of output: 36145
x/crosschain/precompile/fip20crosschain_test.go (2)
34-34
: Check initialization offip20CrossChain
The
fip20CrossChain
is initialized without a specific keeper, which might be intentional to test default behavior or error handling. Ensure that this is the expected setup for these tests.Verification successful
Check initialization of
fip20CrossChain
The
fip20CrossChain
is intentionally initialized withnil
for testing purposes. This pattern appears consistent with the intended behavior for these test cases.
x/crosschain/precompile/fip20crosschain_test.go
:fip20CrossChain := precompile.NewFIP20CrossChainMethod(nil)
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the initialization context for `fip20CrossChain`. # Test: Search for initialization patterns. Expect: Intentional use of nil keeper. rg --type go $'NewFIP20CrossChainMethod'Length of output: 336
Line range hint
1272-1285
: Complex ABI manipulation needs careful reviewManipulating ABI data directly is error-prone and should be handled with care. Ensure that the ABI methods and data manipulation are correctly implemented to avoid runtime errors.
x/crosschain/precompile/crosschain_test.go (2)
27-27
: Import Optimization: Consider grouping imports for clarity.The imports are generally well-organized, but grouping them by their source (standard library, third-party, internal) could enhance readability.
import ( "encoding/hex" "fmt" "math/big" "strings" "testing" sdkmath "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" ibctransfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" ibcchanneltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" "github.com/ethereum/go-ethereum/common" evmtypes "github.com/evmos/ethermint/x/evm/types" "github.com/stretchr/testify/require" tmrand "github.com/tendermint/tendermint/libs/rand" "golang.org/x/exp/slices" "github.com/functionx/fx-core/v7/contract" testcontract "github.com/functionx/fx-core/v7/tests/contract" "github.com/functionx/fx-core/v7/testutil/helpers" fxtypes "github.com/functionx/fx-core/v7/types" bsctypes "github.com/functionx/fx-core/v7/x/bsc/types" crosschainkeeper "github.com/functionx/fx-core/v7/x/crosschain/keeper" "github.com/functionx/fx-core/v7/x/crosschain/precompile" crosschaintypes "github.com/functionx/fx-core/v7/x/crosschain/types" "github.com/functionx/fx-core/v7/x/erc20/types" ethtypes "github.com/functionx/fx-core/v7/x/eth/types" )
36-37
: Unit Test Assertions:The assertions in the test
TestCrossChainABI
are clear and check the expected structure of method inputs and outputs. Ensure that these assertions remain aligned with any future changes to the method signatures.Also applies to: 39-39
crosschaintypes "github.com/functionx/fx-core/v7/x/crosschain/types" | ||
"github.com/functionx/fx-core/v7/x/erc20/types" | ||
ethtypes "github.com/functionx/fx-core/v7/x/eth/types" | ||
) | ||
|
||
func TestCrossChainABI(t *testing.T) { | ||
crossChainABI := crosschaintypes.GetABI() | ||
crossChain := precompile.NewCrossChainMethod(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure proper error handling for nil input.
The NewCrossChainMethod
function does not handle nil
inputs for the keeper
parameter. It directly assigns the keeper
to the Keeper
field of the CrossChainMethod
struct without any checks. Consider adding a nil
check to ensure the function handles nil
inputs safely.
- File:
x/crosschain/precompile/crosschain.go
- Lines: 25-31
Analysis chain
Ensure proper error handling for nil input.
The NewCrossChainMethod
function is invoked with a nil
argument. Ensure that the function is designed to handle nil
inputs safely or consider handling potential nil
input scenarios if not already done.
The NewCrossChainMethod
function assigns its keeper
parameter directly to the Keeper
field of the CrossChainMethod
struct without any nil
checks. To confirm this observation, we should inspect the complete implementation of the function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the function `NewCrossChainMethod` to verify it handles nil inputs safely.
ast-grep --lang go --pattern $'func NewCrossChainMethod($_) *CrossChainMethod {
$$$
}'
Length of output: 616
Script:
#!/bin/bash
# Description: Print the complete implementation of the `NewCrossChainMethod` function to confirm if it handles nil inputs.
ast-grep --lang go --pattern $'func NewCrossChainMethod(keeper *Keeper) *CrossChainMethod {
$$$
}' --json
Length of output: 898
crosschaintypes "github.com/functionx/fx-core/v7/x/crosschain/types" | ||
"github.com/functionx/fx-core/v7/x/erc20/types" | ||
ethtypes "github.com/functionx/fx-core/v7/x/eth/types" | ||
) | ||
|
||
func TestCancelSendToExternalABI(t *testing.T) { | ||
crossChainABI := crosschaintypes.GetABI() | ||
cancelSendToExternal := precompile.NewCancelSendToExternalMethod(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure proper initialization of the CancelSendToExternalMethod
struct in tests/precompile_suite.go
.
The CancelSendToExternalMethod
is instantiated with a nil
keeper in the CancelSendToExternal
method, which could lead to nil pointer dereferences if methods accessing the keeper are called.
tests/precompile_suite.go
, lines around the instantiation ofNewCancelSendToExternalMethod(nil)
Analysis chain
Ensure proper initialization of the CancelSendToExternalMethod
struct.
The CancelSendToExternalMethod
is instantiated with a nil
keeper, which might be intentional for isolated ABI testing but could lead to nil pointer dereferences if methods accessing the keeper are called.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `CancelSendToExternalMethod` is always properly initialized in all test cases.
# Test: Search for improper initializations. Expect: No improper initializations.
rg --type go $'NewCancelSendToExternalMethod\(nil\)'
Length of output: 308
Script:
#!/bin/bash
# Extract and display the relevant sections of code from both files to analyze the context of `NewCancelSendToExternalMethod(nil)`
# Extract 10 lines before and after the match in `cancel_send_to_external_test.go`
rg --type go -A 10 -B 10 'NewCancelSendToExternalMethod\(nil\)' x/crosschain/precompile/cancel_send_to_external_test.go
# Extract 10 lines before and after the match in `tests/precompile_suite.go`
rg --type go -A 10 -B 10 'NewCancelSendToExternalMethod\(nil\)' tests/precompile_suite.go
Length of output: 2207
increaseBridgeFee := precompile.NewIncreaseBridgeFeeMethod(nil) | ||
|
||
method := crossChainABI.Methods[crosschaintypes.IncreaseBridgeFeeMethodName] | ||
require.Equal(t, method, crosschaintypes.IncreaseBridgeFeeMethod) | ||
require.Equal(t, 4, len(method.Inputs)) | ||
require.Equal(t, 1, len(method.Outputs)) | ||
require.Equal(t, 4, len(increaseBridgeFee.Method.Inputs)) | ||
require.Equal(t, 1, len(increaseBridgeFee.Method.Outputs)) | ||
|
||
event := crossChainABI.Events[crosschaintypes.IncreaseBridgeFeeEventName] | ||
require.Equal(t, event, crosschaintypes.IncreaseBridgeFeeEvent) | ||
require.Equal(t, 5, len(event.Inputs)) | ||
require.Equal(t, 5, len(increaseBridgeFee.Event.Inputs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper test setup and teardown.
This test function lacks setup and teardown logic, which might be necessary for initializing or cleaning up resources before and after tests. Consider using t.Cleanup()
for any setup done.
func TestIncreaseBridgeFeeABI(t *testing.T) {
t.Cleanup(func() {
// cleanup code here
})
// existing test code
}
event := crosschaintypes.GetABI().Events["IncreaseBridgeFee"] | ||
if log.Topics[0] == event.ID.String() { | ||
suite.Require().Equal(log.Address, crosschaintypes.GetAddress().String()) | ||
suite.Require().Equal(log.Topics[1], signer.Address().Hash().String()) | ||
suite.Require().Equal(log.Topics[2], pair.GetERC20Contract().Hash().String()) | ||
unpack, err := crosschaintypes.IncreaseBridgeFeeEvent.Inputs.NonIndexed().Unpack(log.Data) | ||
unpack, err := event.Inputs.NonIndexed().Unpack(log.Data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check correctness of event log processing.
The processing of event logs should include error handling for cases where the log format does not meet expectations or when the unpacking fails.
- unpack, err := event.Inputs.NonIndexed().Unpack(log.Data)
- suite.Require().NoError(err)
+ unpack, err := event.Inputs.NonIndexed().Unpack(log.Data)
+ suite.Require().NoError(err, "Failed to unpack log 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.
event := crosschaintypes.GetABI().Events["IncreaseBridgeFee"] | |
if log.Topics[0] == event.ID.String() { | |
suite.Require().Equal(log.Address, crosschaintypes.GetAddress().String()) | |
suite.Require().Equal(log.Topics[1], signer.Address().Hash().String()) | |
suite.Require().Equal(log.Topics[2], pair.GetERC20Contract().Hash().String()) | |
unpack, err := crosschaintypes.IncreaseBridgeFeeEvent.Inputs.NonIndexed().Unpack(log.Data) | |
unpack, err := event.Inputs.NonIndexed().Unpack(log.Data) | |
event := crosschaintypes.GetABI().Events["IncreaseBridgeFee"] | |
if log.Topics[0] == event.ID.String() { | |
suite.Require().Equal(log.Address, crosschaintypes.GetAddress().String()) | |
suite.Require().Equal(log.Topics[1], signer.Address().Hash().String()) | |
suite.Require().Equal(log.Topics[2], pair.GetERC20Contract().Hash().String()) | |
unpack, err := event.Inputs.NonIndexed().Unpack(log.Data) | |
suite.Require().NoError(err, "Failed to unpack log data") |
@@ -1250,7 +1247,7 @@ func (suite *PrecompileTestSuite) TestAccountFIP20CrossChain() { | |||
name: "failed - call with address - pair not found", | |||
malleate: func(pair *types.TokenPair, md Metadata, signer *helpers.Signer, randMint *big.Int) ([]byte, []string) { | |||
data, err := crossChainABI.Pack( | |||
crosschaintypes.FIP20CrossChainMethod.Name, | |||
"fip20CrossChain", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issue with error message consistency
The error message for a failed test case might not be consistent with the actual error thrown by the system. It's important to ensure that the error messages are accurate to avoid confusion during debugging.
- return fmt.Sprintf("token pair not found: %s", args[0])
+ return fmt.Sprintf("execution reverted: token pair not found: %s", args[0])
Committable suggestion was skipped due to low confidence.
fip20CrossChain := precompile.NewFIP20CrossChainMethod(nil) | ||
|
||
method := crossChainABI.Methods[crosschaintypes.FIP20CrossChainMethod.Name] | ||
require.Equal(t, method, crosschaintypes.FIP20CrossChainMethod) | ||
require.Equal(t, 6, len(method.Inputs)) | ||
require.Equal(t, 1, len(method.Outputs)) | ||
require.Equal(t, 6, len(fip20CrossChain.Method.Inputs)) | ||
require.Equal(t, 1, len(fip20CrossChain.Method.Outputs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure proper test setup for TestFIP20CrossChainABI
The test checks the number of inputs and outputs for the FIP20CrossChain
ABI. However, it initializes fip20CrossChain
with nil
as the keeper, which is not handled internally and could lead to null pointer exceptions or unintended behavior. Please ensure the keeper is properly initialized for this test scenario.
fip20_crosschain.go
:NewFIP20CrossChainMethod
directly uses thekeeper
parameter without null checks.
crosschain.go
:NewCrossChainMethod
also directly uses thekeeper
parameter without null checks.
Analysis chain
Ensure proper test setup for TestFIP20CrossChainABI
The test checks the number of inputs and outputs for the FIP20CrossChain
ABI. However, it initializes fip20CrossChain
with nil
as the keeper, which might not be intended unless handled internally. Please confirm if the keeper is supposed to be nil for this test scenario.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the nil keeper is handled internally without causing null pointer exceptions.
# Test: Search for the function usage. Expect: Handling of nil keeper.
rg --type go $'NewFIP20CrossChainMethod'
Length of output: 336
Script:
#!/bin/bash
# Extract the implementation of NewFIP20CrossChainMethod from fip20_crosschain.go
ast-grep --lang go --pattern $'func NewFIP20CrossChainMethod($_) $_'
Length of output: 593
Script:
#!/bin/bash
# Extract the implementation of NewCrossChainMethod from the codebase to check for nil handling of the keeper.
ast-grep --lang go --pattern $'func NewCrossChainMethod($_) $_'
Length of output: 591
func (m *IncreaseBridgeFeeMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | ||
if m.router == nil { | ||
return nil, errors.New("cross chain router empty") | ||
} | ||
|
||
args, err := m.UnpackInput(contract.Input) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
fxTarget := fxtypes.ParseFxTarget(args.Chain) | ||
route, has := m.router.GetRoute(fxTarget.GetTarget()) | ||
if !has { | ||
return nil, fmt.Errorf("chain not support: %s", args.Chain) | ||
} | ||
|
||
value := contract.Value() | ||
sender := contract.Caller() | ||
totalCoin := sdk.Coin{} | ||
if value.Cmp(big.NewInt(0)) == 1 && fxcontract.IsZeroEthAddress(args.Token) { | ||
if args.Fee.Cmp(value) != 0 { | ||
return nil, errors.New("add bridge fee not equal msg.value") | ||
} | ||
totalCoin, err = m.handlerOriginToken(ctx, evm, sender, args.Fee) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
totalCoin, err = m.handlerERC20Token(ctx, evm, sender, args.Token, args.Fee) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
// convert token to bridge fee token | ||
feeCoin := sdk.NewCoin(totalCoin.Denom, sdkmath.NewIntFromBigInt(args.Fee)) | ||
addBridgeFee, err := m.erc20Keeper.ConvertDenomToTarget(ctx, sender.Bytes(), feeCoin, fxTarget) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if err = route.PrecompileIncreaseBridgeFee(ctx, args.TxID.Uint64(), sender.Bytes(), addBridgeFee); err != nil { | ||
return nil, err | ||
} | ||
|
||
data, topic, err := m.NewIncreaseBridgeFeeEvent(sender, args.Token, args.Chain, args.TxID, args.Fee) | ||
if err != nil { | ||
return nil, err | ||
} | ||
EmitEvent(evm, data, topic) | ||
|
||
return m.PackOutput(true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core logic in Run method: Several issues detected.
- Error Handling: The method correctly checks for null router and missing chain support, which is good for robustness.
- Token Handling Logic: The logic for handling ERC20 tokens and native tokens (lines 65-78) is complex and should be encapsulated in separate methods to improve readability and maintainability.
- Event Emission: The event emission (line 95) is done without checking the success of previous steps, which could lead to incorrect event states if earlier operations fail.
[REFACTOR_SUGGESTion]
+ func (m *IncreaseBridgeFeeMethod) handleTokens(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract, args *crosschaintypes.IncreaseBridgeFeeArgs) (sdk.Coin, error) {
+ // existing token handling logic here
+ }
+ // Use the new function in the Run method
Ensure all operations are successful before emitting events to prevent state inconsistencies.
Committable suggestion was skipped due to low confidence.
func (m *FIP20CrossChainMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | ||
tokenContract := contract.Caller() | ||
tokenPair, found := m.erc20Keeper.GetTokenPairByAddress(ctx, tokenContract) | ||
if !found { | ||
return nil, fmt.Errorf("token pair not found: %s", tokenContract.String()) | ||
} | ||
|
||
args, err := m.UnpackInput(contract.Input) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
amountCoin := sdk.NewCoin(tokenPair.GetDenom(), sdkmath.NewIntFromBigInt(args.Amount)) | ||
feeCoin := sdk.NewCoin(tokenPair.GetDenom(), sdkmath.NewIntFromBigInt(args.Fee)) | ||
totalCoin := sdk.NewCoin(tokenPair.GetDenom(), amountCoin.Amount.Add(feeCoin.Amount)) | ||
|
||
// NOTE: if user call evm denom transferCrossChain with msg.value | ||
// we need transfer msg.value from sender to contract in bank keeper | ||
if tokenPair.GetDenom() == fxtypes.DefaultDenom { | ||
balance := m.bankKeeper.GetBalance(ctx, tokenContract.Bytes(), fxtypes.DefaultDenom) | ||
evmBalance := evm.StateDB.GetBalance(tokenContract) | ||
|
||
cmp := evmBalance.Cmp(balance.Amount.BigInt()) | ||
if cmp == -1 { | ||
return nil, fmt.Errorf("invalid balance(chain: %s,evm: %s)", balance.Amount.String(), evmBalance.String()) | ||
} | ||
if cmp == 1 { | ||
// sender call transferCrossChain with msg.value, the msg.value evm denom should send to contract | ||
value := big.NewInt(0).Sub(evmBalance, balance.Amount.BigInt()) | ||
valueCoin := sdk.NewCoins(sdk.NewCoin(fxtypes.DefaultDenom, sdkmath.NewIntFromBigInt(value))) | ||
if err := m.bankKeeper.SendCoins(ctx, args.Sender.Bytes(), tokenContract.Bytes(), valueCoin); err != nil { | ||
return nil, fmt.Errorf("send coin: %s", err.Error()) | ||
} | ||
} | ||
} | ||
|
||
// transfer token from evm to local chain | ||
if err = m.convertERC20(ctx, evm, tokenPair, totalCoin, args.Sender); err != nil { | ||
return nil, err | ||
} | ||
|
||
fxTarget := fxtypes.ParseFxTarget(fxtypes.Byte32ToString(args.Target)) | ||
if err = m.handlerCrossChain(ctx, args.Sender.Bytes(), args.Receipt, amountCoin, feeCoin, fxTarget, args.Memo, false); err != nil { | ||
return nil, err | ||
} | ||
|
||
data, topic, err := m.NewCrossChainEvent(args.Sender, tokenPair.GetERC20Contract(), tokenPair.GetDenom(), args.Receipt, args.Amount, args.Fee, args.Target, args.Memo) | ||
if err != nil { | ||
return nil, err | ||
} | ||
EmitEvent(evm, data, topic) | ||
|
||
legacy.Fip20CrossChainEvents(ctx, args.Sender, tokenPair.GetERC20Contract(), args.Receipt, | ||
fxtypes.Byte32ToString(args.Target), tokenPair.GetDenom(), args.Amount, args.Fee) | ||
|
||
return m.PackOutput(true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complex logic in Run method: Needs refactoring and error handling improvements.
The Run
method contains complex logic for handling token transfers and cross-chain operations. The error handling is adequate, but the method's length and complexity could be reduced by extracting some logic into helper methods.
[REFACTOR_SUGGESTion]
+ func (m *FIP20CrossChainMethod) handleTokenTransfer(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract, args *crosschaintypes.FIP20CrossChainArgs) error {
+ // Logic from lines 43-81
+ }
+ // Use the new function in the Run method
This refactoring will improve the readability and maintainability of the code.
Ensure that all operations are successful before proceeding to the next steps to avoid state inconsistencies.
Committable suggestion was skipped due to low confidence.
func CrossChainEvents(ctx sdk.Context, from, token common.Address, recipient, target, denom, memo string, amount, fee *big.Int) { | ||
ctx.EventManager().EmitEvent(sdk.NewEvent( | ||
EventTypeCrossChain, | ||
sdk.NewAttribute(AttributeKeyFrom, from.String()), | ||
sdk.NewAttribute(AttributeKeyRecipient, recipient), | ||
sdk.NewAttribute(sdk.AttributeKeyAmount, amount.String()), | ||
sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), | ||
sdk.NewAttribute(AttributeKeyTarget, target), | ||
sdk.NewAttribute(AttributeKeyTokenAddress, token.String()), | ||
sdk.NewAttribute(AttributeKeyDenom, denom), | ||
sdk.NewAttribute(AttributeKeyMemo, memo), | ||
)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Deprecated function check
The CrossChainEvents
function is still in use within x/crosschain/precompile/crosschain.go
. Therefore, it cannot be removed or archived at this time. Consider marking the function as deprecated and updating the documentation to reflect its status.
x/crosschain/precompile/crosschain.go
:legacy.CrossChainEvents(ctx, sender, args.Token, args.Receipt, fxtypes.Byte32ToString(args.Target), ...
Analysis chain
Deprecated function check
The CrossChainEvents
function is also deprecated. Verify its usage across the codebase and consider removing or archiving if it is no longer needed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the deprecated function `CrossChainEvents` is not used elsewhere in the codebase.
# Test: Search for the function usage. Expect: No occurrences outside this file.
rg --type go $'CrossChainEvents'
Length of output: 800
|
||
var ( | ||
// BridgeCoinAmountMethod query the amount of bridge coin | ||
BridgeCoinAmountMethod = GetABI().Methods[BridgeCoinAmountMethodName] | ||
|
||
// CancelSendToExternalMethod cancel send to external tx | ||
CancelSendToExternalMethod = GetABI().Methods[CancelSendToExternalMethodName] | ||
|
||
// FIP20CrossChainMethod cross chain with FIP20 token, only for FIP20 token | ||
// Deprecated: use CrossChainMethod instead | ||
FIP20CrossChainMethod = GetABI().Methods[FIP20CrossChainMethodName] | ||
|
||
// CrossChainMethod cross chain with FIP20 token | ||
CrossChainMethod = GetABI().Methods[CrossChainMethodName] | ||
|
||
// IncreaseBridgeFeeMethod increase bridge fee | ||
IncreaseBridgeFeeMethod = GetABI().Methods[IncreaseBridgeFeeMethodName] | ||
|
||
// BridgeCallMethod bridge call other chain | ||
BridgeCallMethod = GetABI().Methods[BridgeCallMethodName] | ||
) | ||
|
||
type BridgeCoinAmountArgs struct { | ||
Token common.Address `abi:"_token"` | ||
Target [32]byte `abi:"_target"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure Consistent Error Messaging
For the BridgeCoinAmountArgs
validation, consider using a more descriptive error message than "empty target" to clarify what is expected from the user.
- return errors.New("empty target")
+ return errors.New("target field must not be empty")
Committable suggestion was skipped due to low confidence.
func (m *CancelSendToExternalMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | ||
if m.router == nil { | ||
return nil, errors.New("cross chain router is empty") | ||
} | ||
|
||
args, err := m.UnpackInput(contract.Input) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
sender := contract.Caller() | ||
route, has := m.router.GetRoute(args.Chain) | ||
if !has { | ||
return nil, fmt.Errorf("chain not support: %s", args.Chain) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle Potential Null Pointer Exception
There is a potential null pointer dereference on m.router
which is accessed without checking if it's null. This could lead to runtime panics.
+ if m == nil || m.router == nil {
+ return nil, errors.New("method or router is not initialized")
+ }
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 (m *CancelSendToExternalMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | |
if m.router == nil { | |
return nil, errors.New("cross chain router is empty") | |
} | |
args, err := m.UnpackInput(contract.Input) | |
if err != nil { | |
return nil, err | |
} | |
sender := contract.Caller() | |
route, has := m.router.GetRoute(args.Chain) | |
if !has { | |
return nil, fmt.Errorf("chain not support: %s", args.Chain) | |
} | |
func (m *CancelSendToExternalMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | |
if m == nil || m.router == nil { | |
return nil, errors.New("method or router is not initialized") | |
} | |
args, err := m.UnpackInput(contract.Input) | |
if err != nil { | |
return nil, err | |
} | |
sender := contract.Caller() | |
route, has := m.router.GetRoute(args.Chain) | |
if !has { | |
return nil, fmt.Errorf("chain not support: %s", args.Chain) | |
} |
// NOTE: must be get relation before cancel, cancel will delete it if relation exist | ||
hasRelation := m.erc20Keeper.HasOutgoingTransferRelation(ctx, args.Chain, args.TxID.Uint64()) | ||
|
||
refundCoin, err := route.PrecompileCancelSendToExternal(ctx, args.TxID.Uint64(), sender.Bytes()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if !hasRelation && refundCoin.Denom == fxtypes.DefaultDenom { | ||
// add refund to sender in evm state db, because bank keeper add refund to sender | ||
evm.StateDB.AddBalance(sender, refundCoin.Amount.BigInt()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimize Condition Check and Error Handling
The check for hasRelation
and the subsequent operations can be optimized by handling the negative case first to reduce nesting and improve readability.
- if !hasRelation && refundCoin.Denom == fxtypes.DefaultDenom {
+ if hasRelation {
+ return nil, errors.New("relation already exists")
+ }
+ if refundCoin.Denom != fxtypes.DefaultDenom {
+ return nil, fmt.Errorf("unexpected denomination: %s", refundCoin.Denom)
+ }
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.
// NOTE: must be get relation before cancel, cancel will delete it if relation exist | |
hasRelation := m.erc20Keeper.HasOutgoingTransferRelation(ctx, args.Chain, args.TxID.Uint64()) | |
refundCoin, err := route.PrecompileCancelSendToExternal(ctx, args.TxID.Uint64(), sender.Bytes()) | |
if err != nil { | |
return nil, err | |
} | |
if !hasRelation && refundCoin.Denom == fxtypes.DefaultDenom { | |
// add refund to sender in evm state db, because bank keeper add refund to sender | |
evm.StateDB.AddBalance(sender, refundCoin.Amount.BigInt()) | |
} | |
// NOTE: must be get relation before cancel, cancel will delete it if relation exist | |
hasRelation := m.erc20Keeper.HasOutgoingTransferRelation(ctx, args.Chain, args.TxID.Uint64()) | |
if hasRelation { | |
return nil, errors.New("relation already exists") | |
} | |
refundCoin, err := route.PrecompileCancelSendToExternal(ctx, args.TxID.Uint64(), sender.Bytes()) | |
if err != nil { | |
return nil, err | |
} | |
if refundCoin.Denom != fxtypes.DefaultDenom { | |
return nil, fmt.Errorf("unexpected denomination: %s", refundCoin.Denom) | |
} | |
// add refund to sender in evm state db, because bank keeper add refund to sender | |
evm.StateDB.AddBalance(sender, refundCoin.Amount.BigInt()) |
func TestBridgeCallABI(t *testing.T) { | ||
bridgeCall := precompile.NewBridgeCallMethod(nil) | ||
|
||
require.Equal(t, 8, len(bridgeCall.Method.Inputs)) | ||
require.Equal(t, 1, len(bridgeCall.Method.Outputs)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve Test Coverage
The tests for BridgeCallMethod
are basic and only check the count of inputs and outputs. Consider adding more comprehensive tests that cover different scenarios and edge cases.
+ func TestBridgeCallMethod_ErrorHandling(t *testing.T) {
+ _, err := bridgeCall.Run(ctx, nil, nil)
+ require.Error(t, err, "should handle nil contexts and contracts")
+ }
Committable suggestion was skipped due to low confidence.
func (m *BridgeCoinAmountMethod) Run(ctx sdk.Context, evm *vm.EVM, contract *vm.Contract) ([]byte, error) { | ||
args, err := m.UnpackInput(contract.Input) | ||
if err != nil { | ||
return nil, err | ||
} | ||
pair, has := c.erc20Keeper.GetTokenPair(cacheCtx, args.Token.Hex()) | ||
|
||
pair, has := m.erc20Keeper.GetTokenPair(ctx, args.Token.Hex()) | ||
if !has { | ||
return nil, fmt.Errorf("token not support: %s", args.Token.Hex()) | ||
} | ||
// FX | ||
if contract.IsZeroEthAddress(args.Token) { | ||
supply := c.bankKeeper.GetSupply(cacheCtx, fxtypes.DefaultDenom) | ||
balance := c.bankKeeper.GetBalance(cacheCtx, c.accountKeeper.GetModuleAddress(ethtypes.ModuleName), fxtypes.DefaultDenom) | ||
return crosschaintypes.BridgeCoinAmountMethod.Outputs.Pack(supply.Amount.Sub(balance.Amount).BigInt()) | ||
if fxcontract.IsZeroEthAddress(args.Token) { | ||
supply := m.bankKeeper.GetSupply(ctx, fxtypes.DefaultDenom) | ||
balance := m.bankKeeper.GetBalance(ctx, m.accountKeeper.GetModuleAddress(ethtypes.ModuleName), fxtypes.DefaultDenom) | ||
return m.PackOutput(supply.Amount.Sub(balance.Amount).BigInt()) | ||
} | ||
// OriginDenom | ||
if c.erc20Keeper.IsOriginDenom(cacheCtx, pair.GetDenom()) { | ||
erc20Call := contract.NewERC20Call(evm, c.Address(), args.Token, c.GetBlockGasLimit()) | ||
if m.erc20Keeper.IsOriginDenom(ctx, pair.GetDenom()) { | ||
erc20Call := fxcontract.NewERC20Call(evm, crosschaintypes.GetAddress(), args.Token, m.RequiredGas()) | ||
supply, err := erc20Call.TotalSupply() | ||
if err != nil { | ||
return nil, err | ||
} | ||
return crosschaintypes.BridgeCoinAmountMethod.Outputs.Pack(supply) | ||
return m.PackOutput(supply) | ||
} | ||
// one to one | ||
_, has = c.erc20Keeper.HasDenomAlias(cacheCtx, pair.GetDenom()) | ||
_, has = m.erc20Keeper.HasDenomAlias(ctx, pair.GetDenom()) | ||
if !has && pair.GetDenom() != fxtypes.DefaultDenom { | ||
return crosschaintypes.BridgeCoinAmountMethod.Outputs.Pack( | ||
c.bankKeeper.GetSupply(cacheCtx, pair.GetDenom()).Amount.BigInt(), | ||
return m.PackOutput( | ||
m.bankKeeper.GetSupply(ctx, pair.GetDenom()).Amount.BigInt(), | ||
) | ||
} | ||
// many to one | ||
md, has := c.bankKeeper.GetDenomMetaData(cacheCtx, pair.GetDenom()) | ||
md, has := m.bankKeeper.GetDenomMetaData(ctx, pair.GetDenom()) | ||
if !has { | ||
return nil, fmt.Errorf("denom not support: %s", pair.GetDenom()) | ||
} | ||
denom := c.erc20Keeper.ToTargetDenom( | ||
cacheCtx, | ||
denom := m.erc20Keeper.ToTargetDenom( | ||
ctx, | ||
pair.GetDenom(), | ||
md.GetBase(), | ||
md.GetDenomUnits()[0].GetAliases(), | ||
fxtypes.ParseFxTarget(fxtypes.Byte32ToString(args.Target)), | ||
) | ||
|
||
balance := c.bankKeeper.GetBalance(cacheCtx, c.erc20Keeper.ModuleAddress().Bytes(), pair.GetDenom()) | ||
supply := c.bankKeeper.GetSupply(cacheCtx, denom) | ||
balance := m.bankKeeper.GetBalance(ctx, m.erc20Keeper.ModuleAddress().Bytes(), pair.GetDenom()) | ||
supply := m.bankKeeper.GetSupply(ctx, denom) | ||
if balance.Amount.LT(supply.Amount) { | ||
return crosschaintypes.BridgeCoinAmountMethod.Outputs.Pack(balance.Amount.BigInt()) | ||
supply = balance | ||
} | ||
return m.PackOutput(supply.Amount.BigInt()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure Consistent Error Handling and Refactor for Clarity
The Run
method in BridgeCoinAmountMethod
has multiple return points and error checks. Consider refactoring for clarity and consistent error handling.
- // Original complex method
+ // Refactored method with consolidated error handling
Committable suggestion was skipped due to low confidence.
func Fip20CrossChainEvents(ctx sdk.Context, from, token common.Address, recipient, target, denom string, amount, fee *big.Int) { | ||
ctx.EventManager().EmitEvent(sdk.NewEvent( | ||
EventTypeRelayTransferCrossChain, | ||
sdk.NewAttribute(AttributeKeyFrom, from.String()), | ||
sdk.NewAttribute(AttributeKeyRecipient, recipient), | ||
sdk.NewAttribute(sdk.AttributeKeyAmount, amount.String()), | ||
sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), | ||
sdk.NewAttribute(AttributeKeyTarget, target), | ||
sdk.NewAttribute(AttributeKeyTokenAddress, token.String()), | ||
sdk.NewAttribute(AttributeKeyDenom, denom), | ||
)) | ||
|
||
telemetry.IncrCounterWithLabels( | ||
[]string{"relay_transfer_cross_chain"}, | ||
1, | ||
[]metrics.Label{ | ||
telemetry.NewLabel("erc20", token.String()), | ||
telemetry.NewLabel("denom", denom), | ||
telemetry.NewLabel("target", target), | ||
}, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Deprecated function usage
The function Fip20CrossChainEvents
is marked as deprecated but is still in use in x/crosschain/precompile/fip20_crosschain.go
. Consider replacing it with the CrossChainEvents
function found in the same file, if it serves the same purpose. This will help in cleaning up the legacy code.
- File:
x/crosschain/precompile/fip20_crosschain.go
- Line:
legacy.Fip20CrossChainEvents(ctx, args.Sender, tokenPair.GetERC20Contract(), args.Receipt, fxtypes.Byte32ToString(args.Target), tokenPair.GetDenom(), args.Amount, args.Fee)
Analysis chain
Deprecated function usage
The function Fip20CrossChainEvents
is marked as deprecated. Ensure that it is no longer used in the codebase or consider removing it to clean up the legacy code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the deprecated function `Fip20CrossChainEvents` is not used anywhere else in the codebase.
# Test: Search for the function usage. Expect: No occurrences outside this file.
rg --type go $'Fip20CrossChainEvents'
Length of output: 434
Script:
#!/bin/bash
# Description: Gather the surrounding lines of the usage of the deprecated function `Fip20CrossChainEvents` in `x/crosschain/precompile/fip20_crosschain.go`.
# Test: Extract 10 lines before and after the usage for context.
rg --type go --context 10 $'Fip20CrossChainEvents' x/crosschain/precompile/fip20_crosschain.go
Length of output: 943
Script:
#!/bin/bash
# Description: Search for potential alternative functions to `Fip20CrossChainEvents` in the codebase.
# Test: Look for other functions related to cross-chain events.
rg --type go 'func .*CrossChainEvents'
Length of output: 381
Summary by CodeRabbit
New Features
PrecompileMethod
interface for handling precompiled contracts within Cosmos SDK using EVM.Refactor
Contract
struct.BridgeCall
,CrossChain
, andBridgeCoinAmount
methods into dedicated structs for improved organization and maintainability.Tests
PrecompileMethod
functionalities, including bridge calls, coin amounts, and cross-chain sends.