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.

Sign up for GitHub

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

Jump to bottom

refactor: skip 'from' account verification during contract query #787

Merged
merged 1 commit into from
Oct 28, 2024
Merged

refactor: skip 'from' account verification during contract query #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions contract/contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package contract

import (
"context"
"math/big"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
Expand Down Expand Up @@ -48,8 +48,8 @@ var (
)

type Caller interface {
QueryContract(ctx sdk.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error
ApplyContract(ctx sdk.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*evmtypes.MsgEthereumTxResponse, error)
QueryContract(ctx context.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error
ApplyContract(ctx context.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*evmtypes.MsgEthereumTxResponse, error)
Comment on lines +51 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Context type change requires attention in implementation

The implementation in x/evm/keeper/contract_code.go still uses sdk.UnwrapSDKContext(ctx) in the QueryContract method, which indicates a potential issue with the context type change. This suggests that while the interface has been updated to use context.Context, the implementation still relies on SDK context functionality.

  • x/evm/keeper/contract_code.go: The QueryContract implementation needs to be reviewed as it's converting the context back to SDK context: sdk.UnwrapSDKContext(ctx)
🔗 Analysis chain

Verify the impact of context type change.

The change from sdk.Context to context.Context looks good and aligns with the PR objective. However, we should ensure that all implementations of this interface have been updated accordingly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all implementations of the Caller interface
ast-grep --pattern 'type $_ struct {
  $$$
}

func ($_ $_) QueryContract(ctx $_, $$$) error {
  $$$
}'

# Search for any remaining uses of sdk.Context with QueryContract
rg -A 2 'QueryContract.*sdk\.Context'

Length of output: 144

Script:

#!/bin/bash
# Let's try a different approach to find implementations and usages

# Find all files that might contain Caller interface implementations
rg -l "QueryContract|ApplyContract" --type go

# Find struct types that implement these methods
ast-grep --pattern 'type $name struct {
  $$$
}'

# Look for function implementations with context parameter
rg -A 5 'func \(.*\) QueryContract\('
rg -A 5 'func \(.*\) ApplyContract\('

# Look for any remaining imports of sdk.Context
rg 'sdk\s*"github\.com/cosmos/cosmos-sdk/types"'

Length of output: 17379

}

type Contract struct {
Expand Down
2 changes: 1 addition & 1 deletion x/crosschain/keeper/bridge_call_in.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (k Keeper) BridgeCallEvm(ctx sdk.Context, sender, refundAddr, to, receiverA
}

gasLimit := k.GetBridgeCallMaxGasLimit(ctx)
txResp, err := k.evmKeeper.CallEVM(ctx, callEvmSender, &to, value.BigInt(), gasLimit, args, true)
txResp, err := k.evmKeeper.ExecuteEVM(ctx, callEvmSender, &to, value.BigInt(), gasLimit, args)
if err != nil {
return err
}
Expand Down
12 changes: 6 additions & 6 deletions x/crosschain/mock/expected_keepers_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion x/crosschain/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ type Erc20Keeper interface {

// EVMKeeper defines the expected EVM keeper interface used on crosschain
type EVMKeeper interface {
CallEVM(ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, gasLimit uint64, data []byte, commit bool) (*types.MsgEthereumTxResponse, error)
ExecuteEVM(ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, gasLimit uint64, data []byte) (*types.MsgEthereumTxResponse, error)
IsContract(ctx sdk.Context, account common.Address) bool
}

Expand Down
15 changes: 10 additions & 5 deletions x/evm/keeper/contract_code.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
"bytes"
"context"
"encoding/hex"
"math/big"

Expand Down Expand Up @@ -79,7 +80,7 @@ func (k *Keeper) DeployContract(ctx sdk.Context, from common.Address, abi abi.AB
return common.Address{}, err
}

_, err = k.CallEVMWithoutGas(ctx, from, nil, nil, data, true)
_, err = k.callEvm(ctx, from, nil, nil, nonce, data, true)
if err != nil {
return common.Address{}, err
}
Expand Down Expand Up @@ -110,12 +111,12 @@ func (k *Keeper) DeployUpgradableContract(ctx sdk.Context, from, logic common.Ad
}

// QueryContract query contract with args and res
func (k *Keeper) QueryContract(ctx sdk.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error {
func (k *Keeper) QueryContract(ctx context.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent context type usage detected in the codebase

The codebase shows mixed usage of context types:

  • The reviewed QueryContract and ApplyContract methods use context.Context
  • Several other methods in the same package still use sdk.Context:
    • CreateContractWithCode
    • UpdateContractCode
    • DeployContract
    • DeployUpgradableContract
    • IsContract

These related contract operation methods should be updated to maintain consistency in context handling.

🔗 Analysis chain

Verify consistent context type usage across the codebase

Let's ensure all related methods have been updated to use context.Context consistently.

Also applies to: 130-130

Let me gather more information to verify the context usage more thoroughly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining sdk.Context usage in related methods
# Test: Look for method signatures that might still be using sdk.Context
rg -A 1 "func.*QueryContract|func.*ApplyContract"

Length of output: 624

Script:

#!/bin/bash
# Search for any remaining sdk.Context usage in the x/evm directory
rg "sdk\.Context" x/evm/

# Also check for any method signatures containing Context
ast-grep --pattern 'func $_($_Context$_, $$$) $$$'

Length of output: 1816

data, err := abi.Pack(method, args...)
if err != nil {
return types.ErrABIPack.Wrap(err.Error())
}
resp, err := k.CallEVMWithoutGas(ctx, from, &contract, nil, data, false)
resp, err := k.callEvm(sdk.UnwrapSDKContext(ctx), from, &contract, nil, 0, data, false)
if err != nil {
return err
}
Expand All @@ -126,12 +127,16 @@ func (k *Keeper) QueryContract(ctx sdk.Context, from, contract common.Address, a
}

// ApplyContract apply contract with args
func (k *Keeper) ApplyContract(ctx sdk.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*evmtypes.MsgEthereumTxResponse, error) {
func (k *Keeper) ApplyContract(ctx context.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*evmtypes.MsgEthereumTxResponse, error) {
args, err := abi.Pack(method, constructorData...)
if err != nil {
return nil, types.ErrABIPack.Wrap(err.Error())
}
resp, err := k.CallEVMWithoutGas(ctx, from, &contract, value, args, true)
nonce, err := k.accountKeeper.GetSequence(ctx, from.Bytes())
if err != nil {
return nil, err
}
resp, err := k.callEvm(sdk.UnwrapSDKContext(ctx), from, &contract, value, nonce, args, true)
if err != nil {
return nil, err
}
Expand Down
28 changes: 17 additions & 11 deletions x/evm/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func NewKeeper(ek *evmkeeper.Keeper, ak fxevmtypes.AccountKeeper) *Keeper {
}
}

// CallEVMWithoutGas performs a smart contract method call using contract data without gas
// Deprecated: please use callEvm todo: remove this
func (k *Keeper) CallEVMWithoutGas(
ctx sdk.Context,
from common.Address,
Expand All @@ -52,13 +52,24 @@ func (k *Keeper) CallEVMWithoutGas(
data []byte,
commit bool,
) (*types.MsgEthereumTxResponse, error) {
gasMeter := ctx.GasMeter()
ctx = ctx.WithGasMeter(storetypes.NewInfiniteGasMeter())

nonce, err := k.accountKeeper.GetSequence(ctx, from.Bytes())
if err != nil {
return nil, err
}
return k.callEvm(ctx, from, contract, value, nonce, data, commit)
}

func (k *Keeper) callEvm(
ctx sdk.Context,
from common.Address,
contract *common.Address,
value *big.Int,
nonce uint64,
data []byte,
commit bool,
) (*types.MsgEthereumTxResponse, error) {
gasMeter := ctx.GasMeter()
ctx = ctx.WithGasMeter(storetypes.NewInfiniteGasMeter())

gasLimit := fxcontract.DefaultGasCap
params := ctx.ConsensusParams()
Expand Down Expand Up @@ -103,14 +114,13 @@ func (k *Keeper) CallEVMWithoutGas(
return res, nil
}

func (k *Keeper) CallEVM(
func (k *Keeper) ExecuteEVM(
ctx sdk.Context,
from common.Address,
contract *common.Address,
value *big.Int,
gasLimit uint64,
data []byte,
commit bool,
) (*types.MsgEthereumTxResponse, error) {
gasMeter := ctx.GasMeter()
ctx = ctx.WithGasMeter(storetypes.NewInfiniteGasMeter())
Expand All @@ -119,10 +129,6 @@ func (k *Keeper) CallEVM(
if err != nil {
return nil, err
}
params := ctx.ConsensusParams()
if params.Block != nil && params.Block.MaxGas > 0 {
gasLimit = uint64(params.Block.MaxGas)
}

if value == nil {
value = big.NewInt(0)
Expand All @@ -141,7 +147,7 @@ func (k *Keeper) CallEVM(
SkipAccountChecks: false,
}

res, err := k.ApplyMessage(ctx, msg, types.NewNoOpTracer(), commit)
res, err := k.ApplyMessage(ctx, msg, types.NewNoOpTracer(), true)
if err != nil {
return nil, err
}
Expand Down
6 changes: 5 additions & 1 deletion x/evm/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ func (k *Keeper) CallContract(goCtx context.Context, msg *fxevmtypes.MsgCallCont
if account == nil || !account.IsContract() {
return nil, govtypes.ErrInvalidProposalMsg.Wrapf("contract %s not found", contract.Hex())
}
_, err := k.CallEVMWithoutGas(ctx, k.module, &contract, nil, common.Hex2Bytes(msg.Data), true)
nonce, err := k.accountKeeper.GetSequence(ctx, k.module.Bytes())
if err != nil {
return nil, err
}
_, err = k.callEvm(ctx, k.module, &contract, nil, nonce, common.Hex2Bytes(msg.Data), true)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion x/evm/testutil/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (s *EVMSuite) Call(abi abi.ABI, method string, res interface{}, args ...int
}

func (s *EVMSuite) CallEVM(data []byte, gasLimit uint64) *evmtypes.MsgEthereumTxResponse {
tx, err := s.evmKeeper.CallEVM(s.ctx, s.GetFrom(), &s.contractAddr, nil, gasLimit, data, false)
tx, err := s.evmKeeper.ExecuteEVM(s.ctx, s.GetFrom(), &s.contractAddr, nil, gasLimit, data)
s.NoError(err)
return tx
Comment on lines +83 to 85
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update required: CallEVM references need to be migrated to ExecuteEVM

Several files still use the deprecated CallEVM method that needs to be updated to ExecuteEVM:

  • x/staking/testutil/staking_precompile.go: Multiple instances of CallEVM usage
  • x/staking/precompile/delegation_test.go, delegation_rewards_test.go, contract_test.go: Using deprecated CallEVMWithoutGas
  • x/crosschain/precompile/has_oracle_test.go, is_oracle_online_test.go: Using deprecated CallEVMWithoutGas
  • x/evm/testutil/evm.go: Contains both old CallEVM method and new ExecuteEVM usage
  • x/evm/keeper/keeper.go: Contains deprecated CallEVMWithoutGas method

The change in the reviewed file is incomplete as it introduces inconsistency by using ExecuteEVM while keeping the old CallEVM method. All references should be updated to use the new ExecuteEVM method for consistency.

🔗 Analysis chain

LGTM! Verify test coverage.

The change from CallEVM to ExecuteEVM aligns with the broader refactoring effort mentioned in the PR summary.

Let's verify the test coverage and usage of this method:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any test files that might still be using the old CallEVM method name
# or might need updates due to this change

# Search for test files using EVMSuite
rg -t go "EVMSuite" --files-with-matches

# Search for any remaining references to CallEVM that might need updating
rg -t go "CallEVM" -C 3

Length of output: 5431

}
Expand Down
4 changes: 2 additions & 2 deletions x/ibc/middleware/keeper/ibc_call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func (k Keeper) HandlerIbcCallEvm(ctx sdk.Context, sender common.Address, evmPac
}
ctx.EventManager().EmitEvent(sdk.NewEvent(types.EventTypeIBCCall, attrs...))
}()
txResp, err := k.evmKeeper.CallEVM(ctx, sender,
evmPacket.GetToAddress(), evmPacket.Value.BigInt(), uint64(limit), evmPacket.MustGetData(), true)
txResp, err := k.evmKeeper.ExecuteEVM(ctx, sender,
evmPacket.GetToAddress(), evmPacket.Value.BigInt(), uint64(limit), evmPacket.MustGetData())
Comment on lines +48 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Change from CallEVM to ExecuteEVM is correct, but legacy method needs cleanup

The verification revealed that:

  • CallEVMWithoutGas is marked as deprecated with a comment "please use callEvm"
  • The method exists as legacy code that should be removed
  • The change to ExecuteEVM in the IBC middleware is correct and aligned with the refactoring

Recommendations:

  • Create a follow-up issue to remove the deprecated CallEVMWithoutGas method
  • Update test files to use ExecuteEVM instead of the deprecated method
🔗 Analysis chain

Method call updated from CallEVM to ExecuteEVM

The change aligns with the broader refactoring effort. Let's verify the consistency of this change across the codebase.

Remaining references to CallEVM need attention

The verification revealed inconsistencies in the codebase:

  • CallEVMWithoutGas is still being used in multiple test files and the EVM keeper
  • Both ExecuteEVM and CallEVM variants coexist, suggesting incomplete refactoring

Let's verify if these are intentionally different methods or oversight:

Let's check one more aspect to understand the complete picture:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that CallEVM is completely replaced with ExecuteEVM
# and there are no lingering references to the old method name.

echo "Checking for any remaining references to CallEVM..."
rg "CallEVM" --type go

echo "Verifying the new ExecuteEVM method signature in the EVM keeper interface..."
ast-grep --pattern 'interface {
  $$$
  ExecuteEVM($$$)
  $$$
}'

Length of output: 2370

Script:

#!/bin/bash
# Check the implementation and signature of CallEVMWithoutGas
ast-grep --pattern 'func $_ CallEVMWithoutGas($_) $_$ {
  $$$
}'

# Check if ExecuteEVM is called within CallEVMWithoutGas (might be a wrapper)
echo "Checking relationship between CallEVMWithoutGas and ExecuteEVM..."
rg -A 5 "CallEVMWithoutGas" x/evm/keeper/keeper.go

Length of output: 392

Script:

#!/bin/bash
# Get more context around CallEVMWithoutGas implementation
rg -B 2 -A 15 "func \(k \*Keeper\) CallEVMWithoutGas" x/evm/keeper/keeper.go

# Check if there are any direct ExecuteEVM calls in test files
echo "Checking ExecuteEVM usage in tests..."
rg "ExecuteEVM" --type go -g "*_test.go"

Length of output: 643

if err != nil {
evmErrCause = err.Error()
return err
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/middleware/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

type EvmKeeper interface {
CallEVM(ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, gasLimit uint64, data []byte, commit bool) (*evmtypes.MsgEthereumTxResponse, error)
ExecuteEVM(ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, gasLimit uint64, data []byte) (*evmtypes.MsgEthereumTxResponse, error)
}

type CrosschainKeeper interface {
Expand Down