Skip to content
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

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

zakir-code
Copy link
Contributor

@zakir-code zakir-code commented Jun 25, 2024

Summary by CodeRabbit

  • New Features

    • Introduced PrecompileMethod interface for handling precompiled contracts within Cosmos SDK using EVM.
    • Added functionality for canceling cross-chain sends and handling FIP20 token transfers between chains.
  • Refactor

    • Centralized precompile method handling within the Contract struct.
    • Refactored BridgeCall, CrossChain, and BridgeCoinAmount methods into dedicated structs for improved organization and maintainability.
    • Updated various test files to use new struct methods and improve consistency.
  • Tests

    • Added comprehensive tests for new PrecompileMethod functionalities, including bridge calls, coin amounts, and cross-chain sends.
    • Updated existing tests to align with refactored methods and improved event handling.

Copy link

coderabbitai bot commented Jun 25, 2024

Walkthrough

The recent changes introduce a significant refactor to the precompile methods in the cross-chain package of the Cosmos SDK. A new interface, PrecompileMethod, has been implemented along with specialized structs for handling specific precompile methods. The refactor centralizes method logic, enhances test coverage, and simplifies method handling within the contract. This includes new methods for FIP20 cross-chain transactions, canceling send to external chain, and bridge call alterations.

Changes

File(s) Change Summary
contract/precompile.go Introduced PrecompileMethod interface and methods for EVM precompiled contracts.
tests/precompile_suite.go, x/crosschain/..._test.go Refactored tests to use new methods from the precompile package and updated assertions.
x/crosschain/legacy/precompile_events.go Added new event handling functions and attributes for cross-chain events, updating deprecated functions.
x/crosschain/precompile/bridge_call.go Refactored bridge call logic into BridgeCallMethod with new methods for handling bridge calls.
x/crosschain/precompile/bridge_coin_amount.go Introduced BridgeCoinAmountMethod struct and refactored bridge coin amount logic.
x/crosschain/precompile/bridge_coin_amount_test.go Added tests for BridgeCoinAmountMethod.
x/crosschain/precompile/cancel_send_to_external.go Added CancelSendToExternalMethod with methods for canceling external sends.
x/crosschain/precompile/contract.go Centralized methods within the Contract struct by adding a methods slice.
x/crosschain/precompile/crosschain.go Introduced CrossChainMethod for handling cross-chain transactions, removing old functions.
x/crosschain/precompile/fip20_crosschain.go Added new file to handle FIP20 cross-chain methods with associated methods and events.
x/crosschain/types/contract.go Removed incompatible contract handling logic, updated to fit new method structure.

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
Loading

Poem

🌟 In the cosmos, new seeds are sown,

🌐 Cross-chains knit, in fields new-grown.

💡 Methods crafted with care and zest,

🚀 Bridging realms, a coder's quest.

🛤️ Paths once tangled now run straight,

✨ All aboard! Innovation's fate.

🌠 A rabbit weaves the code's new fate.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 Consistency

The 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 Consistency

The 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 in IncreaseBridgeFeeArgs Validation

The 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 in BridgeCallArgs

The validation in BridgeCallArgs is crucial for the integrity of the operation. It might be beneficial to add checks for the lengths of the Tokens and Amounts 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 Readability

The Run method in CrossChainMethod 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

Commits

Files that changed from the base of the PR and between 7b5e453 and 5e9a271.

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 contracts

The 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: Refactored Contract struct with improved modularity

The Contract struct now correctly uses a slice of PrecompileMethod, enhancing modularity and simplifying method management. This is a positive change in line with best practices for maintainability.


61-67: Efficient gas calculation method

The 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 management

The Run method in the Contract 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-structured BridgeCallMethod class

The 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 execution

The Run method in the BridgeCallMethod 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 testing

The 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 Clarity

The 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 struct IncreaseBridgeFeeMethod is well-defined and properly encapsulates the necessary components (Keeper, Method, and Event from the ABI).


26-31: Constructor function for IncreaseBridgeFeeMethod: Properly initializes struct.
The constructor function NewIncreaseBridgeFeeMethod initializes the IncreaseBridgeFeeMethod 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 of IsReadonly, GetMethodId, and RequiredGas 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.
The FIP20CrossChainMethod struct extends CrossChainMethod and includes an additional Method 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 methods IsReadonly, GetMethodId, and RequiredGas 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.
The Keeper 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 for CancelSendToExternal and IncreaseBridgeShee 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 of fip20CrossChain

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 with nil 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 review

Manipulating 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)
Copy link

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)
Copy link

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 of NewCancelSendToExternalMethod(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

Comment on lines +30 to +35
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))
Copy link

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
}

Comment on lines +827 to +832
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)
Copy link

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.

Suggested change
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",
Copy link

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.

Comment on lines +32 to +35
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))
Copy link

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 the keeper parameter without null checks.
  • crosschain.go:
    • NewCrossChainMethod also directly uses the keeper 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

Comment on lines +46 to +98
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)
}
Copy link

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.

  1. Error Handling: The method correctly checks for null router and missing chain support, which is good for robustness.
  2. 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.
  3. 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.

Comment on lines +42 to +98
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)
}
Copy link

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.

Comment on lines +39 to +51
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),
))
}
Copy link

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

Comment on lines 25 to 29

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"`
}
Copy link

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.

Comment on lines +44 to +58
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)
}
Copy link

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.

Suggested change
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)
}

Comment on lines +60 to +71
// 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())
}

Copy link

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.

Suggested change
// 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())

Comment on lines +16 to +21
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))
}
Copy link

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.

Comment on lines +42 to +93
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())
}
Copy link

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.

Comment on lines +14 to +35
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),
},
)
}
Copy link

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

@zakir-code zakir-code merged commit 3c4f227 into main Jun 25, 2024
9 checks passed
@zakir-code zakir-code deleted the zakir/crosschain-precompile branch June 25, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant