refactor global fee zero coins (#2327)

* - 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]> * refactor: global fee zero coins * refactor: add splitGlobalFees() * fix: globalfee antehandler * fix: add test helper func * update according to PR comments * test: add test for splitGlobalFees() * code improvement * refactor: globalfee antehandler * refactor: bypass logic * refactor: fee check logic * fix: lint * fix: return err when required fees are not set up * chore: clean comments * Sainoe/globalfee coins (#2378) * chore(deps): bump github.com/golangci/golangci-lint (#2319) Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.51.2 to 1.52.1. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.51.2...v1.52.1) --- updated-dependencies: - dependency-name: github.com/golangci/golangci-lint dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Milan Mulji <[email protected]> * chore(deps): bump github.com/docker/docker (#2365) Bumps [github.com/docker/docker](https://github.com/docker/docker) from 20.10.19+incompatible to 20.10.24+incompatible. - [Release notes](https://github.com/docker/docker/releases) - [Commits](moby/moby@v20.10.19...v20.10.24) --- updated-dependencies: - dependency-name: github.com/docker/docker dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Removed trust node in docs (#2359) * update ditrosless image url in Dockerfiles * nits --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Milan Mulji <[email protected]> * fix nits --------- Signed-off-by: Yaru Wang <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: MSalopek <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Milan Mulji <[email protected]>
cosmos · Apr 12, 2023 · 6fe097e · 6fe097e
1 parent 53397fa
commit 6fe097e
Show file tree

Hide file tree

Showing 3 changed files with 201 additions and 327 deletions.
diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go
@@ -7,12 +7,10 @@ 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"
+	"github.com/cosmos/gaia/v9/x/globalfee/types"
 )
 
 // FeeWithBypassDecorator checks if the transaction's fee is at least as large
@@ -54,28 +52,51 @@ 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")
+		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must implement the sdk.FeeTx interface")
 	}
 
 	// 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)
+	// Sort fee tx's coins, zero coins in feeCoins are already removed
+	feeCoins := feeTx.GetFee().Sort()
+	gas := feeTx.GetGas()
+	msgs := feeTx.GetMsgs()
+
+	// Get required Global Fee
+	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()
+	// Get local minimum-gas-prices
+	localFees := GetMinGasPrice(ctx, int64(feeTx.GetGas()))
+
+	// CombinedFeeRequirement should never be empty since
+	// global fee is set to its default value, i.e. 0uatom, if empty
+	combinedFeeRequirement := CombinedFeeRequirement(requiredGlobalFees, localFees)
+	if len(combinedFeeRequirement) == 0 {
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "required fees are not setup.")
+	}
+
+	nonZeroCoinFeesReq, zeroCoinFeesDenomReq := getNonZeroFees(combinedFeeRequirement)
+
+	// feeCoinsNonZeroDenom contains non-zero denominations from the combinedFeeRequirement
+	//
+	// feeCoinsNoZeroDenom is used to check if the fees meets the requirement imposed by nonZeroCoinFeesReq
+	// when feeCoins does not contain zero coins' denoms in combinedFeeRequirement
+	feeCoinsNonZeroDenom, feeCoinsZeroDenom := splitCoinsByDenoms(feeCoins, zeroCoinFeesDenomReq)
+
+	// Check that the fees are in expected denominations.
+	// if feeCoinsNoZeroDenom=[], DenomsSubsetOf returns true
+	// if feeCoinsNoZeroDenom is not empty, but nonZeroCoinFeesReq empty, return false
+	if !feeCoinsNonZeroDenom.DenomsSubsetOf(nonZeroCoinFeesReq) {
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, combinedFeeRequirement)
+	}
 
 	// Accept zero fee transactions only if both of the following statements are true:
 	//
@@ -88,45 +109,41 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 	doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage
 	allowedToBypassMinFee := mfd.ContainsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
 
-	if allowedToBypassMinFee {
-		// Transactions with zero fees are accepted
-		if len(feeCoins) == 0 {
+	// 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, besides check the fees are in
+	// expected denominations, check the amounts are greater or equal than
+	// the expected amounts.
+
+	// only check feeCoinsNoZeroDenom has coins IsAnyGTE than nonZeroCoinFeesReq
+	// when feeCoins does not contain denoms of zero denoms in combinedFeeRequirement
+	if !allowedToBypassMinFee && len(feeCoinsZeroDenom) == 0 {
+		// special case: when feeCoins=[] and there is zero coin in fee requirement
+		if len(feeCoins) == 0 && len(zeroCoinFeesDenomReq) != 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.
-
-		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, 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, combinedFees) {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, combinedFees)
+
+		// if feeCoinsNoZeroDenom=[], return false
+		// if nonZeroCoinFeesReq=[], return false (this situation should not happen
+		// because when nonZeroCoinFeesReq empty, and DenomsSubsetOf check passed,
+		// the tx should already passed before)
+		if !feeCoinsNonZeroDenom.IsAnyGTE(nonZeroCoinFeesReq) {
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, combinedFeeRequirement)
 		}
 	}
 
 	return next(ctx, tx, simulate)
 }
 
-// getGlobalFee returns the global fees for a given fee tx's gas (might also returns 0denom if globalMinGasPrice is 0)
+// GetGlobalFee returns the global fees for a given fee tx's gas
+// (might also return 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) {
+func (mfd FeeDecorator) GetGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coins, error) {
 	var (
 		globalMinGasPrices sdk.DecCoins
 		err                error
@@ -185,7 +202,7 @@ func (mfd FeeDecorator) ContainsOnlyBypassMinFeeMsgs(msgs []sdk.Msg) bool {
 	return true
 }
 
-// GetMinGasPrice returns a nodes's local minimum gas prices
+// GetMinGasPrice returns the validator's minimum gas prices
 // fees given a gas limit
 func GetMinGasPrice(ctx sdk.Context, gasLimit int64) sdk.Coins {
 	minGasPrices := ctx.MinGasPrices()

diff --git a/x/globalfee/ante/fee_utils.go b/x/globalfee/ante/fee_utils.go
@@ -4,74 +4,8 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
-// DenomsSubsetOfIncludingZero and IsAnyGTEIncludingZero are similar to DenomsSubsetOf and IsAnyGTE in sdk.
-//
-// 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
-func DenomsSubsetOfIncludingZero(coins, coinsB sdk.Coins) bool {
-	// more denoms in B than in receiver
-	if len(coins) > len(coinsB) {
-		return false
-	}
-	// coins=[], coinsB=[0stake]
-	// let all len(coins) == 0 pass and reject later at IsAnyGTEIncludingZero
-	if len(coins) == 0 && ContainZeroCoins(coinsB) {
-		return true
-	}
-	// coins=1stake, coinsB=[0stake,1uatom]
-	for _, coin := range coins {
-		err := sdk.ValidateDenom(coin.Denom)
-		if err != nil {
-			panic(err)
-		}
-		if ok, _ := Find(coinsB, coin.Denom); !ok {
-			return false
-		}
-	}
-
-	return true
-}
-
-// 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 {
-	// no set is empty set's subset except empty set
-	// this is different from sdk, sdk return false for coinsB empty
-	if len(coinsB) == 0 && len(coins) == 0 {
-		return true
-	}
-	// nothing is gte empty coins
-	if len(coinsB) == 0 && len(coins) != 0 {
-		return false
-	}
-	// if feecoins empty (len(coins)==0 && len(coinsB) != 0 ), and globalfee has one denom of amt zero, return true
-	if len(coins) == 0 {
-		return ContainZeroCoins(coinsB)
-	}
-
-	//  len(coinsB) != 0 && len(coins) != 0
-	// special case: coins=1stake, coinsB=[2stake,0uatom], fail
-	for _, coin := range coins {
-		// not find coin in CoinsB
-		if ok, _ := Find(coinsB, coin.Denom); ok {
-			// find coin in coinsB, and if the amt == 0, mean either coin=0denom or coinsB=0denom...both true
-			amt := coinsB.AmountOf(coin.Denom)
-			if coin.Amount.GTE(amt) {
-				return true
-			}
-		}
-	}
-
-	return false
-}
-
 // ContainZeroCoins returns true if the given coins are empty or contain 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
@@ -88,14 +22,15 @@ func ContainZeroCoins(coins sdk.Coins) bool {
 // 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.
+// if globalfee is empty, CombinedFeeRequirement return sdk.Coins{}
 func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins {
 	// empty min_gas_price
 	if len(minGasPrices) == 0 {
 		return globalFees
 	}
 	// empty global fee is not possible if we set default global fee
 	if len(globalFees) == 0 && len(minGasPrices) != 0 {
-		return globalFees
+		return sdk.Coins{}
 	}
 
 	// if min_gas_price denom is in globalfee, and the amount is higher than globalfee, add min_gas_price to allFees
@@ -139,3 +74,35 @@ func Find(coins sdk.Coins, denom string) (bool, sdk.Coin) {
 		}
 	}
 }
+
+// splitCoinsByDenoms returns the given coins split in two whether
+// their demon is or isn't found in the given denom map.
+func splitCoinsByDenoms(feeCoins sdk.Coins, denomMap map[string]bool) (feeCoinsNonZeroDenom sdk.Coins, feeCoinsZeroDenom sdk.Coins) {
+	for _, fc := range feeCoins {
+		_, found := denomMap[fc.Denom]
+		if found {
+			feeCoinsZeroDenom = append(feeCoinsZeroDenom, fc)
+		} else {
+			feeCoinsNonZeroDenom = append(feeCoinsNonZeroDenom, fc)
+		}
+	}
+
+	return feeCoinsNonZeroDenom.Sort(), feeCoinsZeroDenom.Sort()
+}
+
+// getNonZeroFees returns the given fees nonzero coins
+// and a map storing the zero coins's denoms
+func getNonZeroFees(fees sdk.Coins) (sdk.Coins, map[string]bool) {
+	requiredFeesNonZero := sdk.Coins{}
+	requiredFeesZeroDenom := map[string]bool{}
+
+	for _, gf := range fees {
+		if gf.IsZero() {
+			requiredFeesZeroDenom[gf.Denom] = true
+		} else {
+			requiredFeesNonZero = append(requiredFeesNonZero, gf)
+		}
+	}
+
+	return requiredFeesNonZero.Sort(), requiredFeesZeroDenom
+}