Skip to content

Commit

Permalink
Problem: native fee handler don't respect DefaultPriorityReduction
Browse files Browse the repository at this point in the history
Solution:
- vendor fee deduct decorator
- a little refactor to remove `DeductTxCostsFromUserBalance` and call the DeductFees directly

next: will modify the DeductFees to provide a different impl for parallel exec

Apply suggestions from code review

Signed-off-by: yihuang <[email protected]>
  • Loading branch information
yihuang committed Mar 11, 2024
1 parent 853a1bd commit 630c56b
Show file tree
Hide file tree
Showing 10 changed files with 181 additions and 43 deletions.
2 changes: 1 addition & 1 deletion app/ante/eip712.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func NewLegacyCosmosAnteHandlerEip712(ctx sdk.Context, options HandlerOptions, e
NewMinGasPriceDecorator(options.FeeMarketKeeper, evmDenom),
authante.NewValidateMemoDecorator(options.AccountKeeper),
authante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
authante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
// SetPubKeyDecorator must be called before all signature verification decorators
authante.NewSetPubKeyDecorator(options.AccountKeeper),
authante.NewValidateSigCountDecorator(options.AccountKeeper),
Expand Down
31 changes: 21 additions & 10 deletions app/ante/eth.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
errortypes "github.com/cosmos/cosmos-sdk/types/errors"
authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"

ethermint "github.com/evmos/ethermint/types"
"github.com/evmos/ethermint/x/evm/keeper"
Expand Down Expand Up @@ -105,23 +107,26 @@ func (avd EthAccountVerificationDecorator) AnteHandle(
// EthGasConsumeDecorator validates enough intrinsic gas for the transaction and
// gas consumption.
type EthGasConsumeDecorator struct {
evmKeeper EVMKeeper
maxGasWanted uint64
ethCfg *params.ChainConfig
evmDenom string
baseFee *big.Int
accountKeeper authante.AccountKeeper
bankKeeper authtypes.BankKeeper
maxGasWanted uint64
ethCfg *params.ChainConfig
evmDenom string
baseFee *big.Int
}

// NewEthGasConsumeDecorator creates a new EthGasConsumeDecorator
func NewEthGasConsumeDecorator(
evmKeeper EVMKeeper,
accountKeeper authante.AccountKeeper,
bankKeeper authtypes.BankKeeper,
maxGasWanted uint64,
ethCfg *params.ChainConfig,
evmDenom string,
baseFee *big.Int,
) EthGasConsumeDecorator {
return EthGasConsumeDecorator{
evmKeeper,
accountKeeper,
bankKeeper,
maxGasWanted,
ethCfg,
evmDenom,
Expand Down Expand Up @@ -195,9 +200,15 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
return ctx, errorsmod.Wrapf(err, "failed to verify the fees")
}

err = egcd.evmKeeper.DeductTxCostsFromUserBalance(ctx, fees, common.BytesToAddress(msgEthTx.From))
if err != nil {
return ctx, errorsmod.Wrapf(err, "failed to deduct transaction costs from user balance")
// fetch sender account
acc := egcd.accountKeeper.GetAccount(ctx, msgEthTx.From)
if acc == nil {
return ctx, errorsmod.Wrapf(err, "account not found for sender %s", msgEthTx.From)
}

// deduct the full gas cost from the user balance
if err := authante.DeductFees(egcd.bankKeeper, ctx, acc, fees); err != nil {
return ctx, errorsmod.Wrapf(err, "failed to deduct full gas cost %s from the user %s balance", fees, msgEthTx.From)
}

events = append(events,
Expand Down
2 changes: 1 addition & 1 deletion app/ante/eth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() {
chainCfg := evmParams.GetChainConfig()
ethCfg := chainCfg.EthereumConfig(chainID)
baseFee := suite.app.EvmKeeper.GetBaseFee(suite.ctx, ethCfg)
dec := ante.NewEthGasConsumeDecorator(suite.app.EvmKeeper, config.DefaultMaxTxGasWanted, ethCfg, evmtypes.DefaultEVMDenom, baseFee)
dec := ante.NewEthGasConsumeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, config.DefaultMaxTxGasWanted, ethCfg, evmtypes.DefaultEVMDenom, baseFee)

addr := tests.GenerateAddress()

Expand Down
11 changes: 8 additions & 3 deletions app/ante/fee_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,14 @@ func NewDynamicFeeChecker(k DynamicFeeEVMKeeper) authante.TxFeeChecker {

// checkTxFeeWithValidatorMinGasPrices implements the default fee logic, where the minimum price per
// unit of gas is fixed and set by each validator, and the tx priority is computed from the gas price.
func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.FeeTx) (sdk.Coins, int64, error) {
feeCoins := tx.GetFee()
gas := tx.GetGas()
func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) {
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return nil, 0, errorsmod.Wrap(errortypes.ErrTxDecode, "Tx must be a FeeTx")
}

feeCoins := feeTx.GetFee()
gas := feeTx.GetGas()
minGasPrices := ctx.MinGasPrices()

// Ensure that the provided fees meet a minimum threshold for the validator,
Expand Down
4 changes: 2 additions & 2 deletions app/ante/handler_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func newEthAnteHandler(ctx sdk.Context, options HandlerOptions, extra ...sdk.Ant
NewEthSigVerificationDecorator(chainID),
NewEthAccountVerificationDecorator(options.AccountKeeper, options.EvmKeeper, evmDenom),
NewCanTransferDecorator(options.EvmKeeper, baseFee, &evmParams, ethCfg),
NewEthGasConsumeDecorator(options.EvmKeeper, options.MaxTxGasWanted, ethCfg, evmDenom, baseFee),
NewEthGasConsumeDecorator(options.AccountKeeper, options.BankKeeper, options.MaxTxGasWanted, ethCfg, evmDenom, baseFee),
NewEthIncrementSenderSequenceDecorator(options.AccountKeeper), // innermost AnteDecorator.
NewGasWantedDecorator(options.FeeMarketKeeper, ethCfg),
NewEthEmitEventDecorator(options.EvmKeeper), // emit eth tx hash and index at the very last ante handler.
Expand All @@ -108,7 +108,7 @@ func newCosmosAnteHandler(ctx sdk.Context, options HandlerOptions, extra ...sdk.
NewMinGasPriceDecorator(options.FeeMarketKeeper, evmDenom),
ante.NewValidateMemoDecorator(options.AccountKeeper),
ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
// SetPubKeyDecorator must be called before all signature verification decorators
ante.NewSetPubKeyDecorator(options.AccountKeeper),
ante.NewValidateSigCountDecorator(options.AccountKeeper),
Expand Down
2 changes: 0 additions & 2 deletions app/ante/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
tx "github.com/cosmos/cosmos-sdk/types/tx"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/params"
"github.com/evmos/ethermint/x/evm/statedb"
evmtypes "github.com/evmos/ethermint/x/evm/types"
Expand All @@ -40,7 +39,6 @@ type EVMKeeper interface {
statedb.Keeper
DynamicFeeEVMKeeper

DeductTxCostsFromUserBalance(ctx sdk.Context, fees sdk.Coins, from common.Address) error
ResetTransientGasUsed(ctx sdk.Context)
GetTxIndexTransient(ctx sdk.Context) uint64
}
Expand Down
133 changes: 133 additions & 0 deletions app/ante/nativefee.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
package ante

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/auth/ante"
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// DeductFeeDecorator deducts fees from the fee payer. The fee payer is the fee granter (if specified) or first signer of the tx.
// If the fee payer does not have the funds to pay for the fees, return an InsufficientFunds error.
// Call next AnteHandler if fees successfully deducted.
// CONTRACT: Tx must implement FeeTx interface to use DeductFeeDecorator
type DeductFeeDecorator struct {
accountKeeper ante.AccountKeeper
bankKeeper types.BankKeeper
feegrantKeeper ante.FeegrantKeeper
txFeeChecker ante.TxFeeChecker
}

func NewDeductFeeDecorator(ak ante.AccountKeeper, bk types.BankKeeper, fk ante.FeegrantKeeper, tfc ante.TxFeeChecker) DeductFeeDecorator {
if tfc == nil {
tfc = checkTxFeeWithValidatorMinGasPrices
}

return DeductFeeDecorator{
accountKeeper: ak,
bankKeeper: bk,
feegrantKeeper: fk,
txFeeChecker: tfc,
}
}

func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")

Check failure on line 39 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrap is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}

if !simulate && ctx.BlockHeight() > 0 && feeTx.GetGas() == 0 {
return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidGasLimit, "must provide positive gas")

Check failure on line 43 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrap is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}

var (
priority int64
err error
)

fee := feeTx.GetFee()
if !simulate {
fee, priority, err = dfd.txFeeChecker(ctx, tx)
if err != nil {
return ctx, err
}
}
if err := dfd.checkDeductFee(ctx, tx, fee); err != nil {
return ctx, err
}

newCtx := ctx.WithPriority(priority)

return next(newCtx, tx, simulate)
}

func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee sdk.Coins) error {
feeTx, ok := sdkTx.(sdk.FeeTx)
if !ok {
return sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")

Check failure on line 70 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrap is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}

if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil {
return fmt.Errorf("fee collector module account (%s) has not been set", types.FeeCollectorName)
}

feePayer := feeTx.FeePayer()
feeGranter := feeTx.FeeGranter()
deductFeesFrom := feePayer

// if feegranter set deduct fee from feegranter account.
// this works with only when feegrant enabled.
if feeGranter != nil {
if dfd.feegrantKeeper == nil {
return sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled")
} else if !feeGranter.Equals(feePayer) {
err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, fee, sdkTx.GetMsgs())
if err != nil {
return sdkerrors.Wrapf(err, "%s does not allow to pay fees for %s", feeGranter, feePayer)

Check failure on line 89 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrapf is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}
}

deductFeesFrom = feeGranter
}

deductFeesFromAcc := dfd.accountKeeper.GetAccount(ctx, deductFeesFrom)
if deductFeesFromAcc == nil {
return sdkerrors.ErrUnknownAddress.Wrapf("fee payer address: %s does not exist", deductFeesFrom)
}

// deduct the fees
if !fee.IsZero() {
err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee)
if err != nil {
return err
}
}

events := sdk.Events{
sdk.NewEvent(
sdk.EventTypeTx,
sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()),
sdk.NewAttribute(sdk.AttributeKeyFeePayer, deductFeesFrom.String()),
),
}
ctx.EventManager().EmitEvents(events)

return nil
}

// DeductFees deducts fees from the given account.
func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc types.AccountI, fees sdk.Coins) error {
if !fees.IsValid() {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "invalid fee amount: %s", fees)

Check failure on line 124 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrapf is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}

err := bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), types.FeeCollectorName, fees)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())

Check failure on line 129 in app/ante/nativefee.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint

SA1019: sdkerrors.Wrapf is deprecated: functionality of this package has been moved to it's own module: (staticcheck)
}

return nil
}
9 changes: 8 additions & 1 deletion x/evm/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/evmos/ethermint/app"
"github.com/evmos/ethermint/app/ante"
ethermint "github.com/evmos/ethermint/types"
"github.com/evmos/ethermint/x/evm"
"github.com/evmos/ethermint/x/evm/types"
Expand Down Expand Up @@ -500,7 +501,13 @@ func (suite *HandlerTestSuite) TestERC20TransferReverted() {
suite.Require().NoError(err)
fees, err := keeper.VerifyFee(txData, "aphoton", baseFee, true, true, true, suite.Ctx.IsCheckTx())
suite.Require().NoError(err)
err = k.DeductTxCostsFromUserBalance(suite.Ctx, fees, tx.GetSender())

// fetch sender account
signerAcc := suite.App.AccountKeeper.GetAccount(suite.Ctx, tx.GetSender().Bytes())
suite.Require().NotNil(signerAcc)

// deduct the full gas cost from the user balance
err = ante.DeductFees(suite.App.BankKeeper, suite.Ctx, signerAcc, fees)
suite.Require().NoError(err)

res, err := k.EthereumTx(sdk.WrapSDKContext(suite.Ctx), tx)
Expand Down
22 changes: 0 additions & 22 deletions x/evm/keeper/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
sdkmath "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
errortypes "github.com/cosmos/cosmos-sdk/types/errors"
authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"github.com/evmos/ethermint/x/evm/types"
Expand Down Expand Up @@ -55,27 +54,6 @@ func GetProposerAddress(ctx sdk.Context, proposerAddress sdk.ConsAddress) sdk.Co
return proposerAddress
}

// 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.
func (k *Keeper) DeductTxCostsFromUserBalance(
ctx sdk.Context,
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.
Expand Down
8 changes: 7 additions & 1 deletion x/evm/keeper/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
ethtypes "github.com/ethereum/go-ethereum/core/types"
ethparams "github.com/ethereum/go-ethereum/params"
"github.com/evmos/ethermint/app"
"github.com/evmos/ethermint/app/ante"
"github.com/evmos/ethermint/testutil"
"github.com/evmos/ethermint/x/evm/keeper"
evmtypes "github.com/evmos/ethermint/x/evm/types"
Expand Down Expand Up @@ -531,7 +532,12 @@ func (suite *UtilsTestSuite) TestVerifyFeeAndDeductTxCostsFromUserBalance() {
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.BytesToAddress(tx.From))
// fetch sender account
acc := suite.app.AccountKeeper.GetAccount(suite.ctx, tx.From)

Check failure on line 536 in x/evm/keeper/utils_test.go

View workflow job for this annotation

GitHub Actions / test-unit-cover

suite.app undefined (type *UtilsTestSuite has no field or method app, but does have App)

Check failure on line 536 in x/evm/keeper/utils_test.go

View workflow job for this annotation

GitHub Actions / test-unit-cover

suite.ctx undefined (type *UtilsTestSuite has no field or method ctx, but does have Ctx)
suite.Require().NotNil(acc, "account not found for sender %s", tx.From)

// deduct the full gas cost from the user balance
err = ante.DeductFees(suite.app.BankKeeper, suite.ctx, acc, fees)

Check failure on line 540 in x/evm/keeper/utils_test.go

View workflow job for this annotation

GitHub Actions / test-unit-cover

suite.app undefined (type *UtilsTestSuite has no field or method app, but does have App)

Check failure on line 540 in x/evm/keeper/utils_test.go

View workflow job for this annotation

GitHub Actions / test-unit-cover

suite.ctx undefined (type *UtilsTestSuite has no field or method ctx, but does have Ctx)
if tc.expectPassDeduct {
suite.Require().NoError(err, "valid test %d failed - '%s'", i, tc.name)
} else {
Expand Down

0 comments on commit 630c56b

Please sign in to comment.