ante, evm: AnteHandler refactor (#517)

* Refactor fees function

* Implement suggestions

* wip test

* checksenderbalance tests

* wip deductcost test

* Tests fixes

* Merge master and fix conflicts

* Merging account and bank interfaces

* Update x/evm/keeper/utils.go

Co-authored-by: Hanchon <guillermo.paoletti@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

app/ante/ante.go

@@ -19,32 +19,20 @@ import (
 	ibcante "github.com/cosmos/ibc-go/modules/core/ante"
 
 	"github.com/tharsis/ethermint/crypto/ethsecp256k1"
+	evmtypes "github.com/tharsis/ethermint/x/evm/types"
 )
 
 const (
 	secp256k1VerifyCost uint64 = 21000
 )
 
-// AccountKeeper defines an expected keeper interface for the auth module's AccountKeeper
-type AccountKeeper interface {
-	authante.AccountKeeper
-	NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
-	GetSequence(sdk.Context, sdk.AccAddress) (uint64, error)
-}
-
-// BankKeeper defines an expected keeper interface for the bank module's Keeper
-type BankKeeper interface {
-	authtypes.BankKeeper
-	GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
-}
-
 // NewAnteHandler returns an ante handler responsible for attempting to route an
 // Ethereum or SDK transaction to an internal ante handler for performing
 // transaction-level processing (e.g. fee payment, signature verification) before
 // being passed onto it's respective handler.
 func NewAnteHandler(
-	ak AccountKeeper,
+	ak evmtypes.AccountKeeper,
-	bankKeeper BankKeeper,
+	bankKeeper evmtypes.BankKeeper,
 	evmKeeper EVMKeeper,
 	feeGrantKeeper authante.FeegrantKeeper,
 	channelKeeper channelkeeper.Keeper,

app/ante/eth.go

@@ -9,6 +9,7 @@ import (
 	"github.com/palantir/stacktrace"
 
 	ethermint "github.com/tharsis/ethermint/types"
+	evmkeeper "github.com/tharsis/ethermint/x/evm/keeper"
 	evmtypes "github.com/tharsis/ethermint/x/evm/types"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -89,13 +90,13 @@ func (esvd EthSigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s
 
 // EthAccountVerificationDecorator validates an account balance checks
 type EthAccountVerificationDecorator struct {
-	ak         AccountKeeper
+	ak         evmtypes.AccountKeeper
-	bankKeeper BankKeeper
+	bankKeeper evmtypes.BankKeeper
 	evmKeeper  EVMKeeper
 }
 
 // NewEthAccountVerificationDecorator creates a new EthAccountVerificationDecorator
-func NewEthAccountVerificationDecorator(ak AccountKeeper, bankKeeper BankKeeper, ek EVMKeeper) EthAccountVerificationDecorator {
+func NewEthAccountVerificationDecorator(ak evmtypes.AccountKeeper, bankKeeper evmtypes.BankKeeper, ek EVMKeeper) EthAccountVerificationDecorator {
 	return EthAccountVerificationDecorator{
 		ak:         ak,
 		bankKeeper: bankKeeper,
@@ -154,18 +155,8 @@ func (avd EthAccountVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx
 			avd.ak.SetAccount(ctx, acc)
 		}
 
-		// validate sender has enough funds to pay for tx cost
+		if err := evmkeeper.CheckSenderBalance(ctx, avd.bankKeeper, from, txData, evmDenom); err != nil {
-		balance := avd.bankKeeper.GetBalance(ctx, from, evmDenom)
+			return ctx, stacktrace.Propagate(err, "failed to check sender balance")
-		cost := txData.Cost()
-
-		if balance.Amount.BigInt().Cmp(cost) < 0 {
-			return ctx, stacktrace.Propagate(
-				sdkerrors.Wrapf(
-					sdkerrors.ErrInsufficientFunds,
-					"sender balance < tx cost (%s < %s%s)", balance, txData.Cost(), evmDenom,
-				),
-				"sender should have had enough funds to pay for tx cost = fee + amount (%s = %s + %s)", cost, txData.Fee(), txData.GetValue(),
-			)
 		}
 
 	}
@@ -177,11 +168,11 @@ func (avd EthAccountVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx
 // EthNonceVerificationDecorator checks that the account nonce from the transaction matches
 // the sender account sequence.
 type EthNonceVerificationDecorator struct {
-	ak AccountKeeper
+	ak evmtypes.AccountKeeper
 }
 
 // NewEthNonceVerificationDecorator creates a new EthNonceVerificationDecorator
-func NewEthNonceVerificationDecorator(ak AccountKeeper) EthNonceVerificationDecorator {
+func NewEthNonceVerificationDecorator(ak evmtypes.AccountKeeper) EthNonceVerificationDecorator {
 	return EthNonceVerificationDecorator{
 		ak: ak,
 	}
@@ -235,13 +226,13 @@ func (nvd EthNonceVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx,
 // EthGasConsumeDecorator validates enough intrinsic gas for the transaction and
 // gas consumption.
 type EthGasConsumeDecorator struct {
-	ak         AccountKeeper
+	ak         evmtypes.AccountKeeper
-	bankKeeper BankKeeper
+	bankKeeper evmtypes.BankKeeper
 	evmKeeper  EVMKeeper
 }
 
 // NewEthGasConsumeDecorator creates a new EthGasConsumeDecorator
-func NewEthGasConsumeDecorator(ak AccountKeeper, bankKeeper BankKeeper, ek EVMKeeper) EthGasConsumeDecorator {
+func NewEthGasConsumeDecorator(ak evmtypes.AccountKeeper, bankKeeper evmtypes.BankKeeper, ek EVMKeeper) EthGasConsumeDecorator {
 	return EthGasConsumeDecorator{
 		ak:         ak,
 		bankKeeper: bankKeeper,
@@ -274,6 +265,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 	blockHeight := big.NewInt(ctx.BlockHeight())
 	homestead := ethCfg.IsHomestead(blockHeight)
 	istanbul := ethCfg.IsIstanbul(blockHeight)
+	evmDenom := params.EvmDenom
 
 	var events sdk.Events
 
@@ -291,45 +283,19 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 			return ctx, stacktrace.Propagate(err, "failed to unpack tx data")
 		}
 
-		isContractCreation := txData.GetTo() == nil
+		fees, err := evmkeeper.DeductTxCostsFromUserBalance(
+			ctx,
+			egcd.bankKeeper,
+			egcd.ak,
+			*msgEthTx,
+			txData,
+			evmDenom,
+			homestead,
+			istanbul,
+		)
 
-		// fetch sender account from signature
-		signerAcc, err := authante.GetSignerAcc(ctx, egcd.ak, msgEthTx.GetFrom())
 		if err != nil {
-			return ctx, stacktrace.Propagate(err, "account not found for sender %s", msgEthTx.From)
+			return ctx, stacktrace.Propagate(err, "failed to deduct transaction costs from user balance")
-		}
-
-		gasLimit := txData.GetGas()
-
-		var accessList ethtypes.AccessList
-		if txData.GetAccessList() != nil {
-			accessList = txData.GetAccessList()
-		}
-
-		intrinsicGas, err := core.IntrinsicGas(txData.GetData(), accessList, isContractCreation, homestead, istanbul)
-		if err != nil {
-			return ctx, stacktrace.Propagate(
-				sdkerrors.Wrap(err, "failed to compute intrinsic gas cost"),
-				"failed to retrieve intrinsic gas, contract creation = %t; homestead = %t, istanbul = %t", isContractCreation, homestead, istanbul)
-		}
-
-		// intrinsic gas verification during CheckTx
-		if ctx.IsCheckTx() && gasLimit < intrinsicGas {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrOutOfGas, "gas limit too low: %d (gas limit) < %d (intrinsic gas)", gasLimit, intrinsicGas)
-		}
-
-		// calculate the fees paid to validators based on gas limit and price
-		feeAmt := txData.Fee() // fee = gas limit * gas price
-
-		evmDenom := egcd.evmKeeper.GetParams(ctx).EvmDenom
-		fees := sdk.Coins{sdk.NewCoin(evmDenom, sdk.NewIntFromBigInt(feeAmt))}
-
-		// deduct the full gas cost from the user balance
-		if err := authante.DeductFees(egcd.bankKeeper, ctx, signerAcc, fees); err != nil {
-			return ctx, stacktrace.Propagate(
-				err,
-				"failed to deduct full gas cost %s from the user %s balance", fees, msgEthTx.From,
-			)
 		}
 
 		events = append(events, sdk.NewEvent(sdk.EventTypeTx, sdk.NewAttribute(sdk.AttributeKeyFee, fees.String())))
@@ -477,11 +443,11 @@ func (ald AccessListDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
 
 // EthIncrementSenderSequenceDecorator increments the sequence of the signers.
 type EthIncrementSenderSequenceDecorator struct {
-	ak AccountKeeper
+	ak evmtypes.AccountKeeper
 }
 
 // NewEthIncrementSenderSequenceDecorator creates a new EthIncrementSenderSequenceDecorator.
-func NewEthIncrementSenderSequenceDecorator(ak AccountKeeper) EthIncrementSenderSequenceDecorator {
+func NewEthIncrementSenderSequenceDecorator(ak evmtypes.AccountKeeper) EthIncrementSenderSequenceDecorator {
 	return EthIncrementSenderSequenceDecorator{
 		ak: ak,
 	}

x/evm/keeper/utils.go

@@ -0,0 +1,97 @@
+package keeper
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
+	"github.com/palantir/stacktrace"
+
+	evmtypes "github.com/tharsis/ethermint/x/evm/types"
+
+	"github.com/ethereum/go-ethereum/core"
+	ethtypes "github.com/ethereum/go-ethereum/core/types"
+)
+
+// AccountKeeper defines an expected keeper interface for the auth module's AccountKeeper
+// DeductTxCostsFromUserBalance it calculates the tx costs and deducts the fees
+func DeductTxCostsFromUserBalance(
+	ctx sdk.Context,
+	bankKeeper evmtypes.BankKeeper,
+	accountKeeper evmtypes.AccountKeeper,
+	msgEthTx evmtypes.MsgEthereumTx,
+	txData evmtypes.TxData,
+	denom string,
+	homestead bool,
+	istanbul bool,
+) (sdk.Coins, error) {
+	isContractCreation := txData.GetTo() == nil
+
+	// fetch sender account from signature
+	signerAcc, err := authante.GetSignerAcc(ctx, accountKeeper, msgEthTx.GetFrom())
+	if err != nil {
+		return nil, stacktrace.Propagate(err, "account not found for sender %s", msgEthTx.From)
+	}
+
+	gasLimit := txData.GetGas()
+
+	var accessList ethtypes.AccessList
+	if txData.GetAccessList() != nil {
+		accessList = txData.GetAccessList()
+	}
+
+	intrinsicGas, err := core.IntrinsicGas(txData.GetData(), accessList, isContractCreation, homestead, istanbul)
+	if err != nil {
+		return nil, stacktrace.Propagate(sdkerrors.Wrap(
+			err,
+			"failed to compute intrinsic gas cost"), "failed to retrieve intrinsic gas, contract creation = %t; homestead = %t, istanbul = %t",
+			isContractCreation, homestead, istanbul,
+		)
+	}
+
+	// intrinsic gas verification during CheckTx
+	if ctx.IsCheckTx() && gasLimit < intrinsicGas {
+		return nil, sdkerrors.Wrapf(
+			sdkerrors.ErrOutOfGas,
+			"gas limit too low: %d (gas limit) < %d (intrinsic gas)", gasLimit, intrinsicGas,
+		)
+	}
+
+	// calculate the fees paid to validators based on gas limit and price
+	feeAmt := txData.Fee() // fee = gas limit * gas price
+
+	fees := sdk.Coins{sdk.NewCoin(denom, sdk.NewIntFromBigInt(feeAmt))}
+
+	// deduct the full gas cost from the user balance
+	if err := authante.DeductFees(bankKeeper, ctx, signerAcc, fees); err != nil {
+		return nil, stacktrace.Propagate(
+			err,
+			"failed to deduct full gas cost %s from the user %s balance",
+			fees, msgEthTx.From,
+		)
+	}
+	return fees, nil
+}
+
+// CheckSenderBalance validates sender has enough funds to pay for tx cost
+func CheckSenderBalance(
+	ctx sdk.Context,
+	bankKeeper evmtypes.BankKeeper,
+	sender sdk.AccAddress,
+	txData evmtypes.TxData,
+	denom string,
+) error {
+	balance := bankKeeper.GetBalance(ctx, sender, denom)
+	cost := txData.Cost()
+
+	if balance.Amount.BigInt().Cmp(cost) < 0 {
+		return stacktrace.Propagate(
+			sdkerrors.Wrapf(
+				sdkerrors.ErrInsufficientFunds,
+				"sender balance < tx cost (%s < %s%s)", balance, txData.Cost(), denom,
+			),
+			"sender should have had enough funds to pay for tx cost = fee + amount (%s = %s + %s)",
+			cost, txData.Fee(), txData.GetValue(),
+		)
+	}
+	return nil
+}

x/evm/keeper/utils_test.go

@@ -0,0 +1,257 @@
+package keeper_test
+
+import (
+	"math/big"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/ethereum/go-ethereum/common"
+	ethtypes "github.com/ethereum/go-ethereum/core/types"
+	evmkeeper "github.com/tharsis/ethermint/x/evm/keeper"
+	evmtypes "github.com/tharsis/ethermint/x/evm/types"
+)
+
+func (suite *KeeperTestSuite) TestCheckSenderBalance() {
+	hundredInt := sdk.NewInt(100)
+	zeroInt := sdk.ZeroInt()
+	oneInt := sdk.NewInt(1)
+	fiveInt := sdk.NewInt(5)
+	fiftyInt := sdk.NewInt(50)
+
+	testCases := []struct {
+		name       string
+		to         string
+		gasLimit   uint64
+		gasPrice   *sdk.Int
+		cost       *sdk.Int
+		from       string
+		accessList *ethtypes.AccessList
+		expectPass bool
+	}{
+		{
+			name:       "Enough balance",
+			to:         suite.address.String(),
+			gasLimit:   10,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Equal balance",
+			to:         suite.address.String(),
+			gasLimit:   99,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Higher gas limit, not enough balance",
+			to:         suite.address.String(),
+			gasLimit:   100,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: false,
+		},
+		{
+			name:       "Higher gas price, enough balance",
+			to:         suite.address.String(),
+			gasLimit:   10,
+			gasPrice:   &fiveInt,
+			cost:       &oneInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Higher gas price, not enough balance",
+			to:         suite.address.String(),
+			gasLimit:   20,
+			gasPrice:   &fiveInt,
+			cost:       &oneInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: false,
+		},
+		{
+			name:       "Higher cost, enough balance",
+			to:         suite.address.String(),
+			gasLimit:   10,
+			gasPrice:   &fiveInt,
+			cost:       &fiftyInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Higher cost, not enough balance",
+			to:         suite.address.String(),
+			gasLimit:   10,
+			gasPrice:   &fiveInt,
+			cost:       &hundredInt,
+			from:       suite.address.String(),
+			accessList: &ethtypes.AccessList{},
+			expectPass: false,
+		},
+	}
+
+	suite.app.EvmKeeper.AddBalance(suite.address, hundredInt.BigInt())
+	balance := suite.app.EvmKeeper.GetBalance(suite.address)
+	suite.Require().Equal(balance, hundredInt.BigInt())
+
+	for i, tc := range testCases {
+		suite.Run(tc.name, func() {
+			to := common.HexToAddress(tc.from)
+
+			var amount, gasPrice *big.Int
+			if tc.cost != nil {
+				amount = tc.cost.BigInt()
+			}
+			if tc.gasPrice != nil {
+				gasPrice = tc.gasPrice.BigInt()
+			}
+
+			tx := evmtypes.NewTx(zeroInt.BigInt(), 1, &to, amount, tc.gasLimit, gasPrice, nil, tc.accessList)
+			tx.From = tc.from
+
+			txData, _ := evmtypes.UnpackTxData(tx.Data)
+
+			err := evmkeeper.CheckSenderBalance(
+				suite.app.EvmKeeper.Ctx(),
+				suite.app.BankKeeper,
+				suite.address[:],
+				txData,
+				evmtypes.DefaultEVMDenom,
+			)
+
+			if tc.expectPass {
+				suite.Require().NoError(err, "valid test %d failed", i)
+			} else {
+				suite.Require().Error(err, "invalid test %d passed", i)
+			}
+
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
+	hundredInt := sdk.NewInt(100)
+	zeroInt := sdk.ZeroInt()
+	oneInt := sdk.NewInt(1)
+	fiveInt := sdk.NewInt(5)
+	fiftyInt := sdk.NewInt(50)
+
+	testCases := []struct {
+		name       string
+		gasLimit   uint64
+		gasPrice   *sdk.Int
+		cost       *sdk.Int
+		accessList *ethtypes.AccessList
+		expectPass bool
+	}{
+		{
+			name:       "Enough balance",
+			gasLimit:   10,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Equal balance",
+			gasLimit:   100,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Higher gas limit, not enough balance",
+			gasLimit:   105,
+			gasPrice:   &oneInt,
+			cost:       &oneInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: false,
+		},
+		{
+			name:       "Higher gas price, enough balance",
+			gasLimit:   20,
+			gasPrice:   &fiveInt,
+			cost:       &oneInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+		{
+			name:       "Higher gas price, not enough balance",
+			gasLimit:   20,
+			gasPrice:   &fiftyInt,
+			cost:       &oneInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: false,
+		},
+		// This case is expected to be true because the fees can be deducted, but the tx
+		// execution is going to fail because there is no more balance to pay the cost
+		{
+			name:       "Higher cost, enough balance",
+			gasLimit:   100,
+			gasPrice:   &oneInt,
+			cost:       &fiftyInt,
+			accessList: &ethtypes.AccessList{},
+			expectPass: true,
+		},
+	}
+
+	for i, tc := range testCases {
+
+		suite.Run(tc.name, func() {
+			suite.SetupTest()
+			suite.app.EvmKeeper.AddBalance(suite.address, hundredInt.BigInt())
+			balance := suite.app.EvmKeeper.GetBalance(suite.address)
+			suite.Require().Equal(balance, hundredInt.BigInt())
+
+			var amount, gasPrice *big.Int
+			if tc.cost != nil {
+				amount = tc.cost.BigInt()
+			}
+			if tc.gasPrice != nil {
+				gasPrice = tc.gasPrice.BigInt()
+			}
+
+			tx := evmtypes.NewTx(zeroInt.BigInt(), 1, &suite.address, amount, tc.gasLimit, gasPrice, nil, tc.accessList)
+			tx.From = suite.address.String()
+
+			txData, _ := evmtypes.UnpackTxData(tx.Data)
+
+			fees, err := evmkeeper.DeductTxCostsFromUserBalance(
+				suite.app.EvmKeeper.Ctx(),
+				suite.app.BankKeeper,
+				suite.app.AccountKeeper,
+				*tx,
+				txData,
+				evmtypes.DefaultEVMDenom,
+				false,
+				false,
+			)
+
+			if tc.expectPass {
+				suite.Require().NoError(err, "valid test %d failed", i)
+				suite.Require().Equal(
+					fees,
+					sdk.NewCoins(
+						sdk.NewCoin(evmtypes.DefaultEVMDenom, tc.gasPrice.Mul(sdk.NewIntFromUint64(tc.gasLimit))),
+					),
+					"valid test %d failed, fee value is wrong ", i,
+				)
+
+			} else {
+				suite.Require().Error(err, "invalid test %d passed", i)
+				suite.Require().Nil(fees, "invalid test %d passed. fees value must be nil", i)
+			}
+
+		})
+	}
+}

x/evm/types/interfaces.go

@@ -18,6 +18,7 @@ type AccountKeeper interface {
 	GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
 	SetAccount(ctx sdk.Context, account authtypes.AccountI)
 	RemoveAccount(ctx sdk.Context, account authtypes.AccountI)
+	GetParams(ctx sdk.Context) (params authtypes.Params)
 }
 
 // BankKeeper defines the expected interface needed to retrieve account balances.