Reformat global fee

docs/modules/globalfee.md

@@ -200,10 +200,9 @@ 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.
+Note that the required amounts of `uatom` and `stake` in minimum-gas-prices is are ignored.
 
   - msg withdraw-all-rewards with paidfee="", `pass`
   - msg withdraw-all-rewards with paidfee="200000 * 0.05uatom", `pass`

x/globalfee/ante/fee.go

@@ -10,9 +10,10 @@ import (
 	"github.com/cosmos/gaia/v9/x/globalfee/types"
 
 	"github.com/cosmos/gaia/v9/x/globalfee"
+	tmstrings "github.com/tendermint/tendermint/libs/strings"
 )
 
-// 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,59 +50,43 @@ 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)
+	}
+
+	// sort fee tx's coins
 	feeCoins := feeTx.GetFee().Sort()
 	gas := feeTx.GetGas()
 	msgs := feeTx.GetMsgs()
 
+	// Get required Global Fee and min gas price
+	requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx)
+	if err != nil {
+		panic(err)
+	}
+	requiredFees := getMinGasPrice(ctx, feeTx)
+
 	// 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 MaxBypassMinFeeMsgGasUsage,
 	//	i.e., totalGas <= len(msgs) * MaxBypassMinFeeMsgGasUsage
+	//
 	// Otherwise, minimum fees and global fees are checked to prevent spam.
-	containsOnlyBypassMinFeeMsgs := mfd.bypassMinFeeMsgs(msgs)
 	doesNotExceedMaxGasUsage := gas <= uint64(len(msgs))*mfd.MaxBypassMinFeeMsgGasUsage
-	allowedToBypassMinFee := containsOnlyBypassMinFeeMsgs && 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)
-	}
-
-	if !allowedToBypassMinFee {
-		// Either the transaction contains at least on 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.
+	allowedToBypassMinFee := mfd.containsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
 
-		allFees = 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)
-		}
-		// 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 {
+	if allowedToBypassMinFee {
 		// Transactions with zero fees are accepted
 		if len(feeCoins) == 0 {
 			return next(ctx, tx, simulate)
@@ -113,10 +98,32 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 		}
 	}
 
+	// Either the transaction contains at least on 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)
+
+	// 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)
+	}
+	// 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)
+	}
+
 	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
@@ -129,6 +136,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
@@ -139,7 +149,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) {
@@ -159,3 +169,16 @@ 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
+}

x/globalfee/ante/fee_utils.go

@@ -1,13 +1,11 @@
 package ante
 
 import (
-	"math"
-
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	tmstrings "github.com/tendermint/tendermint/libs/strings"
 )
 
-// getMinGasPrice will also return sorted coins
+// getMinGasPrice returns the validator's minimum gas prices
+// for a given fee tx's gas sorted in ascending order
 func getMinGasPrice(ctx sdk.Context, feeTx sdk.FeeTx) sdk.Coins {
 	minGasPrices := ctx.MinGasPrices()
 	gas := feeTx.GetGas()
@@ -27,22 +25,14 @@ func getMinGasPrice(ctx sdk.Context, feeTx sdk.FeeTx) sdk.Coins {
 	return requiredFees.Sort()
 }
 
-func (mfd FeeDecorator) bypassMinFeeMsgs(msgs []sdk.Msg) bool {
-	for _, msg := range msgs {
-		if tmstrings.StringInSlice(sdk.MsgTypeURL(msg), mfd.BypassMinFeeMsgTypes) {
-			continue
-		}
-		return false
-	}
-
-	return true
-}
-
-// DenomsSubsetOfIncludingZero and IsAnyGTEIncludingZero are similar to DenomsSubsetOf and IsAnyGTE in sdk. Since we allow zero coins in global fee(zero coins means the chain does not want to set a global fee but still want to define the fee's denom)
+// DenomsSubsetOfIncludingZero and IsAnyGTEIncludingZero are similar to DenomsSubsetOf and IsAnyGTE in sdk.
+// Since we allow zero coins in global fee(zero coins means the chain does not want to set a global fee but still want to define the fee's denom)
 //
-// overwrite DenomsSubsetOfIncludingZero from sdk, to allow zero amt coins in superset. e.g. 1stake is DenomsSubsetOfIncludingZero 0stake. [] is the DenomsSubsetOfIncludingZero of [0stake] but not [1stake].
+// DenomsSubsetOfIncludingZero overwrites DenomsSubsetOf from sdk, to allow zero amt coins in superset. e.g.
+// e.g. [1stake] is the DenomsSubsetOfIncludingZero of [0stake] and
+// [] is the DenomsSubsetOfIncludingZero of [0stake] but not [1stake].
 // DenomsSubsetOfIncludingZero returns true if coins's denoms is subset of coinsB's denoms.
-// if coins is empty set, empty set is any sets' subset
+// If coins is empty set, empty set is any sets' subset
 func DenomsSubsetOfIncludingZero(coins, coinsB sdk.Coins) bool {
 	// more denoms in B than in receiver
 	if len(coins) > len(coinsB) {
@@ -67,9 +57,9 @@ func DenomsSubsetOfIncludingZero(coins, coinsB sdk.Coins) bool {
 	return true
 }
 
-// overwrite the IsAnyGTEIncludingZero from sdk to allow zero coins in coins and coinsB.
-// IsAnyGTEIncludingZero returns true if coins contain at least one denom that is present at a greater or equal amount in coinsB; it returns false otherwise.
-// if CoinsB is emptyset, no coins sets are IsAnyGTEIncludingZero coinsB unless coins is also empty set.
+// IsAnyGTEIncludingZero overwrites the IsAnyGTE from sdk to allow zero coins in coins and coinsB.
+// IsAnyGTEIncludingZero returns true if coins contain at least one denom that is present at a greater or equal amount in coinsB;
+// it returns false otherwise. If CoinsB is emptyset, no coins sets are IsAnyGTEIncludingZero coinsB unless coins is also empty set.
 // NOTE: IsAnyGTEIncludingZero operates under the invariant that both coin sets are sorted by denoms.
 // contract !!!! coins must be DenomsSubsetOfIncludingZero of coinsB
 func IsAnyGTEIncludingZero(coins, coinsB sdk.Coins) bool {
@@ -103,13 +93,13 @@ func IsAnyGTEIncludingZero(coins, coinsB sdk.Coins) bool {
 	return false
 }
 
-// return true if coinsB is empty or contains zero coins,
-// CoinsB must be validate coins !!!
-func ContainZeroCoins(coinsB sdk.Coins) bool {
-	if len(coinsB) == 0 {
+// ContainZeroCoins returns true if the given coins is empty or contains zero coins,
+// Note that the coins denoms must be validated, see sdk.ValidateDenom
+func ContainZeroCoins(coins sdk.Coins) bool {
+	if len(coins) == 0 {
 		return true
 	}
-	for _, coin := range coinsB {
+	for _, coin := range coins {
 		if coin.IsZero() {
 			return true
 		}
@@ -118,7 +108,9 @@ func ContainZeroCoins(coinsB sdk.Coins) bool {
 	return false
 }
 
-// CombinedFeeRequirement will combine the global fee and min_gas_price. Both globalFees and minGasPrices must be valid, but CombinedFeeRequirement does not validate them, so it may return 0denom.
+// CombinedFeeRequirement returns the global fee and min_gas_price combined and sorted.
+// Both globalFees and minGasPrices must be valid, but CombinedFeeRequirement
+// does not validate them, so it may return 0denom.
 func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins {
 	// empty min_gas_price
 	if len(minGasPrices) == 0 {
@@ -144,23 +136,6 @@ func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins {
 	return allFees.Sort()
 }
 
-// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the fee
-// provided in a transaction.
-func GetTxPriority(fee sdk.Coins) int64 {
-	var priority int64
-	for _, c := range fee {
-		p := int64(math.MaxInt64)
-		if c.Amount.IsInt64() {
-			p = c.Amount.Int64()
-		}
-		if priority == 0 || p < priority {
-			priority = p
-		}
-	}
-
-	return priority
-}
-
 // Find replaces the functionality of Coins.Find from SDK v0.46.x
 func Find(coins sdk.Coins, denom string) (bool, sdk.Coin) {
 	switch len(coins) {

x/globalfee/module.go

@@ -58,7 +58,11 @@ func (a AppModuleBasic) RegisterRESTRoutes(context client.Context, router *mux.R
 }
 
 func (a AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) {
-	_ = types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx))
+	err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx))
+	if err != nil {
+		// same behavior as in cosmos-sdk
+		panic(err)
+	}
 }
 
 func (a AppModuleBasic) GetTxCmd() *cobra.Command {

x/globalfee/types/params.go

@@ -45,53 +45,37 @@ func validateMinimumGasPrices(i interface{}) error {
 	return dec.Validate()
 }
 
-// Validate checks that the DecCoins are sorted, have nonnegtive amount, with a valid and unique
-// denomination (i.e no duplicates). Otherwise, it returns an error.
 type DecCoins sdk.DecCoins
 
+// Validate checks that the DecCoins are sorted, have nonnegtive amount, with a valid and unique
+// denomination (i.e no duplicates). Otherwise, it returns an error.
 func (coins DecCoins) Validate() error {
-	switch len(coins) {
-	case 0:
+	if len(coins) == 0 {
 		return nil
+	}
 
-	case 1:
-		// match the denom reg expr
-		if err := sdk.ValidateDenom(coins[0].Denom); err != nil {
-			return err
-		}
-		if coins[0].IsNegative() {
-			return fmt.Errorf("coin %s amount is negtive", coins[0])
+	lowDenom := ""
+	seenDenoms := make(map[string]bool)
+
+	for _, coin := range coins {
+		if seenDenoms[coin.Denom] {
+			return fmt.Errorf("duplicate denomination %s", coin.Denom)
 		}
-		return nil
-	default:
-		// check single coin case
-		if err := (DecCoins{coins[0]}).Validate(); err != nil {
+		if err := sdk.ValidateDenom(coin.Denom); err != nil {
 			return err
 		}
-
-		lowDenom := coins[0].Denom
-		seenDenoms := make(map[string]bool)
-		seenDenoms[lowDenom] = true
-
-		for _, coin := range coins[1:] {
-			if seenDenoms[coin.Denom] {
-				return fmt.Errorf("duplicate denomination %s", coin.Denom)
-			}
-			if err := sdk.ValidateDenom(coin.Denom); err != nil {
-				return err
-			}
-			if coin.Denom <= lowDenom {
-				return fmt.Errorf("denomination %s is not sorted", coin.Denom)
-			}
-			if coin.IsNegative() {
-				return fmt.Errorf("coin %s amount is negtive", coin.Denom)
-			}
-
-			// we compare each coin against the last denom
-			lowDenom = coin.Denom
-			seenDenoms[coin.Denom] = true
+		if coin.Denom <= lowDenom {
+			return fmt.Errorf("denomination %s is not sorted", coin.Denom)
+		}
+		if coin.IsNegative() {
+			return fmt.Errorf("coin %s amount is negative", coin.Amount)
 		}
 
-		return nil
+		// we compare each coin against the last denom
+		lowDenom = coin.Denom
+		seenDenoms[coin.Denom] = true
 	}
+
+	return nil
+
 }

x/globalfee/types/params_test.go

@@ -46,6 +46,18 @@ func Test_validateParams(t *testing.T) {
 			},
 			true,
 		},
+		"negative amount, fail": {
+			sdk.DecCoins{
+				sdk.DecCoin{Denom: "photon", Amount: sdk.OneDec().Neg()},
+			},
+			true,
+		},
+		"invalid denom, fail": {
+			sdk.DecCoins{
+				sdk.DecCoin{Denom: "photon!", Amount: sdk.OneDec().Neg()},
+			},
+			true,
+		},
 	}
 
 	for name, test := range tests {