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: use interface call evm #887

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

zakir-code
Copy link
Contributor

@zakir-code zakir-code commented Jan 10, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced EVM and contract interaction capabilities with new methods for querying, applying, and executing contracts.
    • Improved crosschain and bridge call functionality with more flexible caller context management.
  • Changes

    • Removed direct dependencies on specific contract keeper implementations.
    • Updated method signatures across multiple modules to support more dynamic contract interactions.
    • Streamlined keeper structures by removing redundant keeper fields.
  • Technical Improvements

    • Added support for more granular contract method execution.
    • Introduced more flexible context handling for EVM and crosschain operations.
    • Simplified keeper initialization and dependency management.
  • Compatibility

    • Existing method flows and error handling largely preserved.
    • Modifications primarily focus on expanding interaction capabilities.

These changes represent a significant refactoring of contract and crosschain interaction mechanisms, providing more robust and flexible contract management capabilities.

Copy link

coderabbitai bot commented Jan 10, 2025

Walkthrough

This pull request introduces significant architectural changes to the crosschain and ERC20 modules by removing direct dependencies on specific contract-related components. The modifications focus on enhancing the flexibility of keeper interactions by introducing a new contract.Caller interface and updating method signatures across multiple files. The changes involve removing hardcoded contract instantiations, adding more generic caller parameters, and streamlining the way EVM and contract interactions are handled.

Changes

File Change Summary
app/keepers/keepers.go Removed import and initialization of contract.NewBridgeFeeQuoteKeeper and contract.NewERC20TokenKeeper
contract/contract.go Added ExecuteEVM method to Caller interface
contract/interface.go Added ExecuteEVM method to EvmKeeper interface
precompiles/crosschain/* Updated methods to include vmCaller parameter and modified bridge call handling
precompiles/types/expected_keepers.go Updated method signatures to include contract.Caller parameter
x/crosschain/keeper/* Updated multiple keeper methods to include contract.Caller parameter
x/erc20/keeper/* Removed evmErc20Keeper field, updated method signatures to use contract.Caller

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Keeper
    participant EVMKeeper
    participant Contract

    Caller->>Keeper: Method Call with Caller Context
    Keeper->>EVMKeeper: Execute with Caller
    EVMKeeper->>Contract: Perform Contract Interaction
    Contract-->>EVMKeeper: Return Response
    EVMKeeper-->>Keeper: Return Result
    Keeper-->>Caller: Return Final Result
Loading

Possibly related PRs

Poem

🐰 Hopping through code with glee,
Contracts dance, now more free!
Caller context, a new delight,
Bridging worlds with rabbit might!
Refactored paths, clean and bright 🌟


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f57f1f and 5a87334.

📒 Files selected for processing (3)
  • contract/contract.go (2 hunks)
  • precompiles/vm_caller.go (2 hunks)
  • x/evm/keeper/contract_code.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • precompiles/vm_caller.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🔇 Additional comments (3)
x/evm/keeper/contract_code.go (1)

Line range hint 130-144: LGTM! Parameter name change improves clarity.

The renaming of constructorData to args better reflects the parameter's purpose, as it's used for any method arguments, not just constructor data. The implementation remains functionally equivalent.

contract/contract.go (2)

90-90: LGTM! Parameter name change maintains consistency.

The renaming of constructorData to args in the interface matches the implementation change, ensuring consistency across the codebase.


91-91: LGTM! New method enhances interface flexibility.

The addition of ExecuteEVM provides a lower-level interface for direct EVM execution, which aligns with the architectural goal of enhancing keeper interaction flexibility. The method signature includes all essential parameters for EVM transactions.

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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 generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration 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: 3

🔭 Outside diff range comments (3)
x/crosschain/keeper/bridge_call_out.go (1)

Update BuildOutgoingTxBatch call in msg_server.go to use caller parameter

The function signature has been updated correctly, but one call site needs to be fixed:

  • x/crosschain/keeper/msg_server.go:357: Change s.BuildOutgoingTxBatch(ctx, s.evmKeeper, sender, msg.Dest, msg.Amount, msg.BridgeFee) to use caller parameter instead of s.evmKeeper
🔗 Analysis chain

Line range hint 237-250: Ensure BuildOutgoingTxBatch now accepts the caller parameter

With caller added to CrosschainBaseCoin and passed to BuildOutgoingTxBatch, please ensure:

  • BuildOutgoingTxBatch has been updated to accept caller.
  • All other calls to BuildOutgoingTxBatch have been updated to include caller.

Use the script below to confirm:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `BuildOutgoingTxBatch` includes `caller` in definitions and calls.

# Check function definition
ast-grep --pattern $'func ($_).BuildOutgoingTxBatch(ctx $_, caller contract.Caller, $$$) ($_|$_, error) { $$$ }' --lang go

# Check for calls without 'caller'
rg --no-heading --line-number 'BuildOutgoingTxBatch\(ctx,' --glob '*.go' | rg -v 'BuildOutgoingTxBatch\(ctx, caller'

Length of output: 386


Script:

#!/bin/bash
# Find the function definition
ast-grep --pattern $'func ($_).BuildOutgoingTxBatch(ctx $_, $$$) ($_|$_, error) { $$$ }' --lang go

# Get context of the msg_server call
rg -B 5 -A 5 'BuildOutgoingTxBatch\(ctx, s\.evmKeeper' x/crosschain/keeper/msg_server.go

# Look for any other variants of the call
rg --no-heading --line-number 'BuildOutgoingTxBatch' --glob '*.go'

Length of output: 1030

x/crosschain/keeper/send_to_fx.go (1)

Update calls to BaseCoinToEvm to use proper caller parameter

The following calls to BaseCoinToEvm need to be updated to use a proper contract.Caller parameter instead of k.evmKeeper:

  • x/crosschain/keeper/many_to_one.go:142
  • x/crosschain/keeper/many_to_one.go:162
🔗 Analysis chain

Line range hint 16-48: Update SendToFxExecuted to include caller parameter

The function SendToFxExecuted now includes caller contract.Caller:

  • Ensure that the method signature is updated in interfaces or base classes.
  • Update all calls to SendToFxExecuted to include the caller argument.

To verify, run:

Confirm BaseCoinToEvm accepts caller

At line 48, k.erc20Keeper.BaseCoinToEvm is called with caller. Ensure that:

  • The method BaseCoinToEvm has been updated to include caller.
  • All calls to BaseCoinToEvm throughout the codebase have been updated accordingly.

Use the script below:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `SendToFxExecuted` definitions and calls include `caller`.

# Check function definition
ast-grep --pattern $'func ($_).SendToFxExecuted(ctx $_, caller contract.Caller, $$$) error { $$$ }' --lang go

# Check for calls without 'caller'
rg --no-heading --line-number 'SendToFxExecuted\(ctx,' --glob '*.go' | rg -v 'SendToFxExecuted\(ctx, caller'

Length of output: 221


Script:

#!/bin/bash
# Description: Verify that `BaseCoinToEvm` includes `caller` in definitions and calls.

# Check function definition
ast-grep --pattern $'func ($_).BaseCoinToEvm(ctx $_, caller contract.Caller, $$$) ($_|$_, error) { $$$ }' --lang go

# Check for calls without 'caller'
rg --no-heading --line-number 'BaseCoinToEvm\(ctx,' --glob '*.go' | rg -v 'BaseCoinToEvm\(ctx, caller'

Length of output: 569


Script:

#!/bin/bash
# Search for SendToFxExecuted with a broader pattern
ast-grep --pattern 'func $$.SendToFxExecuted' --lang go

# Search for interface definitions
rg --type go "interface.*\{" -A 10 | rg -A 10 "SendToFxExecuted|BaseCoinToEvm"

# Check evmKeeper type
rg --type go "evmKeeper.*contract\.Caller"

Length of output: 1407


Script:

#!/bin/bash
# Find SendToFxExecuted implementations with different pattern
rg --type go -A 5 "func.*SendToFxExecuted"

# Check if evmKeeper implements or is related to contract.Caller
rg --type go -A 5 "type.*evmKeeper"

Length of output: 537

precompiles/crosschain/bridge_call.go (1)

Line range hint 87-91: Enhance error handling for bridge call operations.

The bridge call operation involves complex state changes across EVM and native chain. Consider adding more detailed error context and ensuring proper state cleanup on failure.

Add error context and cleanup:

-nonce, err := crosschainKeeper.BridgeCallBaseCoin(ctx, vmCaller, sender, args.Refund, args.To, baseCoins, args.Data, args.Memo, args.QuoteId, args.GasLimit, fxTarget, originTokenAmount)
+nonce, err := crosschainKeeper.BridgeCallBaseCoin(ctx, vmCaller, sender, args.Refund, args.To, baseCoins, args.Data, args.Memo, args.QuoteId, args.GasLimit, fxTarget, originTokenAmount)
 if err != nil {
-    return err
+    return sdkerrors.Wrapf(err, "bridge call failed for sender %s, refund %s, amount %v", 
+        sender.String(), args.Refund.String(), baseCoins)
 }
🧹 Nitpick comments (8)
x/erc20/types/expected_keepers.go (1)

48-50: LGTM! Well-structured interface hierarchy for EVM interactions.

The new methods provide a clean separation of concerns:

  • QueryContract: High-level read-only contract queries
  • ApplyContract: High-level contract interactions with value transfer
  • ExecuteEVM: Low-level EVM execution

Consider documenting the recommended usage patterns for each method, particularly when to use ApplyContract vs ExecuteEVM, to guide future developers.

x/crosschain/keeper/keeper.go (1)

23-30: LGTM! Clean removal of direct keeper dependencies.

The removal of bridgeFeeQuoteKeeper and erc20TokenKeeper fields aligns with the architectural goal of using the EVMKeeper interface for contract interactions.

The refactoring improves modularity by reducing direct dependencies between modules.

x/crosschain/keeper/bridge_fee.go (1)

Line range hint 31-43: Optimize token transfer path.

The function could be optimized by checking if the token is native before making the ERC20 token lookup.

Consider reordering the checks:

 func (k Keeper) TransferBridgeFee(ctx sdk.Context, caller contract.Caller, from, to common.Address, bridgeFee *big.Int, bridgeTokenName string) error {
-    if strings.ToUpper(bridgeTokenName) == fxtypes.DefaultDenom {
+    upperTokenName := strings.ToUpper(bridgeTokenName)
+    if upperTokenName == fxtypes.DefaultDenom {
         fees := sdk.NewCoins(sdk.NewCoin(fxtypes.DefaultDenom, sdkmath.NewIntFromBigInt(bridgeFee)))
         return k.bankKeeper.SendCoins(ctx, from.Bytes(), to.Bytes(), fees)
     }
     erc20Token, err := k.erc20Keeper.GetERC20Token(ctx, bridgeTokenName)
     if err != nil {
         return err
     }
     erc20TokenKeeper := contract.NewERC20TokenKeeper(caller)
     _, err = erc20TokenKeeper.Transfer(ctx, erc20Token.GetERC20Contract(), from, to, bridgeFee)
     return err
 }
precompiles/crosschain/crosschain.go (1)

68-68: Well-structured architectural improvement!

The addition of vmCaller and its consistent usage improves the code's testability and reduces direct dependencies on the EVM implementation. This change aligns well with interface-based design principles.

Also applies to: 95-96

x/crosschain/keeper/bridge_call_in.go (1)

18-18: Good architectural changes, but error handling could be improved

The consistent use of the contract.Caller interface across all EVM interactions improves the code's modularity. However, the error handling for ExecuteEVM could be more specific.

Consider enhancing the error handling:

-	txResp, err := caller.ExecuteEVM(ctx, callEvmSender, &to, nil, gasLimit, args)
-	if err != nil {
-		return err
-	}
-	if txResp.Failed() {
-		return types.ErrInvalid.Wrap(txResp.VmError)
-	}
+	txResp, err := caller.ExecuteEVM(ctx, callEvmSender, &to, nil, gasLimit, args)
+	if err != nil {
+		return fmt.Errorf("failed to execute EVM call: %w", err)
+	}
+	if txResp.Failed() {
+		return types.ErrInvalid.Wrapf("EVM execution failed: %s", txResp.VmError)
+	}

Also applies to: 44-44, 49-49, 79-79, 97-97, 101-101, 130-136

x/crosschain/types/expected_keepers.go (1)

49-49: Well-defined interface changes

The addition of QueryContract and ApplyContract methods provides a clean interface for contract interactions. This improves the separation of concerns and makes the contract interactions more explicit.

Consider adding godoc comments to describe the new methods:

// QueryContract executes a read-only contract call and unmarshals the result into res
QueryContract(ctx context.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error

// ApplyContract executes a state-modifying contract call with the given parameters
ApplyContract(ctx context.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*types.MsgEthereumTxResponse, error)

Also applies to: 72-73

precompiles/types/expected_keepers.go (1)

31-34: LGTM! Interface changes align with the refactoring objectives.

The addition of the caller contract.Caller parameter to ExecuteClaim, CrosschainBaseCoin, and BridgeCallBaseCoin methods is a good architectural improvement that:

  1. Enhances flexibility in EVM interactions
  2. Provides consistent caller context
  3. Reduces direct dependencies on contract components

Consider documenting the contract.Caller interface requirements in the interface comments to help implementers understand the expected behavior.

x/erc20/keeper/contract.go (1)

15-16: Consider caching the ERC20TokenKeeper instance.

Creating a new evmErc20Keeper instance on every call to ERC20BaseInfo could impact performance if this method is called frequently. Consider creating the instance once and storing it as a field in the Keeper struct.

 type Keeper struct {
     evmKeeper contract.EVMKeeper
+    evmErc20Keeper *contract.ERC20TokenKeeper
     // other fields...
 }

 func NewKeeper(...) Keeper {
     k := Keeper{
         evmKeeper: evmKeeper,
+        evmErc20Keeper: contract.NewERC20TokenKeeper(evmKeeper),
         // other fields...
     }
     return k
 }

 func (k Keeper) ERC20BaseInfo(ctx context.Context, contractAddr common.Address) (name, symbol string, decimals uint8, err error) {
-    evmErc20Keeper := contract.NewERC20TokenKeeper(k.evmKeeper)
-    name, err = evmErc20Keeper.Name(ctx, contractAddr)
+    name, err = k.evmErc20Keeper.Name(ctx, contractAddr)
     if err != nil {
         return name, symbol, decimals, err
     }
-    symbol, err = evmErc20Keeper.Symbol(ctx, contractAddr)
+    symbol, err = k.evmErc20Keeper.Symbol(ctx, contractAddr)
     if err != nil {
         return name, symbol, decimals, err
     }
-    decimals, err = evmErc20Keeper.Decimals(ctx, contractAddr)
+    decimals, err = k.evmErc20Keeper.Decimals(ctx, contractAddr)
     if err != nil {
         return name, symbol, decimals, err
     }
     return name, symbol, decimals, err
 }

Also applies to: 20-20, 24-24

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32b058c and 3f57f1f.

📒 Files selected for processing (29)
  • app/keepers/keepers.go (0 hunks)
  • contract/contract.go (2 hunks)
  • contract/interface.go (1 hunks)
  • precompiles/crosschain/bridge_call.go (1 hunks)
  • precompiles/crosschain/crosschain.go (2 hunks)
  • precompiles/crosschain/execute_claim.go (2 hunks)
  • precompiles/types/expected_keepers.go (2 hunks)
  • precompiles/vm_caller.go (2 hunks)
  • x/crosschain/keeper/abci.go (1 hunks)
  • x/crosschain/keeper/attestation_handler.go (3 hunks)
  • x/crosschain/keeper/bridge_call_in.go (5 hunks)
  • x/crosschain/keeper/bridge_call_in_test.go (1 hunks)
  • x/crosschain/keeper/bridge_call_out.go (8 hunks)
  • x/crosschain/keeper/bridge_call_out_test.go (1 hunks)
  • x/crosschain/keeper/bridge_fee.go (4 hunks)
  • x/crosschain/keeper/keeper.go (3 hunks)
  • x/crosschain/keeper/keeper_test.go (0 hunks)
  • x/crosschain/keeper/keeper_v1_test.go (1 hunks)
  • x/crosschain/keeper/many_to_one.go (2 hunks)
  • x/crosschain/keeper/msg_server.go (1 hunks)
  • x/crosschain/keeper/outgoing_tx.go (1 hunks)
  • x/crosschain/keeper/send_to_fx.go (2 hunks)
  • x/crosschain/mock/expected_keepers_mocks.go (4 hunks)
  • x/crosschain/types/expected_keepers.go (3 hunks)
  • x/erc20/keeper/contract.go (1 hunks)
  • x/erc20/keeper/convert.go (4 hunks)
  • x/erc20/keeper/keeper.go (2 hunks)
  • x/erc20/keeper/msg_server.go (1 hunks)
  • x/erc20/types/expected_keepers.go (1 hunks)
💤 Files with no reviewable changes (2)
  • x/crosschain/keeper/keeper_test.go
  • app/keepers/keepers.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🔇 Additional comments (22)
x/crosschain/keeper/bridge_call_out.go (3)

Line range hint 133-142: Proper use of caller.ExecuteEVM in BridgeCallOnRevert

Utilizing caller.ExecuteEVM enhances flexibility and aligns with the architectural changes, allowing for more dynamic EVM executions.


Line range hint 152-187: Ensure caller is correctly used in RefundOutgoingBridgeCall

Passing caller to k.erc20Keeper.BaseCoinToEvm ensures that EVM operations are executed in the correct context.


Line range hint 195-223: Confirm that HandlerBridgeCallOutFee accepts the caller parameter

In BridgeCallBaseCoin, caller is now passed to HandlerBridgeCallOutFee. Please verify that:

  • The method signature of HandlerBridgeCallOutFee has been updated to include caller.
  • All calls to HandlerBridgeCallOutFee in the codebase have been updated accordingly.

To verify, use the following script:

✅ Verification successful

✓ Verified: HandlerBridgeCallOutFee correctly accepts and receives the caller parameter

The method signature has been properly updated to include the caller parameter, and all calls to this method in the codebase pass the caller argument correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `HandlerBridgeCallOutFee` includes `caller` in definitions and calls.

# Check function definition
ast-grep --pattern $'func ($_).HandlerBridgeCallOutFee(ctx $_, caller contract.Caller, $$$) ($_|$_, error) { $$$ }' --lang go

# Check for calls without 'caller'
rg --no-heading --line-number 'HandlerBridgeCallOutFee\(ctx,' --glob '*.go' | rg -v 'HandlerBridgeCallOutFee\(ctx, caller'

Length of output: 251


Script:

#!/bin/bash
# Search for any mention of HandlerBridgeCallOutFee
rg --no-heading --line-number "HandlerBridgeCallOutFee" --glob '*.go' -A 2

# Try more flexible pattern for function definition
ast-grep --pattern 'func $_HandlerBridgeCallOutFee($_) { $$$ }' --lang go

# Look for interface definitions containing this method
ast-grep --pattern 'type $_ interface {
  $$$
  HandlerBridgeCallOutFee($_) $_
  $$$
}' --lang go

Length of output: 882

contract/interface.go (1)

18-18: Add ExecuteEVM method to all EvmKeeper implementations

The addition of ExecuteEVM to the EvmKeeper interface extends its functionality. Please ensure:

  • All structs implementing EvmKeeper also implement the new ExecuteEVM method to satisfy the interface.
  • Any mock implementations used in tests are updated accordingly.

To confirm, run the following script:

✅ Verification successful

All EvmKeeper implementations include ExecuteEVM method

The main implementation in x/evm/keeper/keeper.go and mock implementation in x/crosschain/mock/ both include the ExecuteEVM method with the correct signature. No other implementations were found that need updating.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all `EvmKeeper` implementations include `ExecuteEVM`.

# Find files containing structs that implement `EvmKeeper`
rg --files-with-matches 'type .* struct' --glob '*.go' | while read -r file; do
    echo "Checking $file:"
    if rg -q 'func.*ExecuteEVM\(' "$file"; then
        echo "ExecuteEVM method found in $file"
    else
        echo "ExecuteEVM method NOT found in $file"
    fi
done

Length of output: 67734


Script:

#!/bin/bash
# First find the EvmKeeper interface definition and implementations
echo "Finding EvmKeeper interface definition:"
ast-grep --pattern 'type EvmKeeper interface {
  $$$
}'

echo -e "\nFinding structs that implement EvmKeeper:"
ast-grep --pattern 'type $_ struct {
  $$$
}' | rg -B2 -A2 'EvmKeeper'

Length of output: 2743


Script:

#!/bin/bash
# Check the implementation of ExecuteEVM in fxevmkeeper
echo "Checking fxevmkeeper implementation:"
rg -p "func \(k .*Keeper\) ExecuteEVM" x/evm/keeper/

echo -e "\nChecking mock implementations:"
rg -p "func \(k .*\) ExecuteEVM" x/crosschain/mock/

Length of output: 324


Script:

#!/bin/bash
# Get the full implementation details
echo "Checking ExecuteEVM implementation in keeper:"
rg -A 5 "func \(k \*Keeper\) ExecuteEVM" x/evm/keeper/keeper.go

echo -e "\nChecking for other potential implementations:"
rg -p "type.*Keeper struct" x/evm/

Length of output: 488

x/crosschain/keeper/attestation_handler.go (2)

31-31: LGTM: Method signature update aligns with interface-based approach

The addition of the caller contract.Caller parameter aligns with the refactoring goal of using interface calls for EVM interactions.


41-45: Verify consistent caller parameter usage

The caller parameter is consistently passed to all executed methods. However, let's verify that all these methods have been updated to handle the caller parameter correctly.

✅ Verification successful

✓ Caller parameter implementation verified

All three methods (SendToFxExecuted, BridgeCallExecuted, and BridgeCallResultExecuted) have been correctly updated with consistent signatures that include the caller contract.Caller parameter.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all called methods have been updated to accept the caller parameter
# Search for method declarations to ensure they match the new signature

ast-grep --pattern 'func $$$SendToFxExecuted$$$($_, $caller contract.Caller, $_) $_'
ast-grep --pattern 'func $$$BridgeCallExecuted$$$($_, $caller contract.Caller, $_) $_'
ast-grep --pattern 'func $$$BridgeCallResultExecuted$$$($_, $caller contract.Caller, $_) $_'

Length of output: 270


Script:

#!/bin/bash
# Search for method definitions with context
rg -U "func.*SendToFxExecuted|func.*BridgeCallExecuted|func.*BridgeCallResultExecuted" -A 5

Length of output: 1724

x/crosschain/keeper/bridge_call_out_test.go (1)

52-52: LGTM: Test updated correctly for new method signature

The test has been properly updated to pass the evmKeeper parameter to BridgeCallResultExecuted.

x/crosschain/keeper/bridge_call_in_test.go (1)

64-64: LGTM: Test properly adapted for interface-based EVM calls

The test has been correctly updated to use the EvmKeeper as the contract caller, maintaining comprehensive validation of the bridge call execution.

precompiles/crosschain/execute_claim.go (1)

59-59: LGTM! Proper EVM context propagation.

The addition of precompiles.NewVMCall(evm) ensures that the EVM context is correctly propagated through the call chain.

Let's verify that all ExecuteClaim callers have been updated:

✅ Verification successful

All ExecuteClaim callers are correctly updated

The verification confirms that the only direct caller in precompiles/crosschain/execute_claim.go correctly uses precompiles.NewVMCall(evm). Other occurrences are in tests, interfaces, and generated code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ExecuteClaim calls to ensure they include the new VMCall parameter
rg -A 2 "ExecuteClaim\(" --type go

Length of output: 13757

x/erc20/keeper/msg_server.go (1)

30-30: LGTM! Consistent use of EVMKeeper interface.

The addition of s.k.evmKeeper parameter aligns with the architectural changes to standardize EVM interactions through the new interface.

Let's verify that all similar conversion methods have been updated:

✅ Verification successful

All ConvertCoin calls are properly using the EVMKeeper interface

The verification confirms that:

  • The EvmKeeper interface correctly implements the required contract.Caller interface
  • All ConvertCoin implementations consistently use the interface
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ConvertCoin calls to ensure consistent parameter usage
rg -A 2 "ConvertCoin\(" --type go

Length of output: 5398


Script:

#!/bin/bash
# Check the contract.Caller interface definition and its implementation
ast-grep --pattern 'type Caller interface {
  $$$
}'

# Also search for the evmKeeper type to verify it implements the interface
rg "type.*evmKeeper" --type go -A 5

Length of output: 724


Script:

#!/bin/bash
# Search for evmKeeper with different patterns and its method implementations
rg "type.*[Ee]vm[Kk]eeper" --type go -A 5

# Search for the interface methods implementation
rg "func \(.*[Ee]vm[Kk]eeper\) (QueryContract|ApplyContract|ExecuteEVM)" --type go -A 2

Length of output: 2897

x/erc20/keeper/keeper.go (1)

22-24: LGTM! Clean removal of direct keeper dependency.

The removal of evmErc20Keeper and retention of only the necessary keeper interfaces improves the architecture by reducing coupling.

precompiles/crosschain/bridge_call.go (1)

Line range hint 73-83: Consider adding validation for token amounts.

The code processes token amounts without explicit validation of the array lengths.

Add validation for token arrays:

+if len(args.Tokens) != len(args.Amounts) {
+    return errors.New("token and amount arrays must have equal length")
+}
 for i, token := range args.Tokens {
     baseCoin, err := m.EvmTokenToBaseCoin(ctx, vmCaller, crosschainKeeper, sender, token, args.Amounts[i])
     if err != nil {
         return err
     }
     baseCoins = append(baseCoins, baseCoin)
 }
x/crosschain/keeper/keeper_v1_test.go (1)

112-114: Test updated correctly for new interface

The test has been properly updated to accommodate the new interface requirements by passing the EvmKeeper instance.

x/crosschain/keeper/many_to_one.go (1)

142-142: LGTM! Method calls updated consistently.

The addition of k.evmKeeper to BaseCoinToEvm calls aligns with the interface refactoring changes.

Also applies to: 162-162

x/crosschain/keeper/outgoing_tx.go (1)

18-20: LGTM! Proper integration of the caller interface.

The changes correctly:

  1. Add the caller parameter to the method signature
  2. Initialize the bridge fee quote keeper with the caller
  3. Maintain existing error handling

Let's verify that all callers of this method have been updated:

✅ Verification successful

All callers updated correctly, but method is marked as deprecated

The verification confirms that all callers have been updated to include the caller parameter. However, note that this method is marked as deprecated with a comment suggesting to use BuildOutgoingBridgeCall instead.

  • msg_server.go: Uses s.evmKeeper as caller
  • bridge_call_out.go: Uses the provided caller parameter
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for calls to BuildOutgoingTxBatch to ensure they include the caller parameter
ast-grep --pattern 'BuildOutgoingTxBatch($$$)'

Length of output: 48


Script:

#!/bin/bash
# Search for BuildOutgoingTxBatch calls using ripgrep
rg -l "BuildOutgoingTxBatch" --type go

# Show the actual calls with context
rg "BuildOutgoingTxBatch" --type go -B 2 -A 1

Length of output: 1209

x/crosschain/keeper/abci.go (1)

174-174: LGTM! Consistent with interface refactoring.

The addition of k.evmKeeper to the RefundOutgoingBridgeCall call aligns with the broader refactoring changes.

x/crosschain/keeper/msg_server.go (1)

357-357: LGTM: Consistent with the interface-based EVM interaction pattern.

The change correctly passes the evmKeeper to BuildOutgoingTxBatch, aligning with the PR's objective of using interface-based EVM calls.

x/crosschain/mock/expected_keepers_mocks.go (3)

364-375: LGTM: Mock correctly updated for BaseCoinToEvm.

The mock implementation correctly reflects the updated signature with the new caller parameter.


548-566: LGTM: ApplyContract mock implementation.

The mock correctly implements the variadic function pattern for contract application, properly handling the constructor data arguments.


597-613: LGTM: QueryContract mock implementation.

The mock correctly implements the variadic function pattern for contract queries, properly handling the args parameter.

contract/contract.go (2)

8-8: LGTM: Required import for SDK context.

The import is necessary for the new ExecuteEVM method signature.


91-91: LGTM: New ExecuteEVM method in Caller interface.

The method signature provides all necessary parameters for direct EVM execution while maintaining consistency with the existing interface methods.

x/crosschain/keeper/bridge_call_out.go Show resolved Hide resolved
precompiles/vm_caller.go Outdated Show resolved Hide resolved
x/erc20/keeper/convert.go Show resolved Hide resolved
@nulnut nulnut merged commit 558e8f1 into main Jan 10, 2025
12 checks passed
@nulnut nulnut deleted the nulnut/refactor-convert-coins branch January 10, 2025 07:07
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.

2 participants