fix: nil check in post (#118)

* fix * fix with check * add early exit * fix
skip-mev · Jun 13, 2024 · f0997fb · f0997fb
1 parent efbd10d
commit f0997fb
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 68 deletions.
diff --git a/docs/INTEGRATIONS.md b/docs/INTEGRATIONS.md
@@ -67,7 +67,7 @@ There are two ways to construct a transaction with `gasPrice`:
 
 ### Understanding Fee Deducted
 
-The actual amount of fee deducted from the fee payer is based on gas consumed, not `gasLimit`.
+The actual amount of fee deducted from the fee payer is based on gas consumed, not `gasLimit`.  The total amount deducted (`fee + tip`) will be equal to the amount of fee specified on your transaction.
 
 The amount consumed is equal to the `inferredTip + gasPrice * gasConsumed`, where `inferredTip = feeAmount - gasLimit * gasPrice`  (This may be different than the tip you specified when building the transaction because the `gasPrice` on chain may have changed since when you queried it.)
 

diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go
@@ -135,7 +135,6 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula
 		}
 
 		ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice))
-		return next(ctx, tx, simulate)
 	}
 	return next(ctx, tx, simulate)
 }

diff --git a/x/feemarket/post/fee.go b/x/feemarket/post/fee.go
@@ -2,6 +2,7 @@ package post
 
 import (
 	"bytes"
+	"cosmossdk.io/math"
 	"fmt"
 
 	errorsmod "cosmossdk.io/errors"
@@ -54,11 +55,6 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
 		return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidGasLimit, "must provide positive gas")
 	}
 
-	var (
-		tip     sdk.Coin
-		payCoin sdk.Coin
-	)
-
 	// update fee market params
 	params, err := dfd.feemarketKeeper.GetParams(ctx)
 	if err != nil {
@@ -89,6 +85,11 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
 	feeCoin := feeCoins[0]
 	feeGas := int64(feeTx.GetGas())
 
+	var (
+		tip     = sdk.NewCoin(feeCoin.Denom, math.ZeroInt())
+		payCoin = feeCoin
+	)
+
 	minGasPrice, err := dfd.feemarketKeeper.GetMinGasPrice(ctx, feeCoin.GetDenom())
 	if err != nil {
 		return ctx, errorsmod.Wrapf(err, "unable to get min gas price for denom %s", feeCoins[0].GetDenom())
@@ -107,7 +108,7 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
 	}
 
 	ctx.Logger().Info("fee deduct post handle",
-		"fee", feeCoins,
+		"fee", payCoin,
 		"tip", tip,
 	)
 
@@ -160,9 +161,11 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
 		if dfd.feegrantKeeper == nil {
 			return sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled")
 		} else if !bytes.Equal(feeGranter, feePayer) {
-			err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, sdk.NewCoins(fee), sdkTx.GetMsgs())
-			if err != nil {
-				return errorsmod.Wrapf(err, "%s does not allow to pay fees for %s", feeGranter, feePayer)
+			if !fee.IsNil() {
+				err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, sdk.NewCoins(fee), sdkTx.GetMsgs())
+				if err != nil {
+					return errorsmod.Wrapf(err, "%s does not allow to pay fees for %s", feeGranter, feePayer)
+				}
 			}
 		}
 
@@ -177,7 +180,7 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
 	var events sdk.Events
 
 	// deduct the fees and tip
-	if !fee.Amount.IsNil() && !fee.IsZero() {
+	if !fee.IsNil() {
 		err := DeductCoins(dfd.bankKeeper, ctx, deductFeesFromAcc, sdk.NewCoins(fee), distributeFees)
 		if err != nil {
 			return err
@@ -191,7 +194,7 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
 	}
 
 	proposer := sdk.AccAddress(ctx.BlockHeader().ProposerAddress)
-	if !tip.Amount.IsNil() && !tip.IsZero() {
+	if !tip.IsNil() {
 		err := SendTip(dfd.bankKeeper, ctx, deductFeesFromAcc.GetAddress(), proposer, sdk.NewCoins(tip))
 		if err != nil {
 			return err
@@ -212,10 +215,6 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
 // DeductCoins deducts coins from the given account.
 // Coins can be sent to the default fee collector (causes coins to be distributed to stakers) or sent to the feemarket fee collector account (causes coins to be burned).
 func DeductCoins(bankKeeper BankKeeper, ctx sdk.Context, acc sdk.AccountI, coins sdk.Coins, distributeFees bool) error {
-	if !coins.IsValid() {
-		return errorsmod.Wrapf(sdkerrors.ErrInsufficientFee, "invalid coin amount: %s", coins)
-	}
-
 	targetModuleAcc := feemarkettypes.FeeCollectorName
 	if distributeFees {
 		targetModuleAcc = authtypes.FeeCollectorName
@@ -231,10 +230,6 @@ func DeductCoins(bankKeeper BankKeeper, ctx sdk.Context, acc sdk.AccountI, coins
 
 // SendTip sends a tip to the current block proposer.
 func SendTip(bankKeeper BankKeeper, ctx sdk.Context, acc, proposer sdk.AccAddress, coins sdk.Coins) error {
-	if !coins.IsValid() {
-		return errorsmod.Wrapf(sdkerrors.ErrInsufficientFee, "invalid coin amount: %s", coins)
-	}
-
 	err := bankKeeper.SendCoins(ctx, acc, proposer, coins)
 	if err != nil {
 		return err

diff --git a/x/feemarket/post/fee_test.go b/x/feemarket/post/fee_test.go
@@ -18,10 +18,9 @@ import (
 
 func TestDeductCoins(t *testing.T) {
 	tests := []struct {
-		name        string
-		coins       sdk.Coins
-		wantErr     bool
-		invalidCoin bool
+		name    string
+		coins   sdk.Coins
+		wantErr bool
 	}{
 		{
 			name:    "valid",
@@ -34,25 +33,16 @@ func TestDeductCoins(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name:        "invalid coins negative amount",
-			coins:       sdk.Coins{sdk.Coin{Denom: "test", Amount: math.NewInt(-1)}},
-			wantErr:     true,
-			invalidCoin: true,
-		},
-		{
-			name:        "invalid coins invalid denom",
-			coins:       sdk.Coins{sdk.Coin{Amount: math.NewInt(1)}},
-			wantErr:     true,
-			invalidCoin: true,
+			name:    "valid zero coin",
+			coins:   sdk.NewCoins(sdk.NewCoin("test", math.ZeroInt())),
+			wantErr: false,
 		},
 	}
 	for _, tc := range tests {
 		t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) {
 			s := antesuite.SetupTestSuite(t, true)
 			acc := s.CreateTestAccounts(1)[0]
-			if !tc.invalidCoin {
-				s.MockBankKeeper.On("SendCoinsFromAccountToModule", s.Ctx, acc.Account.GetAddress(), types.FeeCollectorName, tc.coins).Return(nil).Once()
-			}
+			s.MockBankKeeper.On("SendCoinsFromAccountToModule", s.Ctx, acc.Account.GetAddress(), types.FeeCollectorName, tc.coins).Return(nil).Once()
 
 			if err := post.DeductCoins(s.MockBankKeeper, s.Ctx, acc.Account, tc.coins, false); (err != nil) != tc.wantErr {
 				s.Errorf(err, "DeductCoins() error = %v, wantErr %v", err, tc.wantErr)
@@ -63,10 +53,9 @@ func TestDeductCoins(t *testing.T) {
 
 func TestDeductCoinsAndDistribute(t *testing.T) {
 	tests := []struct {
-		name        string
-		coins       sdk.Coins
-		wantErr     bool
-		invalidCoin bool
+		name    string
+		coins   sdk.Coins
+		wantErr bool
 	}{
 		{
 			name:    "valid",
@@ -79,25 +68,16 @@ func TestDeductCoinsAndDistribute(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name:        "invalid coins negative amount",
-			coins:       sdk.Coins{sdk.Coin{Denom: "test", Amount: math.NewInt(-1)}},
-			wantErr:     true,
-			invalidCoin: true,
-		},
-		{
-			name:        "invalid coins invalid denom",
-			coins:       sdk.Coins{sdk.Coin{Amount: math.NewInt(1)}},
-			wantErr:     true,
-			invalidCoin: true,
+			name:    "valid zero coin",
+			coins:   sdk.NewCoins(sdk.NewCoin("test", math.ZeroInt())),
+			wantErr: false,
 		},
 	}
 	for _, tc := range tests {
 		t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) {
 			s := antesuite.SetupTestSuite(t, true)
 			acc := s.CreateTestAccounts(1)[0]
-			if !tc.invalidCoin {
-				s.MockBankKeeper.On("SendCoinsFromAccountToModule", s.Ctx, acc.Account.GetAddress(), authtypes.FeeCollectorName, tc.coins).Return(nil).Once()
-			}
+			s.MockBankKeeper.On("SendCoinsFromAccountToModule", s.Ctx, acc.Account.GetAddress(), authtypes.FeeCollectorName, tc.coins).Return(nil).Once()
 
 			if err := post.DeductCoins(s.MockBankKeeper, s.Ctx, acc.Account, tc.coins, true); (err != nil) != tc.wantErr {
 				s.Errorf(err, "DeductCoins() error = %v, wantErr %v", err, tc.wantErr)
@@ -108,10 +88,9 @@ func TestDeductCoinsAndDistribute(t *testing.T) {
 
 func TestSendTip(t *testing.T) {
 	tests := []struct {
-		name        string
-		coins       sdk.Coins
-		wantErr     bool
-		invalidCoin bool
+		name    string
+		coins   sdk.Coins
+		wantErr bool
 	}{
 		{
 			name:    "valid",
@@ -124,19 +103,16 @@ func TestSendTip(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name:        "invalid coins",
-			coins:       sdk.Coins{sdk.Coin{Amount: math.NewInt(-1)}},
-			wantErr:     true,
-			invalidCoin: true,
+			name:    "valid zero coin",
+			coins:   sdk.NewCoins(sdk.NewCoin("test", math.ZeroInt())),
+			wantErr: false,
 		},
 	}
 	for _, tc := range tests {
 		t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) {
 			s := antesuite.SetupTestSuite(t, true)
 			accs := s.CreateTestAccounts(2)
-			if !tc.invalidCoin {
-				s.MockBankKeeper.On("SendCoins", s.Ctx, mock.Anything, mock.Anything, tc.coins).Return(nil).Once()
-			}
+			s.MockBankKeeper.On("SendCoins", s.Ctx, mock.Anything, mock.Anything, tc.coins).Return(nil).Once()
 
 			if err := post.SendTip(s.MockBankKeeper, s.Ctx, accs[0].Account.GetAddress(), accs[1].Account.GetAddress(), tc.coins); (err != nil) != tc.wantErr {
 				s.Errorf(err, "SendTip() error = %v, wantErr %v", err, tc.wantErr)
@@ -180,10 +156,28 @@ func TestPostHandle(t *testing.T) {
 			ExpPass:  false,
 			ExpErr:   sdkerrors.ErrInsufficientFunds,
 		},
+		{
+			Name: "signer has no funds - simulate",
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
+				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(sdkerrors.ErrInsufficientFunds)
+
+				return antesuite.TestCaseArgs{
+					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
+					GasLimit:  gasLimit,
+					FeeAmount: validFee,
+				}
+			},
+			RunAnte:  true,
+			RunPost:  true,
+			Simulate: true,
+			ExpPass:  false,
+			ExpErr:   sdkerrors.ErrInsufficientFunds,
+		},
 		{
 			Name: "0 gas given should fail",
-			Malleate: func(suite *antesuite.TestSuite) antesuite.TestCaseArgs {
-				accs := suite.CreateTestAccounts(1)
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
 
 				return antesuite.TestCaseArgs{
 					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
@@ -197,11 +191,31 @@ func TestPostHandle(t *testing.T) {
 			ExpPass:  false,
 			ExpErr:   sdkerrors.ErrInvalidGasLimit,
 		},
+		{
+			Name: "0 gas given should pass - simulate",
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
+				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+
+				return antesuite.TestCaseArgs{
+					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
+					GasLimit:  0,
+					FeeAmount: validFee,
+				}
+			},
+			RunAnte:  true,
+			RunPost:  true,
+			Simulate: true,
+			ExpPass:  true,
+			ExpErr:   nil,
+		},
 		{
 			Name: "signer has enough funds, should pass, no tip",
 			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
 				accs := s.CreateTestAccounts(1)
 				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
 
 				return antesuite.TestCaseArgs{
 					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
@@ -234,11 +248,31 @@ func TestPostHandle(t *testing.T) {
 			ExpPass:  true,
 			ExpErr:   nil,
 		},
+		{
+			Name: "signer has enough funds, should pass with tip - simulate",
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
+				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+
+				return antesuite.TestCaseArgs{
+					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
+					GasLimit:  gasLimit,
+					FeeAmount: validFeeWithTip,
+				}
+			},
+			RunAnte:  true,
+			RunPost:  true,
+			Simulate: true,
+			ExpPass:  true,
+			ExpErr:   nil,
+		},
 		{
 			Name: "signer has enough funds, should pass, no tip - resolvable denom",
 			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
 				accs := s.CreateTestAccounts(1)
 				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
 
 				return antesuite.TestCaseArgs{
 					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
@@ -252,6 +286,25 @@ func TestPostHandle(t *testing.T) {
 			ExpPass:  true,
 			ExpErr:   nil,
 		},
+		{
+			Name: "signer has enough funds, should pass, no tip - resolvable denom - simulate",
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
+				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+
+				return antesuite.TestCaseArgs{
+					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
+					GasLimit:  gasLimit,
+					FeeAmount: validResolvableFee,
+				}
+			},
+			RunAnte:  true,
+			RunPost:  true,
+			Simulate: true,
+			ExpPass:  true,
+			ExpErr:   nil,
+		},
 		{
 			Name: "signer has enough funds, should pass with tip - resolvable denom",
 			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
@@ -271,6 +324,25 @@ func TestPostHandle(t *testing.T) {
 			ExpPass:  true,
 			ExpErr:   nil,
 		},
+		{
+			Name: "signer has enough funds, should pass with tip - resolvable denom - simulate",
+			Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs {
+				accs := s.CreateTestAccounts(1)
+				s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil)
+				s.MockBankKeeper.On("SendCoins", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+
+				return antesuite.TestCaseArgs{
+					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
+					GasLimit:  gasLimit,
+					FeeAmount: validResolvableFeeWithTip,
+				}
+			},
+			RunAnte:  true,
+			RunPost:  true,
+			Simulate: true,
+			ExpPass:  true,
+			ExpErr:   nil,
+		},
 	}
 
 	for _, tc := range testCases {