Some txs are hitting FeePay when no --fee is used (#874)

* all Wasm Txs  are hitting feepay, even if not 0 fee

* v1800alpha2 upgrade

* Handle zero fee in helper

* Add sdk deduct fee handler as fallback

* Fix linting errors

* Add e2e testing for fallback handler

* bump: packet-forward-middleware/v7 v7.0.1

* globalfee: only check insufficient fees err

* lint

* Works.

* Refactor Fee Decorator Logic

* fix: v18alpha upgrade checker

---------

Co-authored-by: Joel Smith <[email protected]>

app/ante.go

@@ -79,6 +79,16 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 		sigGasConsumer = ante.DefaultSigVerificationGasConsumer
 	}
 
+	// Flag for determining if the tx is a FeePay transaction. This flag
+	// is used to communicate between the FeePay decorator and the GlobalFee decorator.
+	isFeePayTx := false
+
+	// Define FeePay and Global Fee decorators. These decorators are called in different orders based on the type of
+	// transaction. The FeePay decorator is called first for FeePay transactions, and the GlobalFee decorator is called
+	// first for all other transactions. See the FeeRouteDecorator for more details.
+	fpd := feepayante.NewDeductFeeDecorator(options.FeePayKeeper, options.GlobalFeeKeeper, options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.BondDenom, &isFeePayTx)
+	gfd := globalfeeante.NewFeeDecorator(options.BypassMinFeeMsgTypes, options.GlobalFeeKeeper, options.StakingKeeper, maxBypassMinFeeMsgGasUsage, &isFeePayTx)
+
 	anteDecorators := []sdk.AnteDecorator{
 		// GLobalFee query params for minimum fee
 		ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
@@ -90,8 +100,11 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 		ante.NewTxTimeoutHeightDecorator(),
 		ante.NewValidateMemoDecorator(options.AccountKeeper),
 		ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
-		globalfeeante.NewFeeDecorator(options.BypassMinFeeMsgTypes, options.GlobalFeeKeeper, options.StakingKeeper, options.FeePayKeeper, maxBypassMinFeeMsgGasUsage),
-		feepayante.NewDeductFeeDecorator(options.FeePayKeeper, options.GlobalFeeKeeper, options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.BondDenom),
+
+		// Fee route decorator calls FeePay and Global Fee decorators in different orders
+		// depending on the type of incoming tx.
+		feepayante.NewFeeRouteDecorator(options.FeePayKeeper, &fpd, &gfd, &isFeePayTx),
+
 		feeshareante.NewFeeSharePayoutDecorator(options.BankKeeper, options.FeeShareKeeper),
 		// SetPubKeyDecorator must be called before all signature verification decorators
 		ante.NewSetPubKeyDecorator(options.AccountKeeper),

app/app.go

@@ -66,6 +66,7 @@ import (
 	"github.com/CosmosContracts/juno/v18/app/keepers"
 	"github.com/CosmosContracts/juno/v18/app/openapiconsole"
 	upgrades "github.com/CosmosContracts/juno/v18/app/upgrades"
+	testnetV18alpha2 "github.com/CosmosContracts/juno/v18/app/upgrades/testnet/v18.0.0-alpha.2"
 	v10 "github.com/CosmosContracts/juno/v18/app/upgrades/v10"
 	v11 "github.com/CosmosContracts/juno/v18/app/upgrades/v11"
 	v12 "github.com/CosmosContracts/juno/v18/app/upgrades/v12"
@@ -97,6 +98,9 @@ var (
 	EnableSpecificProposals = ""
 
 	Upgrades = []upgrades.Upgrade{
+		// testnet
+		testnetV18alpha2.Upgrade,
+
 		v10.Upgrade,
 		v11.Upgrade,
 		v12.Upgrade,

app/keepers/keepers.go

@@ -11,9 +11,9 @@ import (
 	buildertypes "github.com/skip-mev/pob/x/builder/types"
 	"github.com/spf13/cast"
 
-	packetforward "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router"
-	packetforwardkeeper "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/keeper"
-	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types"
+	packetforward "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward"
+	packetforwardkeeper "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/keeper"
+	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/types"
 	icq "github.com/cosmos/ibc-apps/modules/async-icq/v7"
 	icqkeeper "github.com/cosmos/ibc-apps/modules/async-icq/v7/keeper"
 	icqtypes "github.com/cosmos/ibc-apps/modules/async-icq/v7/types"

app/keepers/keys.go

@@ -4,7 +4,7 @@ import (
 	wasmtypes "github.com/CosmWasm/wasmd/x/wasm/types"
 	buildertypes "github.com/skip-mev/pob/x/builder/types"
 
-	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types"
+	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/types"
 	icqtypes "github.com/cosmos/ibc-apps/modules/async-icq/v7/types"
 	ibchookstypes "github.com/cosmos/ibc-apps/modules/ibc-hooks/v7/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"

app/modules.go

@@ -6,8 +6,8 @@ import (
 	buildermodule "github.com/skip-mev/pob/x/builder"
 	buildertypes "github.com/skip-mev/pob/x/builder/types"
 
-	packetforward "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router"
-	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types"
+	packetforward "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward"
+	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/types"
 	icq "github.com/cosmos/ibc-apps/modules/async-icq/v7"
 	icqtypes "github.com/cosmos/ibc-apps/modules/async-icq/v7/types"
 	ibc_hooks "github.com/cosmos/ibc-apps/modules/ibc-hooks/v7"

app/upgrades/testnet/v18.0.0-alpha.2/constants.go

@@ -0,0 +1,16 @@
+package v18
+
+import (
+	store "github.com/cosmos/cosmos-sdk/store/types"
+
+	"github.com/CosmosContracts/juno/v18/app/upgrades"
+)
+
+// UpgradeName defines the on-chain upgrade name for the upgrade.
+const UpgradeName = "v1800alpha2"
+
+var Upgrade = upgrades.Upgrade{
+	UpgradeName:          UpgradeName,
+	CreateUpgradeHandler: upgrades.CreateBlankUpgradeHandler,
+	StoreUpgrades:        store.StoreUpgrades{},
+}

app/upgrades/testnet/v18.0.0-alpha.2/upgrade_test.go

@@ -0,0 +1,40 @@
+package v18_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+
+	"github.com/CosmosContracts/juno/v18/app/apptesting"
+	v18alpha2 "github.com/CosmosContracts/juno/v18/app/upgrades/testnet/v18.0.0-alpha.2"
+)
+
+type UpgradeTestSuite struct {
+	apptesting.KeeperTestHelper
+}
+
+func (s *UpgradeTestSuite) SetupTest() {
+	s.Setup()
+}
+
+func TestKeeperTestSuite(t *testing.T) {
+	suite.Run(t, new(UpgradeTestSuite))
+}
+
+// Ensures the test does not error out.
+func (s *UpgradeTestSuite) TestUpgrade() {
+	s.Setup()
+
+	preUpgradeChecks(s)
+
+	upgradeHeight := int64(5)
+	s.ConfirmUpgradeSucceeded(v18alpha2.UpgradeName, upgradeHeight)
+
+	postUpgradeChecks(s)
+}
+
+func preUpgradeChecks(_ *UpgradeTestSuite) {
+}
+
+func postUpgradeChecks(_ *UpgradeTestSuite) {
+}

app/upgrades/types.go

@@ -46,3 +46,14 @@ func GetChainsDenomToken(chainID string) string {
 	}
 	return "ujuno"
 }
+
+// A blank upgrade handler with no logic, just a migration.
+func CreateBlankUpgradeHandler(
+	mm *module.Manager,
+	cfg module.Configurator,
+	_ *keepers.AppKeepers,
+) upgradetypes.UpgradeHandler {
+	return func(ctx sdk.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
+		return mm.RunMigrations(ctx, cfg, vm)
+	}
+}

app/upgrades/v13/constants.go

@@ -1,7 +1,7 @@
 package v13
 
 import (
-	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types"
+	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/types"
 	ibchookstypes "github.com/cosmos/ibc-apps/modules/ibc-hooks/v7/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	ibcfeetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"

app/upgrades/v13/upgrades.go

@@ -3,7 +3,7 @@ package v13
 import (
 	"fmt"
 
-	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types"
+	packetforwardtypes "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/packetforward/types"
 	// ICA
 	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icahosttypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"

go.mod

@@ -17,7 +17,7 @@ require (
 	github.com/cometbft/cometbft-db v0.8.0
 	github.com/cosmos/cosmos-sdk v0.47.5
 	github.com/cosmos/gogoproto v1.4.10
-	github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1-0.20231012160012-d0f49580a238
+	github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1
 	github.com/cosmos/ibc-apps/modules/async-icq/v7 v7.0.0
 	github.com/cosmos/ibc-apps/modules/ibc-hooks/v7 v7.0.0-20230803181732-7c8f814d3b79
 	github.com/cosmos/ibc-go/v7 v7.3.1

go.sum

@@ -411,8 +411,8 @@ github.com/cosmos/gogoproto v1.4.10 h1:QH/yT8X+c0F4ZDacDv3z+xE3WU1P1Z3wQoLMBRJoK
 github.com/cosmos/gogoproto v1.4.10/go.mod h1:3aAZzeRWpAwr+SS/LLkICX2/kDFyaYVzckBDzygIxek=
 github.com/cosmos/iavl v0.20.1 h1:rM1kqeG3/HBT85vsZdoSNsehciqUQPWrR4BYmqE2+zg=
 github.com/cosmos/iavl v0.20.1/go.mod h1:WO7FyvaZJoH65+HFOsDir7xU9FWk2w9cHXNW1XHcl7A=
-github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1-0.20231012160012-d0f49580a238 h1:vc9zQUjiYctU3q4uF5usbl2JUqaa3F6bEyboNKOKyBk=
-github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1-0.20231012160012-d0f49580a238/go.mod h1:WO/xWf+I8m+9PZsBkwZkEREZIW24j90Sz2yOsXU3B9U=
+github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1 h1:8mK4Ha/56P6jkRcLhIYhg/ipWhEuXBtj5O4I6fAi6vg=
+github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7 v7.0.1/go.mod h1:GGUJN4LnB3J1Luu4kxTklQLbW69So3QXUndSalB003s=
 github.com/cosmos/ibc-apps/modules/async-icq/v7 v7.0.0 h1:mMHedP3Q+mz5gpOWNz0P+X8hxPdamylrBKc/P2cFakA=
 github.com/cosmos/ibc-apps/modules/async-icq/v7 v7.0.0/go.mod h1:/P6l2bWo2AR3rrsfs0DHuFZO3Imzb93sxFD3ihrIgw4=
 github.com/cosmos/ibc-apps/modules/ibc-hooks/v7 v7.0.0-20230803181732-7c8f814d3b79 h1:pCxyhIxgWTabAQC5UerkITraHG3SwajdLKKMCFDWCv4=

interchaintest/go.mod

@@ -2,7 +2,7 @@ module github.com/CosmosContracts/juno/tests/interchaintest
 
 go 1.21
 
-toolchain go1.21.0
+// toolchain go1.21.0
 
 replace (
 	// interchaintest supports ICS features so we need this for now

interchaintest/module_feepay_test.go

@@ -5,11 +5,12 @@ import (
 	"strconv"
 	"testing"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/strangelove-ventures/interchaintest/v7"
 	"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
 	"github.com/stretchr/testify/require"
 
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
 	helpers "github.com/CosmosContracts/juno/tests/interchaintest/helpers"
 )
 
@@ -98,6 +99,62 @@ func TestJunoFeePay(t *testing.T) {
 	t.Log("uses", uses)
 	require.Equal(t, uses.Uses, "1")
 
+	// Instantiate a new contract
+	contractAddr, err = juno.InstantiateContract(ctx, admin.KeyName(), codeId, `{"count":0}`, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Succeed - Test a regular CW contract with fees, regular sdk logic handles Tx
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--fees", "500"+nativeDenom)
+	require.NoError(t, err)
+	fmt.Println("txHash", txHash)
+
+	// Fail - Testing an unregistered contract with no fees, FeePay Tx logic will fail it due to not being registered
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--fees", "0"+nativeDenom)
+	require.Error(t, err)
+	fmt.Println("txHash", txHash)
+
+	// Register the new contract with a limit of 1, fund contract
+	helpers.RegisterFeePay(t, ctx, juno, admin, contractAddr, 1)
+	helpers.FundFeePayContract(t, ctx, juno, admin, contractAddr, strconv.Itoa(balance)+nativeDenom)
+
+	// Test the registered contract - with fees
+	// Will succeed, routed through normal sdk because a fee was provided
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--fees", "500"+nativeDenom)
+	require.NoError(t, err)
+	fmt.Println("txHash", txHash)
+
+	// Before balance - should be the same as after balance (feepay covers fee)
+	// Calculated before interacting with a registered contract to ensure the
+	// contract covers the fee.
+	beforeBal, err = juno.GetBalance(ctx, user.FormattedAddress(), nativeDenom)
+	require.NoError(t, err)
+
+	// Test the registered FeePay contract - without providing fees
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--fees", "0"+nativeDenom)
+	require.NoError(t, err)
+	fmt.Println("txHash", txHash)
+
+	// After balance
+	afterBal, err = juno.GetBalance(ctx, user.FormattedAddress(), nativeDenom)
+	require.NoError(t, err)
+
+	// Validate users balance did not change
+	require.Equal(t, beforeBal, afterBal)
+
+	// Test the fallback sdk route is triggered when the FeePay Tx fails
+	// Fail - Test the registered contract - without fees, exceeded wallet limit
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--fees", "0"+nativeDenom)
+	require.Error(t, err)
+	fmt.Println("txHash", txHash)
+
+	// Test the registered contract - without fees, but specified gas
+	// Tx should succeed, because it uses the sdk fallback route
+	txHash, err = juno.ExecuteContract(ctx, user.KeyName(), contractAddr, `{"increment":{}}`, "--gas", "200000")
+	require.NoError(t, err)
+	fmt.Println("txHash", txHash)
+
 	t.Cleanup(func() {
 		_ = ic.Close()
 	})

x/feepay/ante/deduct_fee.go

@@ -15,7 +15,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
 	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
 
-	feepayhelpers "github.com/CosmosContracts/juno/v18/x/feepay/helpers"
 	feepaykeeper "github.com/CosmosContracts/juno/v18/x/feepay/keeper"
 	feepaytypes "github.com/CosmosContracts/juno/v18/x/feepay/types"
 	globalfeekeeper "github.com/CosmosContracts/juno/v18/x/globalfee/keeper"
@@ -36,16 +35,18 @@ type DeductFeeDecorator struct {
 	bankKeeper      bankkeeper.Keeper
 	feegrantKeeper  ante.FeegrantKeeper
 	bondDenom       string
+	isFeePayTx      *bool
 }
 
-func NewDeductFeeDecorator(fpk feepaykeeper.Keeper, gfk globalfeekeeper.Keeper, ak ante.AccountKeeper, bk bankkeeper.Keeper, fgk ante.FeegrantKeeper, bondDenom string) DeductFeeDecorator {
+func NewDeductFeeDecorator(fpk feepaykeeper.Keeper, gfk globalfeekeeper.Keeper, ak ante.AccountKeeper, bk bankkeeper.Keeper, fgk ante.FeegrantKeeper, bondDenom string, isFeePayTx *bool) DeductFeeDecorator {
 	return DeductFeeDecorator{
 		feepayKeeper:    fpk,
 		globalfeeKeeper: gfk,
 		accountKeeper:   ak,
 		bankKeeper:      bk,
 		feegrantKeeper:  fgk,
 		bondDenom:       bondDenom,
+		isFeePayTx:      isFeePayTx,
 	}
 }
 
@@ -114,17 +115,35 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee
 		return sdkerrors.ErrUnknownAddress.Wrapf("fee payer address: %s does not exist", deductFeesFrom)
 	}
 
-	if feepayhelpers.IsValidFeePayTransaction(ctx, dfd.feepayKeeper, sdkTx, fee) {
-		err := dfd.handleZeroFees(ctx, deductFeesFromAcc, sdkTx, fee)
-		if err != nil {
-			return err
+	// Define errors per route
+	var feePayErr error
+	var sdkErr error
+
+	// First try to handle FeePay transactions, if error, try the std sdk route.
+	// If not a FeePay transaction, default to the std sdk route.
+	if *dfd.isFeePayTx {
+		// If the fee pay route fails, try the std sdk route
+		feePayErr = dfd.handleZeroFees(ctx, deductFeesFromAcc, sdkTx, fee)
+		if feePayErr != nil {
+			// Flag the tx to be processed by GlobalFee
+			*dfd.isFeePayTx = false
+
+			// call GlobalFee handler here
+			sdkErr = DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee)
 		}
+		// caught in globalfee
 	} else if !fee.IsZero() {
 		// Std sdk route
-		err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee)
-		if err != nil {
-			return err
+		sdkErr = DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee)
+	}
+
+	// If no fee pay error exists, the tx processed successfully. If
+	// a sdk error is present, return all errors.
+	if sdkErr != nil {
+		if feePayErr != nil {
+			return errorsmod.Wrapf(feepaytypes.ErrDeductFees, "error deducting fees; fee pay error: %s, sdk error: %s", feePayErr, sdkErr)
 		}
+		return sdkErr
 	}
 
 	events := sdk.Events{
@@ -225,7 +244,7 @@ func (dfd DeductFeeDecorator) checkTxFeeWithValidatorMinGasPrices(ctx sdk.Contex
 	// Ensure that the provided fees meet a minimum threshold for the validator,
 	// if this is a CheckTx. This is only for local mempool purposes, and thus
 	// is only ran on check tx.
-	if ctx.IsCheckTx() && !feepayhelpers.IsValidFeePayTransaction(ctx, dfd.feepayKeeper, tx, feeTx.GetFee()) {
+	if ctx.IsCheckTx() && !*dfd.isFeePayTx {
 		minGasPrices := ctx.MinGasPrices()
 		if !minGasPrices.IsZero() {
 			requiredFees := make(sdk.Coins, len(minGasPrices))