global fee refactor (#2316)

* - Update doc to follow Go conventions - Improve code readability * update params * rebase and fix nits * add GetMinGasPrice UT * remove unused feetestsuite * rebase * update doc * nits * nits * refactor fee tests setup * remove comments * Update x/globalfee/ante/antetest/fee_test.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/ante/antetest/fee_test.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/ante/antetest/fee_test.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/ante/antetest/fee_test.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/ante/antetest/fee_test.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/ante/antetest/fee_test.go * fix GetMinGasPrice to not return nil coins * min gas price zero coins edge cases * min gas price zero coins edge cases * remove debug logs * udpate comments * udpate comments * add comment * Update x/globalfee/types/params.go Co-authored-by: yaruwangway <[email protected]> * Update x/globalfee/types/params.go Co-authored-by: yaruwangway <[email protected]> * fix: lint Signed-off-by: Yaru Wang <[email protected]> * update according to PR comments * Update x/globalfee/ante/fee.go Co-authored-by: Marius Poke <[email protected]> --------- Signed-off-by: Yaru Wang <[email protected]> Co-authored-by: yaruwangway <[email protected]> Co-authored-by: Yaru Wang <[email protected]> Co-authored-by: MSalopek <[email protected]> Co-authored-by: Marius Poke <[email protected]>
cosmos · Apr 3, 2023 · 5e389ac · 5e389ac
1 parent 6a1b40d
commit 5e389ac
Show file tree

Hide file tree

Showing 10 changed files with 374 additions and 258 deletions.
diff --git a/ante/ante.go b/ante/ante.go
@@ -13,6 +13,13 @@ import (
 	gaiafeeante "github.com/cosmos/gaia/v9/x/globalfee/ante"
 )
 
+// maxTotalBypassMinFeeMsgGasUsage is the allowed maximum gas usage
+// for all the bypass msgs in a transactions.
+// A transaction that contains only bypass message types and the gas usage does not
+// exceed maxTotalBypassMinFeeMsgGasUsage can be accepted with a zero fee.
+// For details, see gaiafeeante.NewFeeDecorator()
+var maxTotalBypassMinFeeMsgGasUsage uint64 = 1_000_000
+
 // HandlerOptions extend the SDK's AnteHandler options by requiring the IBC
 // channel keeper.
 type HandlerOptions struct {
@@ -53,13 +60,6 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) {
 		sigGasConsumer = ante.DefaultSigVerificationGasConsumer
 	}
 
-	// maxTotalBypassMinFeeMsgGasUsage is the allowed maximum gas usage
-	// for all the bypass msgs in a transactions.
-	// A transaction that contains only bypass message types and the gas usage does not
-	// exceed maxTotalBypassMinFeeMsgGasUsage can be accepted with a zero fee.
-	// For details, see gaiafeeante.NewFeeDecorator()
-	var maxTotalBypassMinFeeMsgGasUsage uint64 = 1_000_000
-
 	anteDecorators := []sdk.AnteDecorator{
 		ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
 		ante.NewRejectExtensionOptionsDecorator(),
@@ -69,7 +69,6 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) {
 		ante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper),
 		NewGovPreventSpamDecorator(opts.Codec, opts.GovKeeper),
 		gaiafeeante.NewFeeDecorator(opts.BypassMinFeeMsgTypes, opts.GlobalFeeSubspace, opts.StakingSubspace, maxTotalBypassMinFeeMsgGasUsage),
-
 		ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper),
 		ante.NewSetPubKeyDecorator(opts.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
 		ante.NewValidateSigCountDecorator(opts.AccountKeeper),

diff --git a/docs/modules/globalfee.md b/docs/modules/globalfee.md
@@ -201,7 +201,7 @@ Note that the required amount of `uatom` in globalfee is overwritten by the amou
 
 ### Case 7
 
-**Setting:** globalfee=[0.1uatom], minimum-gas-prices=0.2uatom,1stake, gas=200000, bypass-min-fee-msg-types = ["/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward"]
+**Setting:** globalfee=[0.1uatom], minimum-gas-prices=[0.2uatom, 1stake], gas=200000, bypass-min-fee-msg-types = ["/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward"]
 
 Note that the required amount of `uatom` in globalfee is overwritten by the amount in minimum-gas-prices. 
 Also, the `1stake` in minimum-gas-prices is ignored.

diff --git a/x/globalfee/ante/antetest/fee_test.go b/x/globalfee/ante/antetest/fee_test.go
diff --git a/x/globalfee/ante/antetest/fee_test_setup.go b/x/globalfee/ante/antetest/fee_test_setup.go
@@ -12,11 +12,13 @@ import (
 	xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 	"github.com/cosmos/cosmos-sdk/x/params/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-	gaiahelpers "github.com/cosmos/gaia/v9/app/helpers"
 	"github.com/stretchr/testify/suite"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 
+	gaiahelpers "github.com/cosmos/gaia/v9/app/helpers"
+	gaiafeeante "github.com/cosmos/gaia/v9/x/globalfee/ante"
+
 	gaiaapp "github.com/cosmos/gaia/v9/app"
 	"github.com/cosmos/gaia/v9/x/globalfee"
 	globfeetypes "github.com/cosmos/gaia/v9/x/globalfee/types"
@@ -31,6 +33,11 @@ type IntegrationTestSuite struct {
 	txBuilder client.TxBuilder
 }
 
+var (
+	testBondDenom                              = "uatom"
+	testMaxTotalBypassMinFeeMsgGasUsage uint64 = 1_000_000
+)
+
 func (s *IntegrationTestSuite) SetupTest() {
 	app := gaiahelpers.Setup(s.T())
 	ctx := app.BaseApp.NewContext(false, tmproto.Header{
@@ -47,12 +54,23 @@ func (s *IntegrationTestSuite) SetupTest() {
 	s.clientCtx = client.Context{}.WithTxConfig(encodingConfig.TxConfig)
 }
 
-func (s *IntegrationTestSuite) SetupTestGlobalFeeStoreAndMinGasPrice(minGasPrice []sdk.DecCoin, globalFeeParams *globfeetypes.Params) types.Subspace {
+func (s *IntegrationTestSuite) SetupTestGlobalFeeStoreAndMinGasPrice(minGasPrice []sdk.DecCoin, globalFeeParams *globfeetypes.Params) (gaiafeeante.FeeDecorator, sdk.AnteHandler) {
 	subspace := s.app.GetSubspace(globalfee.ModuleName)
 	subspace.SetParamSet(s.ctx, globalFeeParams)
 	s.ctx = s.ctx.WithMinGasPrices(minGasPrice).WithIsCheckTx(true)
 
-	return subspace
+	// set staking params
+	stakingParam := stakingtypes.DefaultParams()
+	stakingParam.BondDenom = testBondDenom
+	stakingSubspace := s.SetupTestStakingSubspace(stakingParam)
+
+	// build fee decorator
+	feeDecorator := gaiafeeante.NewFeeDecorator(gaiaapp.GetDefaultBypassFeeMessages(), subspace, stakingSubspace, uint64(1_000_000))
+
+	// chain fee decorator to antehandler
+	antehandler := sdk.ChainAnteDecorators(feeDecorator)
+
+	return feeDecorator, antehandler
 }
 
 // SetupTestStakingSubspace sets uatom as bond denom for the fee tests.

diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go
@@ -7,12 +7,15 @@ import (
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+
 	"github.com/cosmos/gaia/v9/x/globalfee/types"
 
+	tmstrings "github.com/tendermint/tendermint/libs/strings"
+
 	"github.com/cosmos/gaia/v9/x/globalfee"
 )
 
-// FeeWithBypassDecorator will check if the transaction's fee is at least as large
+// FeeWithBypassDecorator checks if the transaction's fee is at least as large
 // as the local validator's minimum gasFee (defined in validator config) and global fee, and the fee denom should be in the global fees' denoms.
 //
 // If fee is too low, decorator returns error and tx is rejected from mempool.
@@ -49,73 +52,80 @@ func NewFeeDecorator(bypassMsgTypes []string, globalfeeSubspace, stakingSubspace
 	}
 }
 
+// AnteHandle implements the AnteDecorator interface
 func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
 	// please note: after parsing feeflag, the zero fees are removed already
 	feeTx, ok := tx.(sdk.FeeTx)
 	if !ok {
 		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
 	}
+
+	// Only check for minimum fees and global fee if the execution mode is CheckTx
+	if !ctx.IsCheckTx() || simulate {
+		return next(ctx, tx, simulate)
+	}
+
+	// Get required Global Fee and min gas price
+	requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx)
+	if err != nil {
+		return ctx, err
+	}
+	requiredFees := GetMinGasPrice(ctx, int64(feeTx.GetGas()))
+
+	// sort fee tx's coins
 	feeCoins := feeTx.GetFee().Sort()
 	gas := feeTx.GetGas()
 	msgs := feeTx.GetMsgs()
 
 	// Accept zero fee transactions only if both of the following statements are true:
+	//
 	// 	- the tx contains only message types that can bypass the minimum fee,
 	//	see BypassMinFeeMsgTypes;
 	//	- the total gas limit per message does not exceed MaxTotalBypassMinFeeMsgGasUsage,
-	//	i.e., totalGas <= MaxTotalBypassMinFeeMsgGasUsage
+	//	i.e., totalGas <=  MaxTotalBypassMinFeeMsgGasUsage
+	//
 	// Otherwise, minimum fees and global fees are checked to prevent spam.
 	doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage
-	allowedToBypassMinFee := mfd.containsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
-
-	var allFees sdk.Coins
-	requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx)
-	if err != nil {
-		panic(err)
-	}
-	requiredFees := getMinGasPrice(ctx, feeTx)
-
-	// Only check for minimum fees and global fee if the execution mode is CheckTx
-	if !ctx.IsCheckTx() || simulate {
-		return next(ctx, tx, simulate)
-	}
+	allowedToBypassMinFee := mfd.ContainsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
 
-	if !allowedToBypassMinFee {
-		// Either the transaction contains at least on message of a type
+	if allowedToBypassMinFee {
+		// Transactions with zero fees are accepted
+		if len(feeCoins) == 0 {
+			return next(ctx, tx, simulate)
+		}
+		// If the transaction fee is non-zero, then check that the fees are in
+		// expected denominations.
+		if !DenomsSubsetOfIncludingZero(feeCoins, requiredGlobalFees) {
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fees denom is wrong; got: %s required: %s", feeCoins, requiredGlobalFees)
+		}
+	} else {
+		// Either the transaction contains at least one message of a type
 		// that cannot bypass the minimum fee or the total gas limit exceeds
 		// the imposed threshold. As a result, check that the fees are in
 		// expected denominations and the amounts are greater or equal than
 		// the expected amounts.
 
-		allFees = CombinedFeeRequirement(requiredGlobalFees, requiredFees)
+		combinedFees := CombinedFeeRequirement(requiredGlobalFees, requiredFees)
 
 		// Check that the fees are in expected denominations. Note that a zero fee
 		// is accepted if the global fee has an entry with a zero amount, e.g., 0uatoms.
-		if !DenomsSubsetOfIncludingZero(feeCoins, allFees) {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, allFees)
+		if !DenomsSubsetOfIncludingZero(feeCoins, combinedFees) {
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, combinedFees)
 		}
 		// Check that the amounts of the fees are greater or equal than
 		// the expected amounts, i.e., at least one feeCoin amount must
 		// be greater or equal to one of the combined required fees.
-		if !IsAnyGTEIncludingZero(feeCoins, allFees) {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, allFees)
-		}
-	} else {
-		// Transactions with zero fees are accepted
-		if len(feeCoins) == 0 {
-			return next(ctx, tx, simulate)
-		}
-		// If the transaction fee is non-zero, then check that the fees are in
-		// expected denominations.
-		if !DenomsSubsetOfIncludingZero(feeCoins, requiredGlobalFees) {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fees denom is wrong; got: %s required: %s", feeCoins, requiredGlobalFees)
+		if !IsAnyGTEIncludingZero(feeCoins, combinedFees) {
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, combinedFees)
 		}
 	}
 
 	return next(ctx, tx, simulate)
 }
 
-// ParamStoreKeyMinGasPrices type require coins sorted. getGlobalFee will also return sorted coins (might return 0denom if globalMinGasPrice is 0)
+// getGlobalFee returns the global fees for a given fee tx's gas (might also returns 0denom if globalMinGasPrice is 0)
+// sorted in ascending order.
+// Note that ParamStoreKeyMinGasPrices type requires coins sorted.
 func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coins, error) {
 	var (
 		globalMinGasPrices sdk.DecCoins
@@ -128,6 +138,9 @@ func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coin
 	// global fee is empty set, set global fee to 0uatom
 	if len(globalMinGasPrices) == 0 {
 		globalMinGasPrices, err = mfd.DefaultZeroGlobalFee(ctx)
+		if err != nil {
+			return sdk.Coins{}, err
+		}
 	}
 	requiredGlobalFees := make(sdk.Coins, len(globalMinGasPrices))
 	// Determine the required fees by multiplying each required minimum gas
@@ -138,7 +151,7 @@ func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coin
 		requiredGlobalFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt())
 	}
 
-	return requiredGlobalFees.Sort(), err
+	return requiredGlobalFees.Sort(), nil
 }
 
 func (mfd FeeDecorator) DefaultZeroGlobalFee(ctx sdk.Context) ([]sdk.DecCoin, error) {
@@ -158,3 +171,37 @@ func (mfd FeeDecorator) getBondDenom(ctx sdk.Context) string {
 
 	return bondDenom
 }
+
+// ContainsOnlyBypassMinFeeMsgs returns true if all the given msgs type are listed
+// in the BypassMinFeeMsgTypes of the FeeDecorator.
+func (mfd FeeDecorator) ContainsOnlyBypassMinFeeMsgs(msgs []sdk.Msg) bool {
+	for _, msg := range msgs {
+		if tmstrings.StringInSlice(sdk.MsgTypeURL(msg), mfd.BypassMinFeeMsgTypes) {
+			continue
+		}
+		return false
+	}
+
+	return true
+}
+
+// GetMinGasPrice returns a nodes's local minimum gas prices
+// fees given a gas limit
+func GetMinGasPrice(ctx sdk.Context, gasLimit int64) sdk.Coins {
+	minGasPrices := ctx.MinGasPrices()
+	// special case: if minGasPrices=[], requiredFees=[]
+	if minGasPrices.IsZero() {
+		return sdk.Coins{}
+	}
+
+	requiredFees := make(sdk.Coins, len(minGasPrices))
+	// Determine the required fees by multiplying each required minimum gas
+	// price by the gas limit, where fee = ceil(minGasPrice * gasLimit).
+	glDec := sdk.NewDec(gasLimit)
+	for i, gp := range minGasPrices {
+		fee := gp.Amount.Mul(glDec)
+		requiredFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt())
+	}
+
+	return requiredFees.Sort()
+}