chore: (x/feegrant) update keeper logic to do not allow duplicate grant

CHANGELOG.md

@@ -145,6 +145,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/auth/vesting) [#13502](https://github.com/cosmos/cosmos-sdk/pull/13502) Add Amino Msg registration for `MsgCreatePeriodicVestingAccount`.
 * (x/auth)[#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to account-id(type of uint64) to use `AccountAddressByID`.
 * (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
+* (x/feegrant) [#14294](https://github.com/cosmos/cosmos-sdk/pull/14294) moved the logic of rejecting duplicate grant from `msg_server` to `keeper` method.
 * (store) [#14378](https://github.com/cosmos/cosmos-sdk/pull/14378) The `CacheKV` store is thread-safe again, which includes improved iteration and deletion logic. Iteration is on a strictly isolated view now, which is breaking from previous behavior.
 
 ### API Breaking Changes

x/feegrant/keeper/keeper.go

@@ -41,6 +41,11 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 
 // GrantAllowance creates a new grant
 func (k Keeper) GrantAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress, feeAllowance feegrant.FeeAllowanceI) error {
+	// Checking for duplicate entry
+	if f, _ := k.GetAllowance(ctx, granter, grantee); f != nil {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "fee allowance already exists")
+	}
+
 	// create the account if it is not in account state
 	granteeAcc := k.authKeeper.GetAccount(ctx, grantee)
 	if granteeAcc == nil {
@@ -51,54 +56,21 @@ func (k Keeper) GrantAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress,
 	store := ctx.KVStore(k.storeKey)
 	key := feegrant.FeeAllowanceKey(granter, grantee)
 
-	var oldExp *time.Time
-	existingGrant, err := k.getGrant(ctx, granter, grantee)
-
-	// If we didn't find any grant, we don't return any error.
-	// All other kinds of errors are returned.
-	if err != nil && !sdkerrors.IsOf(err, sdkerrors.ErrNotFound) {
+	exp, err := feeAllowance.ExpiresAt()
+	if err != nil {
 		return err
 	}
 
-	if existingGrant != nil && existingGrant.GetAllowance() != nil {
-		grantInfo, err := existingGrant.GetGrant()
-		if err != nil {
-			return err
-		}
-
-		oldExp, err = grantInfo.ExpiresAt()
-		if err != nil {
-			return err
-		}
-	}
-
-	newExp, err := feeAllowance.ExpiresAt()
-	switch {
-	case err != nil:
-		return err
-
-	case newExp != nil && newExp.Before(ctx.BlockTime()):
+	// expiration shouldn't be in the past.
+	if exp != nil && exp.Before(ctx.BlockTime()) {
 		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "expiration is before current block time")
+	}
 
-	case oldExp == nil && newExp != nil:
-		// when old oldExp is nil there won't be any key added before to queue.
-		// add the new key to queue directly.
-		k.addToFeeAllowanceQueue(ctx, key[1:], newExp)
-
-	case oldExp != nil && newExp == nil:
-		// when newExp is nil no need of adding the key to the pruning queue
-		// remove the old key from queue.
-		k.removeFromGrantQueue(ctx, oldExp, key[1:])
-
-	case oldExp != nil && newExp != nil && !oldExp.Equal(*newExp):
+	// if expiry is not nil, add the new key to pruning queue.
+	if exp != nil {
 		// `key` formed here with the prefix of `FeeAllowanceKeyPrefix` (which is `0x00`)
-		// remove the 1st byte and reuse the remaining key as it is.
-
-		// remove the old key from queue.
-		k.removeFromGrantQueue(ctx, oldExp, key[1:])
-
-		// add the new key to queue.
-		k.addToFeeAllowanceQueue(ctx, key[1:], newExp)
+		// remove the 1st byte and reuse the remaining key as it is
+		k.addToFeeAllowanceQueue(ctx, key[1:], exp)
 	}
 
 	grant, err := feegrant.NewGrant(granter, grantee, feeAllowance)
@@ -312,12 +284,6 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) (*feegrant.GenesisState, error) {
 	}, err
 }
 
-func (k Keeper) removeFromGrantQueue(ctx sdk.Context, exp *time.Time, allowanceKey []byte) {
-	key := feegrant.FeeAllowancePrefixQueue(exp, allowanceKey)
-	store := ctx.KVStore(k.storeKey)
-	store.Delete(key)
-}
-
 func (k Keeper) addToFeeAllowanceQueue(ctx sdk.Context, grantKey []byte, exp *time.Time) {
 	store := ctx.KVStore(k.storeKey)
 	store.Set(feegrant.FeeAllowancePrefixQueue(exp, grantKey), []byte{})

x/feegrant/keeper/keeper_test.go

@@ -73,24 +73,35 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 	}
 
 	// let's set up some initial state here
+
+	// addrs[0] -> addrs[1] (basic)
 	err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[0], suite.addrs[1], basic)
 	suite.Require().NoError(err)
 
+	// addrs[0] -> addrs[2] (basic2)
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[0], suite.addrs[2], basic2)
 	suite.Require().NoError(err)
 
+	// addrs[1] -> addrs[2] (basic)
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[1], suite.addrs[2], basic)
 	suite.Require().NoError(err)
 
+	// addrs[1] -> addrs[3] (basic)
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[1], suite.addrs[3], basic)
 	suite.Require().NoError(err)
 
+	// addrs[3] -> addrs[0] (basic2)
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[3], suite.addrs[0], basic2)
 	suite.Require().NoError(err)
 
+	// addrs[3] -> addrs[0] (basic2) expect error with duplicate grant
+	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[3], suite.addrs[0], basic2)
+	suite.Require().Error(err)
+
 	// remove some, overwrite other
 	_, err = suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{Granter: suite.addrs[0].String(), Grantee: suite.addrs[1].String()})
 	suite.Require().NoError(err)
+
 	_, err = suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{Granter: suite.addrs[0].String(), Grantee: suite.addrs[2].String()})
 	suite.Require().NoError(err)
 
@@ -101,6 +112,10 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[0], suite.addrs[2], basic)
 	suite.Require().NoError(err)
 
+	// revoke an existing grant and grant again with different allowance.
+	_, err = suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{Granter: suite.addrs[1].String(), Grantee: suite.addrs[2].String()})
+	suite.Require().NoError(err)
+
 	err = suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[1], suite.addrs[2], basic3)
 	suite.Require().NoError(err)
 
@@ -188,34 +203,50 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 		fee     sdk.Coins
 		allowed bool
 		final   feegrant.FeeAllowanceI
+		postRun func()
 	}{
 		"use entire pot": {
 			granter: suite.addrs[0],
 			grantee: suite.addrs[1],
 			fee:     suite.atom,
 			allowed: true,
 			final:   nil,
+			postRun: func() {},
 		},
 		"too high": {
 			granter: suite.addrs[0],
 			grantee: suite.addrs[1],
 			fee:     hugeAtom,
 			allowed: false,
 			final:   future,
+			postRun: func() {
+				_, err := suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{
+					Granter: suite.addrs[0].String(),
+					Grantee: suite.addrs[1].String(),
+				})
+				suite.Require().NoError(err)
+			},
 		},
 		"use a little": {
 			granter: suite.addrs[0],
 			grantee: suite.addrs[1],
 			fee:     smallAtom,
 			allowed: true,
 			final:   futureAfterSmall,
+			postRun: func() {
+				_, err := suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{
+					Granter: suite.addrs[0].String(),
+					Grantee: suite.addrs[1].String(),
+				})
+				suite.Require().NoError(err)
+			},
 		},
 	}
 
 	for name, tc := range cases {
 		tc := tc
 		suite.Run(name, func() {
-			err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[0], suite.addrs[1], future)
+			err := suite.feegrantKeeper.GrantAllowance(suite.ctx, tc.granter, tc.grantee, future)
 			suite.Require().NoError(err)
 
 			err = suite.feegrantKeeper.UseGrantedFees(suite.ctx, tc.granter, tc.grantee, tc.fee, []sdk.Msg{})
@@ -227,6 +258,8 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 
 			loaded, _ := suite.feegrantKeeper.GetAllowance(suite.ctx, tc.granter, tc.grantee)
 			suite.Equal(tc.final, loaded)
+
+			tc.postRun()
 		})
 	}
 
@@ -281,7 +314,6 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
 	now := suite.ctx.BlockTime()
 	oneYearExpiry := now.AddDate(1, 0, 0)
-	oneDay := now.AddDate(0, 0, 1)
 
 	testCases := []struct {
 		name      string
@@ -345,98 +377,6 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 				Expiration: &oneYearExpiry,
 			},
 		},
-		{
-			name:    "grant created with a day expiry & overwritten with no expiry shouldn't be pruned: no error",
-			ctx:     suite.ctx.WithBlockTime(now.AddDate(0, 0, 2)),
-			granter: suite.addrs[2],
-			grantee: suite.addrs[1],
-			allowance: &feegrant.BasicAllowance{
-				SpendLimit: eth,
-			},
-			preRun: func() {
-				// create a grant with a day expiry.
-				allowance := &feegrant.BasicAllowance{
-					SpendLimit: suite.atom,
-					Expiration: &oneDay,
-				}
-				err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[2], suite.addrs[1], allowance)
-				suite.NoError(err)
-			},
-			postRun: func() {
-				_, err := suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{
-					Granter: suite.addrs[2].String(),
-					Grantee: suite.addrs[1].String(),
-				})
-				suite.NoError(err)
-			},
-		},
-		{
-			name:    "grant created with a day expiry & overwritten with a year expiry shouldn't be pruned: no error",
-			ctx:     suite.ctx.WithBlockTime(now.AddDate(0, 0, 2)),
-			granter: suite.addrs[2],
-			grantee: suite.addrs[1],
-			allowance: &feegrant.BasicAllowance{
-				SpendLimit: eth,
-				Expiration: &oneYearExpiry,
-			},
-			preRun: func() {
-				// create a grant with a day expiry.
-				allowance := &feegrant.BasicAllowance{
-					SpendLimit: suite.atom,
-					Expiration: &oneDay,
-				}
-				err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[2], suite.addrs[1], allowance)
-				suite.NoError(err)
-			},
-			postRun: func() {
-				_, err := suite.msgSrvr.RevokeAllowance(suite.ctx, &feegrant.MsgRevokeAllowance{
-					Granter: suite.addrs[2].String(),
-					Grantee: suite.addrs[1].String(),
-				})
-				suite.NoError(err)
-			},
-		},
-		{
-			name:    "grant created with a year expiry & overwritten with a day expiry should be pruned after a day: error",
-			ctx:     suite.ctx.WithBlockTime(now.AddDate(0, 0, 2)),
-			granter: suite.addrs[2],
-			grantee: suite.addrs[1],
-			allowance: &feegrant.BasicAllowance{
-				SpendLimit: eth,
-				Expiration: &oneDay,
-			},
-			preRun: func() {
-				// create a grant with a year expiry.
-				allowance := &feegrant.BasicAllowance{
-					SpendLimit: suite.atom,
-					Expiration: &oneYearExpiry,
-				}
-				err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[2], suite.addrs[1], allowance)
-				suite.NoError(err)
-			},
-			postRun:   func() {},
-			expErrMsg: "not found",
-		},
-		{
-			name:    "grant created with no expiry & overwritten with a day expiry should be pruned after a day: error",
-			ctx:     suite.ctx.WithBlockTime(now.AddDate(0, 0, 2)),
-			granter: suite.addrs[2],
-			grantee: suite.addrs[1],
-			allowance: &feegrant.BasicAllowance{
-				SpendLimit: eth,
-				Expiration: &oneDay,
-			},
-			preRun: func() {
-				// create a grant with no expiry.
-				allowance := &feegrant.BasicAllowance{
-					SpendLimit: suite.atom,
-				}
-				err := suite.feegrantKeeper.GrantAllowance(suite.ctx, suite.addrs[2], suite.addrs[1], allowance)
-				suite.NoError(err)
-			},
-			postRun:   func() {},
-			expErrMsg: "not found",
-		},
 	}
 
 	for _, tc := range testCases {

x/feegrant/keeper/msg_server.go

@@ -5,7 +5,6 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
-	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/feegrant"
 )
 
@@ -37,11 +36,6 @@ func (k msgServer) GrantAllowance(goCtx context.Context, msg *feegrant.MsgGrantA
 		return nil, err
 	}
 
-	// Checking for duplicate entry
-	if f, _ := k.Keeper.GetAllowance(ctx, granter, grantee); f != nil {
-		return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "fee allowance already exists")
-	}
-
 	allowance, err := msg.GetFeeAllowanceI()
 	if err != nil {
 		return nil, err