From 718dbc81625d4b42cc52fc52f981bb9c5b39d36e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Fri, 25 Nov 2022 11:19:14 +0100 Subject: [PATCH 1/8] chore: verify fees refactor --- x/evm/keeper/utils.go | 48 +++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index beffb5e883..762b6046d6 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -11,6 +11,7 @@ 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" ) @@ -18,12 +19,36 @@ import ( // DeductTxCostsFromUserBalance it calculates the tx costs and deducts the fees func (k Keeper) DeductTxCostsFromUserBalance( ctx sdk.Context, - msgEthTx evmtypes.MsgEthereumTx, + from common.Address, txData evmtypes.TxData, denom string, baseFee *big.Int, - homestead, istanbul, london bool, ) (fees sdk.Coins, err error) { + // fetch sender account from signature + signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, sdk.AccAddress(from.Bytes())) + if err != nil { + return nil, 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 nil, errorsmod.Wrapf( + err, + "failed to deduct full gas cost %s from the user %s balance", + fees, from, + ) + } + + return fees, nil +} + +func VerifyFee( + ctx sdk.Context, + txData evmtypes.TxData, + denom string, + baseFee *big.Int, + homestead, istanbul bool, +) (sdk.Coins, error) { isContractCreation := txData.GetTo() == nil gasLimit := txData.GetGas() @@ -63,24 +88,7 @@ func (k Keeper) DeductTxCostsFromUserBalance( return sdk.Coins{}, nil } - fees = sdk.Coins{{Denom: denom, Amount: sdkmath.NewIntFromBigInt(feeAmt)}} - - // 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 + return sdk.Coins{{Denom: denom, Amount: sdkmath.NewIntFromBigInt(feeAmt)}}, nil } // CheckSenderBalance validates that the tx cost value is positive and that the From 57ca7b13101c10286e0706d2e91c62b166905613 Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Fri, 25 Nov 2022 15:01:56 +0100 Subject: [PATCH 2/8] adjust call structure in rest of repo after splitting up DeductTxCostsFromUserBalance --- app/ante/eth.go | 17 ++++++----------- app/ante/interfaces.go | 4 +--- x/evm/handler_test.go | 8 +++++++- x/evm/keeper/utils.go | 23 ++++++++++------------- x/evm/keeper/utils_test.go | 29 +++++++++++++---------------- 5 files changed, 37 insertions(+), 44 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 684dd8f96b..c4fdad6779 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -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( - ctx, - *msgEthTx, - txData, - evmDenom, - baseFee, - homestead, - istanbul, - london, - ) + fees, err := evmkeeper.VerifyFee(ctx, txData, evmDenom, baseFee, homestead, istanbul) + if err != nil { + return ctx, errorsmod.Wrapf(err, "failed to verify the fees") + } + + err = egcd.evmKeeper.DeductTxCostsFromUserBalance(ctx, fees, common.HexToAddress(msgEthTx.From)) if err != nil { return ctx, errorsmod.Wrapf(err, "failed to deduct transaction costs from user balance") } diff --git a/app/ante/interfaces.go b/app/ante/interfaces.go index b0d62039e4..3619b223df 100644 --- a/app/ante/interfaces.go +++ b/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( - ctx sdk.Context, msgEthTx evmtypes.MsgEthereumTx, txData evmtypes.TxData, denom string, baseFee *big.Int, homestead, istanbul, london bool, - ) (fees sdk.Coins, err error) + DeductTxCostsFromUserBalance(ctx sdk.Context, fees sdk.Coins, from common.Address) error GetBalance(ctx sdk.Context, addr common.Address) *big.Int ResetTransientGasUsed(ctx sdk.Context) GetTxIndexTransient(ctx sdk.Context) uint64 diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index 55ae77a935..8515432d24 100644 --- a/x/evm/handler_test.go +++ b/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,8 +600,13 @@ 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(suite.ctx, txData, "aphoton", baseFee, true, true) suite.Require().NoError(err) + err = k.DeductTxCostsFromUserBalance(suite.ctx, fees, common.HexToAddress(tx.From)) + suite.Require().NoError(err) + // TODO: Remove this once the test is working + //_, err = k.DeductTxCostsFromUserBalance(suite.ctx, *tx, txData, "aphoton", baseFee, true, true, true) + //suite.Require().NoError(err) res, err := k.EthereumTx(sdk.WrapSDKContext(suite.ctx), tx) suite.Require().NoError(err) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index 762b6046d6..93f20e9156 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -16,32 +16,29 @@ import ( 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. func (k Keeper) DeductTxCostsFromUserBalance( ctx sdk.Context, + fees sdk.Coins, from common.Address, - txData evmtypes.TxData, - denom string, - baseFee *big.Int, -) (fees sdk.Coins, err error) { +) error { // fetch sender account from signature - signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, sdk.AccAddress(from.Bytes())) + signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, from.Bytes()) if err != nil { - return nil, errorsmod.Wrapf(err, "account not found for sender %s", from) + 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 nil, errorsmod.Wrapf( - err, - "failed to deduct full gas cost %s from the user %s balance", - fees, from, - ) + return errorsmod.Wrapf(err, "failed to deduct full gas cost %s from the user %s balance", fees, from) } - return fees, nil + 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( ctx sdk.Context, txData evmtypes.TxData, diff --git a/x/evm/keeper/utils_test.go b/x/evm/keeper/utils_test.go index 00a52a73bc..b697e8dac4 100644 --- a/x/evm/keeper/utils_test.go +++ b/x/evm/keeper/utils_test.go @@ -457,19 +457,9 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() { baseFee := suite.app.EvmKeeper.GetBaseFee(suite.ctx, ethCfg) priority := evmtypes.GetTxPriority(txData, baseFee) - fees, err := suite.app.EvmKeeper.DeductTxCostsFromUserBalance( - suite.ctx, - *tx, - txData, - evmtypes.DefaultEVMDenom, - baseFee, - false, - false, - suite.enableFeemarket, // london - ) - + fees, err := evmkeeper.VerifyFee(suite.ctx, txData, evmtypes.DefaultEVMDenom, baseFee, false, false) if tc.expectPass { - suite.Require().NoError(err, "valid test %d failed", i) + suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name) if tc.enableFeemarket { baseFee := suite.app.FeeMarketKeeper.GetBaseFee(suite.ctx) suite.Require().Equal( @@ -477,7 +467,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 +476,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().Nil(fees, "invalid test %d passed. fees value must be nil", 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 - '%s'", i, tc.name) + } + + err = suite.app.EvmKeeper.DeductTxCostsFromUserBalance(suite.ctx, fees, suite.address) + if tc.expectPass { + 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) } }) } From ca944e3baebe1a6f48ef3417d1f8a0c767485268 Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Sun, 27 Nov 2022 16:57:17 +0100 Subject: [PATCH 3/8] adjust test logic after splitting DeductTxCostsFromUserBalance up --- x/evm/keeper/utils.go | 7 +- x/evm/keeper/utils_test.go | 242 ++++++++++++++++++++----------------- 2 files changed, 137 insertions(+), 112 deletions(-) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index 93f20e9156..54c6f58b18 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -16,8 +16,11 @@ import ( ethtypes "github.com/ethereum/go-ethereum/core/types" ) -// DeductTxCostsFromUserBalance deducts the fees from the user balance. -func (k Keeper) DeductTxCostsFromUserBalance( +// DeductTxCostsFromUserBalance deducts the fees from the user balance. Returns an +// error if the specified sender address does not exist or the account balance is not sufficient. +// +// TODO: Currently, Goland raises the warning that there are methods defined on both *keeper and keeper, which is not recommended: https://go.dev/doc/faq#methods_on_values_or_pointers +func (k *Keeper) DeductTxCostsFromUserBalance( ctx sdk.Context, fees sdk.Coins, from common.Address, diff --git a/x/evm/keeper/utils_test.go b/x/evm/keeper/utils_test.go index b697e8dac4..034a8d96aa 100644 --- a/x/evm/keeper/utils_test.go +++ b/x/evm/keeper/utils_test.go @@ -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() { @@ -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) @@ -262,126 +269,138 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() { initBalance := sdkmath.NewInt((ethparams.InitialBaseFee + 10) * 105) testCases := []struct { - name string - gasLimit uint64 - gasPrice *sdkmath.Int - gasFeeCap *big.Int - gasTipCap *big.Int - cost *sdkmath.Int - accessList *ethtypes.AccessList - expectPass bool - enableFeemarket bool - from string - malleate func() + name string + gasLimit uint64 + gasPrice *sdkmath.Int + gasFeeCap *big.Int + gasTipCap *big.Int + cost *sdkmath.Int + accessList *ethtypes.AccessList + expectPassVerify bool + expectPassDeduct bool + enableFeemarket bool + from string + malleate func() }{ { - name: "Enough balance", - gasLimit: 10, - gasPrice: &oneInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - from: suite.address.String(), + name: "Enough balance", + gasLimit: 10, + gasPrice: &oneInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + from: suite.address.String(), }, { - name: "Equal balance", - gasLimit: 100, - gasPrice: &oneInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - from: suite.address.String(), + name: "Equal balance", + gasLimit: 100, + gasPrice: &oneInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + from: suite.address.String(), }, { - name: "Higher gas limit, not enough balance", - gasLimit: 105, - gasPrice: &oneInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: false, - from: suite.address.String(), + name: "Higher gas limit, not enough balance", + gasLimit: 105, + gasPrice: &oneInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: false, + from: suite.address.String(), }, { - name: "Higher gas price, enough balance", - gasLimit: 20, - gasPrice: &fiveInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - from: suite.address.String(), + name: "Higher gas price, enough balance", + gasLimit: 20, + gasPrice: &fiveInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + from: suite.address.String(), }, { - name: "Higher gas price, not enough balance", - gasLimit: 20, - gasPrice: &fiftyInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: false, - from: suite.address.String(), + name: "Higher gas price, not enough balance", + gasLimit: 20, + gasPrice: &fiftyInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: false, + from: suite.address.String(), }, // 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: ðtypes.AccessList{}, - expectPass: true, - from: suite.address.String(), + name: "Higher cost, enough balance", + gasLimit: 100, + gasPrice: &oneInt, + cost: &fiftyInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + from: suite.address.String(), }, // testcases with enableFeemarket enabled. { - name: "Invalid gasFeeCap w/ enableFeemarket", - gasLimit: 10, - gasFeeCap: big.NewInt(1), - gasTipCap: big.NewInt(1), - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: false, - enableFeemarket: true, - from: suite.address.String(), + name: "Invalid gasFeeCap w/ enableFeemarket", + gasLimit: 10, + gasFeeCap: big.NewInt(1), + gasTipCap: big.NewInt(1), + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: false, + expectPassDeduct: true, + enableFeemarket: true, + from: suite.address.String(), }, { - name: "empty tip fee is valid to deduct", - gasLimit: 10, - gasFeeCap: big.NewInt(ethparams.InitialBaseFee), - gasTipCap: big.NewInt(1), - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - enableFeemarket: true, - from: suite.address.String(), + name: "empty tip fee is valid to deduct", + gasLimit: 10, + gasFeeCap: big.NewInt(ethparams.InitialBaseFee), + gasTipCap: big.NewInt(1), + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + enableFeemarket: true, + from: suite.address.String(), }, { - name: "effectiveTip equal to gasTipCap", - gasLimit: 100, - gasFeeCap: big.NewInt(ethparams.InitialBaseFee + 2), - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - enableFeemarket: true, - from: suite.address.String(), + name: "effectiveTip equal to gasTipCap", + gasLimit: 100, + gasFeeCap: big.NewInt(ethparams.InitialBaseFee + 2), + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + enableFeemarket: true, + from: suite.address.String(), }, { - name: "effectiveTip equal to (gasFeeCap - baseFee)", - gasLimit: 105, - gasFeeCap: big.NewInt(ethparams.InitialBaseFee + 1), - gasTipCap: big.NewInt(2), - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: true, - enableFeemarket: true, - from: suite.address.String(), + name: "effectiveTip equal to (gasFeeCap - baseFee)", + gasLimit: 105, + gasFeeCap: big.NewInt(ethparams.InitialBaseFee + 1), + gasTipCap: big.NewInt(2), + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: true, + enableFeemarket: true, + from: suite.address.String(), }, { - name: "Invalid from address", - gasLimit: 10, - gasPrice: &oneInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: false, - from: "", + name: "Invalid from address", + gasLimit: 10, + gasPrice: &oneInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + expectPassVerify: true, + expectPassDeduct: false, + from: "abcdef", }, { name: "Enough balance - with access list", @@ -394,17 +413,19 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() { StorageKeys: []common.Hash{}, }, }, - expectPass: true, - from: suite.address.String(), + expectPassVerify: true, + expectPassDeduct: true, + from: suite.address.String(), }, { - name: "gasLimit < intrinsicGas during IsCheckTx", - gasLimit: 1, - gasPrice: &oneInt, - cost: &oneInt, - accessList: ðtypes.AccessList{}, - expectPass: false, - from: suite.address.String(), + name: "gasLimit < intrinsicGas during IsCheckTx", + gasLimit: 1, + gasPrice: &oneInt, + cost: &oneInt, + accessList: ðtypes.AccessList{}, + 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 @@ -458,7 +480,7 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() { priority := evmtypes.GetTxPriority(txData, baseFee) fees, err := evmkeeper.VerifyFee(suite.ctx, txData, evmtypes.DefaultEVMDenom, baseFee, false, false) - if tc.expectPass { + if tc.expectPassVerify { suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name) if tc.enableFeemarket { baseFee := suite.app.FeeMarketKeeper.GetBaseFee(suite.ctx) @@ -484,8 +506,8 @@ func (suite *KeeperTestSuite) TestDeductTxCostsFromUserBalance() { 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, suite.address) - if tc.expectPass { + 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) From fc006b1b636c19f7e7bf3021611a310729e17ef3 Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Sun, 27 Nov 2022 17:04:46 +0100 Subject: [PATCH 4/8] remove outdated TODO --- x/evm/handler_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index 8515432d24..b05c075190 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -604,9 +604,6 @@ func (suite *EvmTestSuite) TestERC20TransferReverted() { suite.Require().NoError(err) err = k.DeductTxCostsFromUserBalance(suite.ctx, fees, common.HexToAddress(tx.From)) suite.Require().NoError(err) - // TODO: Remove this once the test is working - //_, err = k.DeductTxCostsFromUserBalance(suite.ctx, *tx, txData, "aphoton", baseFee, true, true, true) - //suite.Require().NoError(err) res, err := k.EthereumTx(sdk.WrapSDKContext(suite.ctx), tx) suite.Require().NoError(err) From 0ec8ec0c0fd2192bfdfab28b0fc30d64ba545ee1 Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Tue, 29 Nov 2022 11:10:27 +0100 Subject: [PATCH 5/8] address PR comments - remove import name for evm keeper --- app/ante/eth.go | 6 +++--- x/evm/keeper/utils_test.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index c4fdad6779..17a79b98d8 100644 --- a/app/ante/eth.go +++ b/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") } } @@ -163,7 +163,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula evmDenom := egcd.evmKeeper.GetEVMDenom(ctx) - fees, err := evmkeeper.VerifyFee(ctx, txData, evmDenom, baseFee, homestead, istanbul) + fees, err := keeper.VerifyFee(ctx, txData, evmDenom, baseFee, homestead, istanbul) if err != nil { return ctx, errorsmod.Wrapf(err, "failed to verify the fees") } diff --git a/x/evm/keeper/utils_test.go b/x/evm/keeper/utils_test.go index 034a8d96aa..25169d57be 100644 --- a/x/evm/keeper/utils_test.go +++ b/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" ) @@ -238,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, ) @@ -479,7 +479,7 @@ func (suite *KeeperTestSuite) TestVerifyFeeAndDeductTxCostsFromUserBalance() { baseFee := suite.app.EvmKeeper.GetBaseFee(suite.ctx, ethCfg) priority := evmtypes.GetTxPriority(txData, baseFee) - fees, err := evmkeeper.VerifyFee(suite.ctx, txData, evmtypes.DefaultEVMDenom, baseFee, false, false) + fees, err := keeper.VerifyFee(suite.ctx, txData, evmtypes.DefaultEVMDenom, baseFee, false, false) if tc.expectPassVerify { suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name) if tc.enableFeemarket { From 473e781176988085604e2cc1bab030e9424b6f4c Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Tue, 29 Nov 2022 11:18:38 +0100 Subject: [PATCH 6/8] remove misleading comment --- x/evm/keeper/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index 54c6f58b18..f7feb24382 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -25,7 +25,7 @@ func (k *Keeper) DeductTxCostsFromUserBalance( fees sdk.Coins, from common.Address, ) error { - // fetch sender account from signature + // 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) From 368dda0f74ae35d5224c47f9672c0e400ea3a945 Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Tue, 29 Nov 2022 15:46:45 +0100 Subject: [PATCH 7/8] address review comments - only handover boolean instead of context --- app/ante/eth.go | 2 +- x/evm/handler_test.go | 2 +- x/evm/keeper/utils.go | 5 ++--- x/evm/keeper/utils_test.go | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 17a79b98d8..c2f9fae80a 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -163,7 +163,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula evmDenom := egcd.evmKeeper.GetEVMDenom(ctx) - fees, err := keeper.VerifyFee(ctx, txData, evmDenom, baseFee, homestead, istanbul) + fees, err := keeper.VerifyFee(txData, evmDenom, baseFee, homestead, istanbul, ctx.IsCheckTx()) if err != nil { return ctx, errorsmod.Wrapf(err, "failed to verify the fees") } diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index b05c075190..923cbb6c43 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -600,7 +600,7 @@ func (suite *EvmTestSuite) TestERC20TransferReverted() { txData, err := types.UnpackTxData(tx.Data) suite.Require().NoError(err) - fees, err := keeper.VerifyFee(suite.ctx, txData, "aphoton", baseFee, 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) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index f7feb24382..6c2713c5d2 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -43,11 +43,10 @@ func (k *Keeper) DeductTxCostsFromUserBalance( // 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( - ctx sdk.Context, txData evmtypes.TxData, denom string, baseFee *big.Int, - homestead, istanbul bool, + homestead, istanbul, isCheckTx bool, ) (sdk.Coins, error) { isContractCreation := txData.GetTo() == nil @@ -68,7 +67,7 @@ func VerifyFee( } // 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, diff --git a/x/evm/keeper/utils_test.go b/x/evm/keeper/utils_test.go index 25169d57be..c55868addb 100644 --- a/x/evm/keeper/utils_test.go +++ b/x/evm/keeper/utils_test.go @@ -479,7 +479,7 @@ func (suite *KeeperTestSuite) TestVerifyFeeAndDeductTxCostsFromUserBalance() { baseFee := suite.app.EvmKeeper.GetBaseFee(suite.ctx, ethCfg) priority := evmtypes.GetTxPriority(txData, baseFee) - fees, err := keeper.VerifyFee(suite.ctx, txData, evmtypes.DefaultEVMDenom, baseFee, false, false) + fees, err := keeper.VerifyFee(txData, evmtypes.DefaultEVMDenom, baseFee, false, false, suite.ctx.IsCheckTx()) if tc.expectPassVerify { suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name) if tc.enableFeemarket { From 1cb29214867a2c1bdba045bdff176bf9adf2d70e Mon Sep 17 00:00:00 2001 From: MalteHerrmann Date: Tue, 29 Nov 2022 15:48:00 +0100 Subject: [PATCH 8/8] remove TODO --- x/evm/keeper/utils.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index 6c2713c5d2..bc9ca6492b 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -18,8 +18,6 @@ import ( // DeductTxCostsFromUserBalance deducts the fees from the user balance. Returns an // error if the specified sender address does not exist or the account balance is not sufficient. -// -// TODO: Currently, Goland raises the warning that there are methods defined on both *keeper and keeper, which is not recommended: https://go.dev/doc/faq#methods_on_values_or_pointers func (k *Keeper) DeductTxCostsFromUserBalance( ctx sdk.Context, fees sdk.Coins,