chore: verify fees refactor (#1496)

* chore: verify fees refactor

* adjust call structure in rest of repo after splitting up DeductTxCostsFromUserBalance

* adjust test logic after splitting DeductTxCostsFromUserBalance up

* remove outdated TODO

* address PR comments - remove import name for evm keeper

* remove misleading comment

* address review comments - only handover boolean instead of context

* remove TODO

Co-authored-by: MalteHerrmann <malteherrmann.mail@web.de>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

app/ante/eth.go

@@ -11,7 +11,7 @@ import (
 	errortypes "github.com/cosmos/cosmos-sdk/types/errors"
 
 	ethermint "github.com/evmos/ethermint/types"
-	evmkeeper "github.com/evmos/ethermint/x/evm/keeper"
+	"github.com/evmos/ethermint/x/evm/keeper"
 	"github.com/evmos/ethermint/x/evm/statedb"
 	evmtypes "github.com/evmos/ethermint/x/evm/types"
 
@@ -79,7 +79,7 @@ func (avd EthAccountVerificationDecorator) AnteHandle(
 				"the sender is not EOA: address %s, codeHash <%s>", fromAddr, acct.CodeHash)
 		}
 
-		if err := evmkeeper.CheckSenderBalance(sdkmath.NewIntFromBigInt(acct.Balance), txData); err != nil {
+		if err := keeper.CheckSenderBalance(sdkmath.NewIntFromBigInt(acct.Balance), txData); err != nil {
 			return ctx, errorsmod.Wrap(err, "failed to check sender balance")
 		}
 	}
@@ -132,7 +132,6 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 	blockHeight := big.NewInt(ctx.BlockHeight())
 	homestead := ethCfg.IsHomestead(blockHeight)
 	istanbul := ethCfg.IsIstanbul(blockHeight)
-	london := ethCfg.IsLondon(blockHeight)
 	gasWanted := uint64(0)
 	var events sdk.Events
 
@@ -164,16 +163,12 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 
 		evmDenom := egcd.evmKeeper.GetEVMDenom(ctx)
 
-		fees, err := egcd.evmKeeper.DeductTxCostsFromUserBalance(
+		fees, err := keeper.VerifyFee(txData, evmDenom, baseFee, homestead, istanbul, ctx.IsCheckTx())
-			ctx,
+		if err != nil {
-			*msgEthTx,
+			return ctx, errorsmod.Wrapf(err, "failed to verify the fees")
-			txData,
+		}
-			evmDenom,
+
-			baseFee,
+		err = egcd.evmKeeper.DeductTxCostsFromUserBalance(ctx, fees, common.HexToAddress(msgEthTx.From))
-			homestead,
-			istanbul,
-			london,
-		)
 		if err != nil {
 			return ctx, errorsmod.Wrapf(err, "failed to deduct transaction costs from user balance")
 		}

app/ante/interfaces.go

@@ -28,9 +28,7 @@ type EVMKeeper interface {
 	DynamicFeeEVMKeeper
 
 	NewEVM(ctx sdk.Context, msg core.Message, cfg *evmtypes.EVMConfig, tracer vm.EVMLogger, stateDB vm.StateDB) evm.EVM
-	DeductTxCostsFromUserBalance(
+	DeductTxCostsFromUserBalance(ctx sdk.Context, fees sdk.Coins, from common.Address) error
-		ctx sdk.Context, msgEthTx evmtypes.MsgEthereumTx, txData evmtypes.TxData, denom string, baseFee *big.Int, homestead, istanbul, london bool,
-	) (fees sdk.Coins, err error)
 	GetBalance(ctx sdk.Context, addr common.Address) *big.Int
 	ResetTransientGasUsed(ctx sdk.Context)
 	GetTxIndexTransient(ctx sdk.Context) uint64

x/evm/handler_test.go

@@ -2,6 +2,7 @@ package evm_test
 
 import (
 	"errors"
+	"github.com/evmos/ethermint/x/evm/keeper"
 	"math/big"
 	"testing"
 	"time"
@@ -599,7 +600,9 @@ func (suite *EvmTestSuite) TestERC20TransferReverted() {
 
 			txData, err := types.UnpackTxData(tx.Data)
 			suite.Require().NoError(err)
-			_, err = k.DeductTxCostsFromUserBalance(suite.ctx, *tx, txData, "aphoton", baseFee, true, true, true)
+			fees, err := keeper.VerifyFee(txData, "aphoton", baseFee, true, true, suite.ctx.IsCheckTx())
+			suite.Require().NoError(err)
+			err = k.DeductTxCostsFromUserBalance(suite.ctx, fees, common.HexToAddress(tx.From))
 			suite.Require().NoError(err)
 
 			res, err := k.EthereumTx(sdk.WrapSDKContext(suite.ctx), tx)

x/evm/keeper/utils.go

@@ -11,19 +11,41 @@ import (
 
 	evmtypes "github.com/evmos/ethermint/x/evm/types"
 
+	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 )
 
-// DeductTxCostsFromUserBalance it calculates the tx costs and deducts the fees
+// DeductTxCostsFromUserBalance deducts the fees from the user balance. Returns an
-func (k Keeper) DeductTxCostsFromUserBalance(
+// error if the specified sender address does not exist or the account balance is not sufficient.
+func (k *Keeper) DeductTxCostsFromUserBalance(
 	ctx sdk.Context,
-	msgEthTx evmtypes.MsgEthereumTx,
+	fees sdk.Coins,
+	from common.Address,
+) error {
+	// fetch sender account
+	signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, from.Bytes())
+	if err != nil {
+		return errorsmod.Wrapf(err, "account not found for sender %s", from)
+	}
+
+	// deduct the full gas cost from the user balance
+	if err := authante.DeductFees(k.bankKeeper, ctx, signerAcc, fees); err != nil {
+		return errorsmod.Wrapf(err, "failed to deduct full gas cost %s from the user %s balance", fees, from)
+	}
+
+	return nil
+}
+
+// VerifyFee is used to return the fee for the given transaction data in sdk.Coins. It checks that the
+// gas limit is not reached, the gas limit is higher than the intrinsic gas and that the
+// base fee is higher than the gas fee cap.
+func VerifyFee(
 	txData evmtypes.TxData,
 	denom string,
 	baseFee *big.Int,
-	homestead, istanbul, london bool,
+	homestead, istanbul, isCheckTx bool,
-) (fees sdk.Coins, err error) {
+) (sdk.Coins, error) {
 	isContractCreation := txData.GetTo() == nil
 
 	gasLimit := txData.GetGas()
@@ -43,7 +65,7 @@ func (k Keeper) DeductTxCostsFromUserBalance(
 	}
 
 	// intrinsic gas verification during CheckTx
-	if ctx.IsCheckTx() && gasLimit < intrinsicGas {
+	if isCheckTx && gasLimit < intrinsicGas {
 		return nil, errorsmod.Wrapf(
 			errortypes.ErrOutOfGas,
 			"gas limit too low: %d (gas limit) < %d (intrinsic gas)", gasLimit, intrinsicGas,
@@ -63,24 +85,7 @@ func (k Keeper) DeductTxCostsFromUserBalance(
 		return sdk.Coins{}, nil
 	}
 
-	fees = sdk.Coins{{Denom: denom, Amount: sdkmath.NewIntFromBigInt(feeAmt)}}
+	return sdk.Coins{{Denom: denom, Amount: sdkmath.NewIntFromBigInt(feeAmt)}}, nil
-
-	// fetch sender account from signature
-	signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, msgEthTx.GetFrom())
-	if err != nil {
-		return nil, errorsmod.Wrapf(err, "account not found for sender %s", msgEthTx.From)
-	}
-
-	// deduct the full gas cost from the user balance
-	if err := authante.DeductFees(k.bankKeeper, ctx, signerAcc, fees); err != nil {
-		return nil, errorsmod.Wrapf(
-			err,
-			"failed to deduct full gas cost %s from the user %s balance",
-			fees, msgEthTx.From,
-		)
-	}
-
-	return fees, nil
 }
 
 // CheckSenderBalance validates that the tx cost value is positive and that the

x/evm/keeper/utils_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	ethparams "github.com/ethereum/go-ethereum/params"
-	evmkeeper "github.com/evmos/ethermint/x/evm/keeper"
+	"github.com/evmos/ethermint/x/evm/keeper"
 	evmtypes "github.com/evmos/ethermint/x/evm/types"
 )
 
@@ -207,7 +207,8 @@ func (suite *KeeperTestSuite) TestCheckSenderBalance() {
 	vmdb.AddBalance(suite.address, hundredInt.BigInt())
 	balance := vmdb.GetBalance(suite.address)
 	suite.Require().Equal(balance, hundredInt.BigInt())
-	vmdb.Commit()
+	err := vmdb.Commit()
+	suite.Require().NoError(err, "Unexpected error while committing to vmdb: %d", err)
 
 	for i, tc := range testCases {
 		suite.Run(tc.name, func() {
@@ -237,7 +238,7 @@ func (suite *KeeperTestSuite) TestCheckSenderBalance() {
 			txData, _ := evmtypes.UnpackTxData(tx.Data)
 
 			acct := suite.app.EvmKeeper.GetAccountOrEmpty(suite.ctx, suite.address)
-			err := evmkeeper.CheckSenderBalance(
+			err := keeper.CheckSenderBalance(
 				sdkmath.NewIntFromBigInt(acct.Balance),
 				txData,
 			)
@@ -251,7 +252,13 @@ func (suite *KeeperTestSuite) TestCheckSenderBalance() {
 	}
 }
 
-func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
+// TestVerifyFeeAndDeductTxCostsFromUserBalance is a test method for both the VerifyFee
+// function and the DeductTxCostsFromUserBalance method.
+//
+// NOTE: This method combines testing for both functions, because these used to be
+// in one function and share a lot of the same setup.
+// In practice, the two tested functions will also be sequentially executed.
+func (suite *KeeperTestSuite) TestVerifyFeeAndDeductTxCostsFromUserBalance() {
 	hundredInt := sdkmath.NewInt(100)
 	zeroInt := sdk.ZeroInt()
 	oneInt := sdkmath.NewInt(1)
@@ -269,7 +276,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 		gasTipCap        *big.Int
 		cost             *sdkmath.Int
 		accessList       *ethtypes.AccessList
-		expectPass      bool
+		expectPassVerify bool
+		expectPassDeduct bool
 		enableFeemarket  bool
 		from             string
 		malleate         func()
@@ -280,7 +288,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 		},
 		{
@@ -289,7 +298,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 		},
 		{
@@ -298,7 +308,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: false,
+			expectPassVerify: true,
+			expectPassDeduct: false,
 			from:             suite.address.String(),
 		},
 		{
@@ -307,7 +318,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &fiveInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 		},
 		{
@@ -316,7 +328,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &fiftyInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: false,
+			expectPassVerify: true,
+			expectPassDeduct: false,
 			from:             suite.address.String(),
 		},
 		// This case is expected to be true because the fees can be deducted, but the tx
@@ -327,7 +340,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &fiftyInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 		},
 		//  testcases with enableFeemarket enabled.
@@ -338,7 +352,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasTipCap:        big.NewInt(1),
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass:      false,
+			expectPassVerify: false,
+			expectPassDeduct: true,
 			enableFeemarket:  true,
 			from:             suite.address.String(),
 		},
@@ -349,7 +364,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasTipCap:        big.NewInt(1),
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass:      true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			enableFeemarket:  true,
 			from:             suite.address.String(),
 		},
@@ -359,7 +375,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasFeeCap:        big.NewInt(ethparams.InitialBaseFee + 2),
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass:      true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			enableFeemarket:  true,
 			from:             suite.address.String(),
 		},
@@ -370,7 +387,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasTipCap:        big.NewInt(2),
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass:      true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			enableFeemarket:  true,
 			from:             suite.address.String(),
 		},
@@ -380,8 +398,9 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: false,
+			expectPassVerify: true,
-			from:       "",
+			expectPassDeduct: false,
+			from:             "abcdef",
 		},
 		{
 			name:     "Enough balance - with access list",
@@ -394,7 +413,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 					StorageKeys: []common.Hash{},
 				},
 			},
-			expectPass: true,
+			expectPassVerify: true,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 		},
 		{
@@ -403,7 +423,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			gasPrice:         &oneInt,
 			cost:             &oneInt,
 			accessList:       &ethtypes.AccessList{},
-			expectPass: false,
+			expectPassVerify: false,
+			expectPassDeduct: true,
 			from:             suite.address.String(),
 			malleate: func() {
 				suite.ctx = suite.ctx.WithIsCheckTx(true)
@@ -446,7 +467,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 				balance := vmdb.GetBalance(suite.address)
 				suite.Require().Equal(balance, hundredInt.BigInt())
 			}
-			vmdb.Commit()
+			err := vmdb.Commit()
+			suite.Require().NoError(err, "Unexpected error while committing to vmdb: %d", err)
 
 			tx := evmtypes.NewTx(zeroInt.BigInt(), 1, &suite.address, amount, tc.gasLimit, gasPrice, gasFeeCap, gasTipCap, nil, tc.accessList)
 			tx.From = tc.from
@@ -457,19 +479,9 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 			baseFee := suite.app.EvmKeeper.GetBaseFee(suite.ctx, ethCfg)
 			priority := evmtypes.GetTxPriority(txData, baseFee)
 
-			fees, err := suite.app.EvmKeeper.DeductTxCostsFromUserBalance(
+			fees, err := keeper.VerifyFee(txData, evmtypes.DefaultEVMDenom, baseFee, false, false, suite.ctx.IsCheckTx())
-				suite.ctx,
+			if tc.expectPassVerify {
-				*tx,
+				suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name)
-				txData,
-				evmtypes.DefaultEVMDenom,
-				baseFee,
-				false,
-				false,
-				suite.enableFeemarket, // london
-			)
-
-			if tc.expectPass {
-				suite.Require().NoError(err, "valid test %d failed", i)
 				if tc.enableFeemarket {
 					baseFee := suite.app.FeeMarketKeeper.GetBaseFee(suite.ctx)
 					suite.Require().Equal(
@@ -477,7 +489,7 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 						sdk.NewCoins(
 							sdk.NewCoin(evmtypes.DefaultEVMDenom, sdkmath.NewIntFromBigInt(txData.EffectiveFee(baseFee))),
 						),
-						"valid test %d failed, fee value is wrong ", i,
+						"valid test %d failed, fee value is wrong  - '%s'", i, tc.name,
 					)
 					suite.Require().Equal(int64(0), priority)
 				} else {
@@ -486,12 +498,19 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() {
 						sdk.NewCoins(
 							sdk.NewCoin(evmtypes.DefaultEVMDenom, tc.gasPrice.Mul(sdkmath.NewIntFromUint64(tc.gasLimit))),
 						),
-						"valid test %d failed, fee value is wrong ", i,
+						"valid test %d failed, fee value is wrong  - '%s'", i, tc.name,
 					)
 				}
 			} else {
-				suite.Require().Error(err, "invalid test %d passed", i)
+				suite.Require().Error(err, "invalid test %d passed - '%s'", i, tc.name)
-				suite.Require().Nil(fees, "invalid test %d passed. fees value must be nil", i)
+				suite.Require().Nil(fees, "invalid test %d passed. fees value must be nil - '%s'", i, tc.name)
+			}
+
+			err = suite.app.EvmKeeper.DeductTxCostsFromUserBalance(suite.ctx, fees, common.HexToAddress(tx.From))
+			if tc.expectPassDeduct {
+				suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name)
+			} else {
+				suite.Require().Error(err, "invalid test %d passed - '%s'", i, tc.name)
 			}
 		})
 	}