x/feegrant state machine audit updates (#9176)

* replace panic with error

* improve test coverage

* add time based tests to basic-fee

* add periodic fee tests

* wip

* add msg_server tests

* improve getFeeGrant

* fix failing test

* fix test

* fix comments

* refactor

* review changes

* review changes

* fix errors

* Update x/feegrant/types/basic_fee_test.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

* review changes

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

x/feegrant/client/testutil/suite.go

@@ -142,7 +142,7 @@ func (s *IntegrationTestSuite) TestCmdGetFeeGrant() {
 				grantee.String(),
 				fmt.Sprintf("--%s=json", tmcli.OutputFlag),
 			},
-			"no allowance",
+			"fee-grant not found",
 			true, nil, nil,
 		},
 		{

x/feegrant/genesis_test.go

@@ -39,6 +39,7 @@ var (
 func (suite *GenesisTestSuite) TestImportExportGenesis() {
 	coins := sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(1_000)))
 	now := suite.ctx.BlockHeader().Time
+	msgSrvr := keeper.NewMsgServerImpl(suite.keeper)
 
 	allowance := &types.BasicFeeAllowance{SpendLimit: coins, Expiration: types.ExpiresAtTime(now.AddDate(1, 0, 0))}
 	err := suite.keeper.GrantFeeAllowance(suite.ctx, granterAddr, granteeAddr, allowance)
@@ -46,10 +47,15 @@ func (suite *GenesisTestSuite) TestImportExportGenesis() {
 
 	genesis, err := feegrant.ExportGenesis(suite.ctx, suite.keeper)
 	suite.Require().NoError(err)
-	// Clear keeper
+	// revoke fee allowance
-	suite.keeper.RevokeFeeAllowance(suite.ctx, granterAddr, granteeAddr)
+	_, err = msgSrvr.RevokeFeeAllowance(sdk.WrapSDKContext(suite.ctx), &types.MsgRevokeFeeAllowance{
+		Granter: granterAddr.String(),
+		Grantee: granteeAddr.String(),
+	})
+	suite.Require().NoError(err)
 	err = feegrant.InitGenesis(suite.ctx, suite.keeper, genesis)
 	suite.Require().NoError(err)
+
 	newGenesis, err := feegrant.ExportGenesis(suite.ctx, suite.keeper)
 	suite.Require().NoError(err)
 	suite.Require().Equal(genesis, newGenesis)

x/feegrant/keeper/grpc_query_test.go

@@ -6,8 +6,6 @@ import (
 )
 
 func (suite *KeeperTestSuite) TestFeeAllowance() {
-	ctx := suite.ctx
-	k := suite.app.FeeGrantKeeper
 
 	testCases := []struct {
 		name      string
@@ -73,7 +71,7 @@ func (suite *KeeperTestSuite) TestFeeAllowance() {
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
 			tc.preRun()
-			resp, err := k.FeeAllowance(sdk.WrapSDKContext(ctx), tc.req)
+			resp, err := suite.keeper.FeeAllowance(suite.ctx, tc.req)
 			if tc.expectErr {
 				suite.Require().Error(err)
 			} else {
@@ -85,9 +83,6 @@ func (suite *KeeperTestSuite) TestFeeAllowance() {
 }
 
 func (suite *KeeperTestSuite) TestFeeAllowances() {
-	ctx := suite.ctx
-	k := suite.app.FeeGrantKeeper
-
 	testCases := []struct {
 		name      string
 		req       *types.QueryFeeAllowancesRequest
@@ -142,7 +137,7 @@ func (suite *KeeperTestSuite) TestFeeAllowances() {
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
 			tc.preRun()
-			resp, err := k.FeeAllowances(sdk.WrapSDKContext(ctx), tc.req)
+			resp, err := suite.keeper.FeeAllowances(suite.ctx, tc.req)
 			if tc.expectErr {
 				suite.Require().Error(err)
 			} else {
@@ -154,7 +149,7 @@ func (suite *KeeperTestSuite) TestFeeAllowances() {
 }
 
 func grantFeeAllowance(suite *KeeperTestSuite) {
-	err := suite.app.FeeGrantKeeper.GrantFeeAllowance(suite.ctx, suite.addrs[0], suite.addrs[1], &types.BasicFeeAllowance{
+	err := suite.app.FeeGrantKeeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], &types.BasicFeeAllowance{
 		SpendLimit: sdk.NewCoins(sdk.NewInt64Coin("atom", 555)),
 		Expiration: types.ExpiresAtHeight(334455),
 	})

x/feegrant/keeper/keeper.go

@@ -71,15 +71,15 @@ func (k Keeper) GrantFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddre
 	return nil
 }
 
-// RevokeFeeAllowance removes an existing grant
+// revokeFeeAllowance removes an existing grant
-func (k Keeper) RevokeFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress) error {
+func (k Keeper) revokeFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress) error {
-	store := ctx.KVStore(k.storeKey)
+	_, err := k.getFeeGrant(ctx, granter, grantee)
-	key := types.FeeAllowanceKey(granter, grantee)
+	if err != nil {
-	_, found := k.GetFeeGrant(ctx, granter, grantee)
+		return err
-	if !found {
-		return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "fee-grant not found")
 	}
 
+	store := ctx.KVStore(k.storeKey)
+	key := types.FeeAllowanceKey(granter, grantee)
 	store.Delete(key)
 
 	ctx.EventManager().EmitEvent(
@@ -96,48 +96,29 @@ func (k Keeper) RevokeFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddr
 // If there is none, it returns nil, nil.
 // Returns an error on parsing issues
 func (k Keeper) GetFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress) (types.FeeAllowanceI, error) {
-	grant, found := k.GetFeeGrant(ctx, granter, grantee)
+	grant, err := k.getFeeGrant(ctx, granter, grantee)
-	if !found {
+	if err != nil {
-		return nil, sdkerrors.Wrapf(types.ErrNoAllowance, "grant missing")
+		return nil, err
 	}
 
 	return grant.GetFeeGrant()
 }
 
-// GetFeeGrant returns entire grant between both accounts
+// getFeeGrant returns entire grant between both accounts
-func (k Keeper) GetFeeGrant(ctx sdk.Context, granter sdk.AccAddress, grantee sdk.AccAddress) (types.FeeAllowanceGrant, bool) {
+func (k Keeper) getFeeGrant(ctx sdk.Context, granter sdk.AccAddress, grantee sdk.AccAddress) (*types.FeeAllowanceGrant, error) {
 	store := ctx.KVStore(k.storeKey)
 	key := types.FeeAllowanceKey(granter, grantee)
 	bz := store.Get(key)
 	if len(bz) == 0 {
-		return types.FeeAllowanceGrant{}, false
+		return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "fee-grant not found")
 	}
 
 	var feegrant types.FeeAllowanceGrant
-	k.cdc.MustUnmarshalBinaryBare(bz, &feegrant)
+	if err := k.cdc.UnmarshalBinaryBare(bz, &feegrant); err != nil {
-
+		return nil, err
-	return feegrant, true
-}
-
-// IterateAllGranteeFeeAllowances iterates over all the grants from anyone to the given grantee.
-// Callback to get all data, returns true to stop, false to keep reading
-func (k Keeper) IterateAllGranteeFeeAllowances(ctx sdk.Context, grantee sdk.AccAddress, cb func(types.FeeAllowanceGrant) bool) error {
-	store := ctx.KVStore(k.storeKey)
-	prefix := types.FeeAllowancePrefixByGrantee(grantee)
-	iter := sdk.KVStorePrefixIterator(store, prefix)
-	defer iter.Close()
-
-	stop := false
-	for ; iter.Valid() && !stop; iter.Next() {
-		bz := iter.Value()
-
-		var feeGrant types.FeeAllowanceGrant
-		k.cdc.MustUnmarshalBinaryBare(bz, &feeGrant)
-
-		stop = cb(feeGrant)
 	}
 
-	return nil
+	return &feegrant, nil
 }
 
 // IterateAllFeeAllowances iterates over all the grants in the store.
@@ -152,7 +133,9 @@ func (k Keeper) IterateAllFeeAllowances(ctx sdk.Context, cb func(types.FeeAllowa
 	for ; iter.Valid() && !stop; iter.Next() {
 		bz := iter.Value()
 		var feeGrant types.FeeAllowanceGrant
-		k.cdc.MustUnmarshalBinaryBare(bz, &feeGrant)
+		if err := k.cdc.UnmarshalBinaryBare(bz, &feeGrant); err != nil {
+			return err
+		}
 
 		stop = cb(feeGrant)
 	}
@@ -162,9 +145,9 @@ func (k Keeper) IterateAllFeeAllowances(ctx sdk.Context, cb func(types.FeeAllowa
 
 // UseGrantedFees will try to pay the given fee from the granter's account as requested by the grantee
 func (k Keeper) UseGrantedFees(ctx sdk.Context, granter, grantee sdk.AccAddress, fee sdk.Coins, msgs []sdk.Msg) error {
-	f, found := k.GetFeeGrant(ctx, granter, grantee)
+	f, err := k.getFeeGrant(ctx, granter, grantee)
-	if !found {
+	if err != nil {
-		return sdkerrors.Wrapf(types.ErrNoAllowance, "grant missing")
+		return err
 	}
 
 	grant, err := f.GetFeeGrant()
@@ -173,26 +156,35 @@ func (k Keeper) UseGrantedFees(ctx sdk.Context, granter, grantee sdk.AccAddress,
 	}
 
 	remove, err := grant.Accept(ctx, fee, msgs)
-	if err == nil {
-		ctx.EventManager().EmitEvent(
-			sdk.NewEvent(
-				types.EventTypeUseFeeGrant,
-				sdk.NewAttribute(types.AttributeKeyGranter, granter.String()),
-				sdk.NewAttribute(types.AttributeKeyGrantee, grantee.String()),
-			),
-		)
-	}
 
 	if remove {
-		k.RevokeFeeAllowance(ctx, granter, grantee)
+		// Ignoring the `revokeFeeAllowance` error, because the user has enough grants to perform this transaction.
-		// note this returns nil if err == nil
+		k.revokeFeeAllowance(ctx, granter, grantee)
-		return sdkerrors.Wrap(err, "removed grant")
+		if err != nil {
+			return err
+		}
+
+		emitUseGrantEvent(ctx, granter.String(), grantee.String())
+
+		return nil
 	}
 
 	if err != nil {
-		return sdkerrors.Wrap(err, "invalid grant")
+		return err
 	}
 
-	// if we accepted, store the updated state of the allowance
+	emitUseGrantEvent(ctx, granter.String(), grantee.String())
+
+	// if fee allowance is accepted, store the updated state of the allowance
 	return k.GrantFeeAllowance(ctx, granter, grantee, grant)
 }
+
+func emitUseGrantEvent(ctx sdk.Context, granter, grantee string) {
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			types.EventTypeUseFeeGrant,
+			sdk.NewAttribute(types.AttributeKeyGranter, granter),
+			sdk.NewAttribute(types.AttributeKeyGrantee, grantee),
+		),
+	)
+}

x/feegrant/keeper/keeper_test.go

@@ -1,13 +1,16 @@
 package keeper_test
 
 import (
+	"context"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/suite"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 
 	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/feegrant/keeper"
 	"github.com/cosmos/cosmos-sdk/x/feegrant/types"
 )
 
@@ -15,8 +18,12 @@ type KeeperTestSuite struct {
 	suite.Suite
 
 	app     *simapp.SimApp
-	ctx   sdk.Context
+	sdkCtx  sdk.Context
 	addrs   []sdk.AccAddress
+	msgSrvr types.MsgServer
+	ctx     context.Context
+	atom    sdk.Coins
+	keeper  keeper.Keeper
 }
 
 func TestKeeperTestSuite(t *testing.T) {
@@ -28,19 +35,19 @@ func (suite *KeeperTestSuite) SetupTest() {
 	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
 
 	suite.app = app
-	suite.ctx = ctx
+	suite.sdkCtx = ctx
 	suite.addrs = simapp.AddTestAddrsIncremental(app, ctx, 4, sdk.NewInt(30000000))
+	suite.ctx = sdk.WrapSDKContext(ctx)
+	suite.keeper = suite.app.FeeGrantKeeper
+	suite.msgSrvr = keeper.NewMsgServerImpl(suite.keeper)
+	suite.atom = sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(555)))
 }
 
 func (suite *KeeperTestSuite) TestKeeperCrud() {
-	ctx := suite.ctx
-	k := suite.app.FeeGrantKeeper
-
 	// some helpers
-	atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555))
 	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
 	basic := &types.BasicFeeAllowance{
-		SpendLimit: atom,
+		SpendLimit: suite.atom,
 		Expiration: types.ExpiresAtHeight(334455),
 	}
 
@@ -50,29 +57,35 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 	}
 
 	// let's set up some initial state here
-	err := k.GrantFeeAllowance(ctx, suite.addrs[0], suite.addrs[1], basic)
+	err := suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], basic)
 	suite.Require().NoError(err)
 
-	err = k.GrantFeeAllowance(ctx, suite.addrs[0], suite.addrs[2], basic2)
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[2], basic2)
 	suite.Require().NoError(err)
 
-	err = k.GrantFeeAllowance(ctx, suite.addrs[1], suite.addrs[2], basic)
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[1], suite.addrs[2], basic)
 	suite.Require().NoError(err)
 
-	err = k.GrantFeeAllowance(ctx, suite.addrs[1], suite.addrs[3], basic)
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[1], suite.addrs[3], basic)
 	suite.Require().NoError(err)
 
-	err = k.GrantFeeAllowance(ctx, suite.addrs[3], suite.addrs[0], basic2)
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[3], suite.addrs[0], basic2)
 	suite.Require().NoError(err)
 
 	// remove some, overwrite other
-	k.RevokeFeeAllowance(ctx, suite.addrs[0], suite.addrs[1])
+	_, err = suite.msgSrvr.RevokeFeeAllowance(suite.ctx, &types.MsgRevokeFeeAllowance{Granter: suite.addrs[0].String(), Grantee: suite.addrs[1].String()})
-	k.RevokeFeeAllowance(ctx, suite.addrs[0], suite.addrs[2])
+	suite.Require().NoError(err)
-
+	_, err = suite.msgSrvr.RevokeFeeAllowance(suite.ctx, &types.MsgRevokeFeeAllowance{Granter: suite.addrs[0].String(), Grantee: suite.addrs[2].String()})
-	err = k.GrantFeeAllowance(ctx, suite.addrs[0], suite.addrs[2], basic)
 	suite.Require().NoError(err)
 
-	err = k.GrantFeeAllowance(ctx, suite.addrs[1], suite.addrs[2], basic2)
+	// revoke non-exist fee allowance
+	_, err = suite.msgSrvr.RevokeFeeAllowance(suite.ctx, &types.MsgRevokeFeeAllowance{Granter: suite.addrs[0].String(), Grantee: suite.addrs[2].String()})
+	suite.Require().Error(err)
+
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[2], basic)
+	suite.Require().NoError(err)
+
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[1], suite.addrs[2], basic2)
 	suite.Require().NoError(err)
 
 	// end state:
@@ -109,7 +122,7 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 	for name, tc := range cases {
 		tc := tc
 		suite.Run(name, func() {
-			allow, _ := k.GetFeeAllowance(ctx, tc.granter, tc.grantee)
+			allow, _ := suite.keeper.GetFeeAllowance(suite.sdkCtx, tc.granter, tc.grantee)
 
 			if tc.allowance == nil {
 				suite.Nil(allow)
@@ -119,61 +132,25 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 			suite.Equal(tc.allowance, allow)
 		})
 	}
+	accAddr, err := sdk.AccAddressFromBech32("cosmos1rxr4mq58w3gtnx5tsc438mwjjafv3mja7k5pnu")
+	suite.Require().NoError(err)
 
-	grant1, err := types.NewFeeAllowanceGrant(suite.addrs[3], suite.addrs[0], basic2)
+	// let's grant and revoke authorization to non existing account
-	suite.NoError(err)
+	err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[3], accAddr, basic2)
+	suite.Require().NoError(err)
 
-	grant2, err := types.NewFeeAllowanceGrant(suite.addrs[1], suite.addrs[2], basic2)
+	_, err = suite.keeper.GetFeeAllowance(suite.sdkCtx, suite.addrs[3], accAddr)
-	suite.NoError(err)
+	suite.Require().NoError(err)
 
-	grant3, err := types.NewFeeAllowanceGrant(suite.addrs[0], suite.addrs[2], basic)
+	_, err = suite.msgSrvr.RevokeFeeAllowance(suite.ctx, &types.MsgRevokeFeeAllowance{Granter: suite.addrs[3].String(), Grantee: accAddr.String()})
-	suite.NoError(err)
+	suite.Require().NoError(err)
 
-	allCases := map[string]struct {
-		grantee sdk.AccAddress
-		grants  []types.FeeAllowanceGrant
-	}{
-		"addr2 has none": {
-			grantee: suite.addrs[1],
-		},
-		"addr has one": {
-			grantee: suite.addrs[0],
-			grants: []types.FeeAllowanceGrant{
-				grant1,
-			},
-		},
-		"addr3 has two": {
-			grantee: suite.addrs[2],
-			grants: []types.FeeAllowanceGrant{
-				grant3,
-				grant2,
-			},
-		},
-	}
-
-	for name, tc := range allCases {
-		tc := tc
-		suite.Run(name, func() {
-			var grants []types.FeeAllowanceGrant
-			err := k.IterateAllGranteeFeeAllowances(ctx, tc.grantee, func(grant types.FeeAllowanceGrant) bool {
-				grants = append(grants, grant)
-				return false
-			})
-			suite.NoError(err)
-			suite.Equal(tc.grants, grants)
-		})
-	}
 }
 
 func (suite *KeeperTestSuite) TestUseGrantedFee() {
-	ctx := suite.ctx
-	k := suite.app.FeeGrantKeeper
-
-	// some helpers
-	atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555))
 	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
 	future := &types.BasicFeeAllowance{
-		SpendLimit: atom,
+		SpendLimit: suite.atom,
 		Expiration: types.ExpiresAtHeight(5678),
 	}
 
@@ -203,7 +180,7 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 		"use entire pot": {
 			granter: suite.addrs[0],
 			grantee: suite.addrs[1],
-			fee:     atom,
+			fee:     suite.atom,
 			allowed: true,
 			final:   nil,
 		},
@@ -237,22 +214,45 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 			// addr -> addr2 (future)
 			// addr -> addr3 (expired)
 
-			err := k.GrantFeeAllowance(ctx, suite.addrs[0], suite.addrs[1], future)
+			err := suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], future)
 			suite.Require().NoError(err)
 
-			err = k.GrantFeeAllowance(ctx, suite.addrs[0], suite.addrs[3], expired)
+			err = suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[3], expired)
 			suite.Require().NoError(err)
 
-			err = k.UseGrantedFees(ctx, tc.granter, tc.grantee, tc.fee, []sdk.Msg{})
+			err = suite.keeper.UseGrantedFees(suite.sdkCtx, tc.granter, tc.grantee, tc.fee, []sdk.Msg{})
 			if tc.allowed {
 				suite.NoError(err)
 			} else {
 				suite.Error(err)
 			}
 
-			loaded, _ := k.GetFeeAllowance(ctx, tc.granter, tc.grantee)
+			loaded, _ := suite.keeper.GetFeeAllowance(suite.sdkCtx, tc.granter, tc.grantee)
 
 			suite.Equal(tc.final, loaded)
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestIterateGrants() {
+	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
+	allowance := &types.BasicFeeAllowance{
+		SpendLimit: suite.atom,
+		Expiration: types.ExpiresAtHeight(5678),
+	}
+
+	allowance1 := &types.BasicFeeAllowance{
+		SpendLimit: eth,
+		Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().Add(24 * time.Hour)),
+	}
+
+	suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], allowance)
+	suite.keeper.GrantFeeAllowance(suite.sdkCtx, suite.addrs[2], suite.addrs[1], allowance1)
+
+	suite.keeper.IterateAllFeeAllowances(suite.sdkCtx, func(grant types.FeeAllowanceGrant) bool {
+		suite.Require().Equal(suite.addrs[1].String(), grant.Grantee)
+		suite.Require().Contains([]string{suite.addrs[0].String(), suite.addrs[2].String()}, grant.Granter)
+		return true
+	})
+
+}

x/feegrant/keeper/msg_server.go

@@ -67,7 +67,7 @@ func (k msgServer) RevokeFeeAllowance(goCtx context.Context, msg *types.MsgRevok
 		return nil, err
 	}
 
-	err = k.Keeper.RevokeFeeAllowance(ctx, granter, grantee)
+	err = k.Keeper.revokeFeeAllowance(ctx, granter, grantee)
 	if err != nil {
 		return nil, err
 	}

x/feegrant/keeper/msg_server_test.go

@@ -0,0 +1,220 @@
+package keeper_test
+
+import (
+	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
+
+	"github.com/cosmos/cosmos-sdk/x/feegrant/types"
+)
+
+func (suite *KeeperTestSuite) TestGrantFeeAllowance() {
+	testCases := []struct {
+		name      string
+		req       func() *types.MsgGrantFeeAllowance
+		expectErr bool
+		errMsg    string
+	}{
+		{
+			"invalid granter address",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.BasicFeeAllowance{})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   "invalid-granter",
+					Grantee:   suite.addrs[1].String(),
+					Allowance: any,
+				}
+			},
+			true,
+			"decoding bech32 failed",
+		},
+		{
+			"invalid grantee address",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.BasicFeeAllowance{})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[0].String(),
+					Grantee:   "invalid-grantee",
+					Allowance: any,
+				}
+			},
+			true,
+			"decoding bech32 failed",
+		},
+		{
+			"valid: basic fee allowance",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.BasicFeeAllowance{
+					SpendLimit: suite.atom,
+					Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().AddDate(1, 0, 0)),
+				})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[0].String(),
+					Grantee:   suite.addrs[1].String(),
+					Allowance: any,
+				}
+			},
+			false,
+			"",
+		},
+		{
+			"fail: fee allowance exists",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.BasicFeeAllowance{
+					SpendLimit: suite.atom,
+					Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().AddDate(1, 0, 0)),
+				})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[0].String(),
+					Grantee:   suite.addrs[1].String(),
+					Allowance: any,
+				}
+			},
+			true,
+			"fee allowance already exists",
+		},
+		{
+			"valid: periodic fee allowance",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.PeriodicFeeAllowance{
+					Basic: types.BasicFeeAllowance{
+						SpendLimit: suite.atom,
+						Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().AddDate(1, 0, 0)),
+					},
+				})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[1].String(),
+					Grantee:   suite.addrs[2].String(),
+					Allowance: any,
+				}
+			},
+			false,
+			"",
+		},
+		{
+			"error: fee allowance exists",
+			func() *types.MsgGrantFeeAllowance {
+				any, err := codectypes.NewAnyWithValue(&types.PeriodicFeeAllowance{
+					Basic: types.BasicFeeAllowance{
+						SpendLimit: suite.atom,
+						Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().AddDate(1, 0, 0)),
+					},
+				})
+				suite.Require().NoError(err)
+				return &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[1].String(),
+					Grantee:   suite.addrs[2].String(),
+					Allowance: any,
+				}
+			},
+			true,
+			"fee allowance already exists",
+		},
+	}
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			_, err := suite.msgSrvr.GrantFeeAllowance(suite.ctx, tc.req())
+			if tc.expectErr {
+				suite.Require().Error(err)
+				suite.Require().Contains(err.Error(), tc.errMsg)
+			}
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestRevokeFeeAllowance() {
+
+	testCases := []struct {
+		name      string
+		request   *types.MsgRevokeFeeAllowance
+		preRun    func()
+		expectErr bool
+		errMsg    string
+	}{
+		{
+			"error: invalid granter",
+			&types.MsgRevokeFeeAllowance{
+				Granter: "invalid-granter",
+				Grantee: suite.addrs[1].String(),
+			},
+			func() {},
+			true,
+			"decoding bech32 failed",
+		},
+		{
+			"error: invalid grantee",
+			&types.MsgRevokeFeeAllowance{
+				Granter: suite.addrs[0].String(),
+				Grantee: "invalid-grantee",
+			},
+			func() {},
+			true,
+			"decoding bech32 failed",
+		},
+		{
+			"error: fee allowance not found",
+			&types.MsgRevokeFeeAllowance{
+				Granter: suite.addrs[0].String(),
+				Grantee: suite.addrs[1].String(),
+			},
+			func() {},
+			true,
+			"fee-grant not found",
+		},
+		{
+			"success: revoke fee allowance",
+			&types.MsgRevokeFeeAllowance{
+				Granter: suite.addrs[0].String(),
+				Grantee: suite.addrs[1].String(),
+			},
+			func() {
+				// removing fee allowance from previous tests if exists
+				suite.msgSrvr.RevokeFeeAllowance(suite.ctx, &types.MsgRevokeFeeAllowance{
+					Granter: suite.addrs[0].String(),
+					Grantee: suite.addrs[1].String(),
+				})
+				any, err := codectypes.NewAnyWithValue(&types.PeriodicFeeAllowance{
+					Basic: types.BasicFeeAllowance{
+						SpendLimit: suite.atom,
+						Expiration: types.ExpiresAtTime(suite.sdkCtx.BlockTime().AddDate(1, 0, 0)),
+					},
+				})
+				suite.Require().NoError(err)
+				req := &types.MsgGrantFeeAllowance{
+					Granter:   suite.addrs[0].String(),
+					Grantee:   suite.addrs[1].String(),
+					Allowance: any,
+				}
+				_, err = suite.msgSrvr.GrantFeeAllowance(suite.ctx, req)
+				suite.Require().NoError(err)
+			},
+			false,
+			"",
+		},
+		{
+			"error: check fee allowance revoked",
+			&types.MsgRevokeFeeAllowance{
+				Granter: suite.addrs[0].String(),
+				Grantee: suite.addrs[1].String(),
+			},
+			func() {},
+			true,
+			"fee-grant not found",
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			tc.preRun()
+			_, err := suite.msgSrvr.RevokeFeeAllowance(suite.ctx, tc.request)
+			if tc.expectErr {
+				suite.Require().Error(err)
+				suite.Require().Contains(err.Error(), tc.errMsg)
+			}
+		})
+	}
+
+}

x/feegrant/types/basic_fee_test.go

@@ -2,6 +2,7 @@ package types_test
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -129,3 +130,138 @@ func TestBasicFeeValidAllow(t *testing.T) {
 		})
 	}
 }
+
+func TestBasicFeeAllowTime(t *testing.T) {
+	app := simapp.Setup(false)
+
+	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 10))
+	atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555))
+	smallAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 43))
+	bigAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 1000))
+	leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512))
+
+	now := time.Now()
+	oneHour := now.Add(1 * time.Hour)
+
+	cases := map[string]struct {
+		allow *types.BasicFeeAllowance
+		// all other checks are ignored if valid=false
+		fee       sdk.Coins
+		blockTime time.Time
+		valid     bool
+		accept    bool
+		remove    bool
+		remains   sdk.Coins
+	}{
+		"empty": {
+			allow:  &types.BasicFeeAllowance{},
+			valid:  true,
+			accept: true,
+		},
+		"small fee without expire": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: atom,
+			},
+			valid:   true,
+			fee:     smallAtom,
+			accept:  true,
+			remove:  false,
+			remains: leftAtom,
+		},
+		"all fee without expire": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: smallAtom,
+			},
+			valid:  true,
+			fee:    smallAtom,
+			accept: true,
+			remove: true,
+		},
+		"wrong fee": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: smallAtom,
+			},
+			valid:  true,
+			fee:    eth,
+			accept: false,
+		},
+		"non-expired": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: atom,
+				Expiration: types.ExpiresAtTime(oneHour),
+			},
+			valid:     true,
+			fee:       smallAtom,
+			blockTime: now,
+			accept:    true,
+			remove:    false,
+			remains:   leftAtom,
+		},
+		"expired": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: atom,
+				Expiration: types.ExpiresAtTime(now),
+			},
+			valid:     true,
+			fee:       smallAtom,
+			blockTime: oneHour,
+			accept:    false,
+			remove:    true,
+		},
+		"fee more than allowed": {
+			allow: &types.BasicFeeAllowance{
+				SpendLimit: atom,
+				Expiration: types.ExpiresAtTime(oneHour),
+			},
+			valid:     true,
+			fee:       bigAtom,
+			blockTime: now,
+			accept:    false,
+		},
+		"without spend limit": {
+			allow: &types.BasicFeeAllowance{
+				Expiration: types.ExpiresAtTime(oneHour),
+			},
+			valid:     true,
+			fee:       bigAtom,
+			blockTime: now,
+			accept:    true,
+		},
+		"expired no spend limit": {
+			allow: &types.BasicFeeAllowance{
+				Expiration: types.ExpiresAtTime(now),
+			},
+			valid:     true,
+			fee:       bigAtom,
+			blockTime: oneHour,
+			accept:    false,
+		},
+	}
+
+	for name, stc := range cases {
+		tc := stc // to make scopelint happy
+		t.Run(name, func(t *testing.T) {
+			err := tc.allow.ValidateBasic()
+			if !tc.valid {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			ctx := app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockTime(tc.blockTime)
+
+			// now try to deduct
+			remove, err := tc.allow.Accept(ctx, tc.fee, []sdk.Msg{})
+			if !tc.accept {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			require.Equal(t, tc.remove, remove)
+			if !remove {
+				assert.Equal(t, tc.allow.SpendLimit, tc.remains)
+			}
+		})
+	}
+}

x/feegrant/types/periodic_fee_test.go

@@ -2,6 +2,7 @@ package types_test
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -206,3 +207,183 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 		})
 	}
 }
+
+func TestPeriodicFeeValidAllowTime(t *testing.T) {
+	app := simapp.Setup(false)
+	atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555))
+	smallAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 43))
+	leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512))
+	oneAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 1))
+	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 1))
+
+	now := time.Now()
+	oneHour := now.Add(1 * time.Hour)
+
+	cases := map[string]struct {
+		allow types.PeriodicFeeAllowance
+		// all other checks are ignored if valid=false
+		fee           sdk.Coins
+		blockTime     time.Time
+		valid         bool
+		accept        bool
+		remove        bool
+		remains       sdk.Coins
+		remainsPeriod sdk.Coins
+		periodReset   types.ExpiresAt
+	}{
+		"empty": {
+			allow: types.PeriodicFeeAllowance{},
+			valid: false,
+		},
+		"only basic": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtTime(oneHour),
+				},
+			},
+			valid: false,
+		},
+		"empty basic": {
+			allow: types.PeriodicFeeAllowance{
+				Period:           types.ClockDuration(time.Duration(10) * time.Minute),
+				PeriodSpendLimit: smallAtom,
+				PeriodReset:      types.ExpiresAtTime(now.Add(30 * time.Minute)),
+			},
+			blockTime:   now,
+			valid:       true,
+			accept:      true,
+			remove:      false,
+			periodReset: types.ExpiresAtTime(now.Add(30 * time.Minute)),
+		},
+		"mismatched currencies": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtTime(oneHour),
+				},
+				Period:           types.ClockDuration(10 * time.Minute),
+				PeriodSpendLimit: eth,
+			},
+			valid: false,
+		},
+		"same period": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtTime(now.Add(2 * time.Hour)),
+				},
+				Period:           types.ClockDuration(10),
+				PeriodReset:      types.ExpiresAtTime(now.Add(1 * time.Hour)),
+				PeriodSpendLimit: leftAtom,
+				PeriodCanSpend:   smallAtom,
+			},
+			valid:         true,
+			fee:           smallAtom,
+			blockTime:     now,
+			accept:        true,
+			remove:        false,
+			remainsPeriod: nil,
+			remains:       leftAtom,
+			periodReset:   types.ExpiresAtTime(now.Add(1 * time.Hour)),
+		},
+		"step one period": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtTime(now.Add(2 * time.Hour)),
+				},
+				Period:           types.ClockDuration(10 * time.Minute),
+				PeriodReset:      types.ExpiresAtTime(now),
+				PeriodSpendLimit: leftAtom,
+			},
+			valid:         true,
+			fee:           leftAtom,
+			blockTime:     now.Add(1 * time.Hour),
+			accept:        true,
+			remove:        false,
+			remainsPeriod: nil,
+			remains:       smallAtom,
+			periodReset:   types.ExpiresAtTime(oneHour.Add(10 * time.Minute)), // one step from last reset, not now
+		},
+		"step limited by global allowance": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: smallAtom,
+					Expiration: types.ExpiresAtTime(now.Add(2 * time.Hour)),
+				},
+				Period:           types.ClockDuration(10 * time.Minute),
+				PeriodReset:      types.ExpiresAtTime(now),
+				PeriodSpendLimit: atom,
+			},
+			valid:         true,
+			fee:           oneAtom,
+			blockTime:     oneHour,
+			accept:        true,
+			remove:        false,
+			remainsPeriod: smallAtom.Sub(oneAtom),
+			remains:       smallAtom.Sub(oneAtom),
+			periodReset:   types.ExpiresAtTime(oneHour.Add(10 * time.Minute)), // one step from last reset, not now
+		},
+		"expired": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtTime(now),
+				},
+				Period:           types.ClockDuration(time.Hour),
+				PeriodSpendLimit: smallAtom,
+			},
+			valid:     true,
+			fee:       smallAtom,
+			blockTime: oneHour,
+			accept:    false,
+			remove:    true,
+		},
+		"over period limit": {
+			allow: types.PeriodicFeeAllowance{
+				Basic: types.BasicFeeAllowance{
+					SpendLimit: atom,
+					Expiration: types.ExpiresAtHeight(100),
+				},
+				Period:           types.ClockDuration(time.Hour),
+				PeriodReset:      types.ExpiresAtTime(now.Add(1 * time.Hour)),
+				PeriodSpendLimit: leftAtom,
+				PeriodCanSpend:   smallAtom,
+			},
+			valid:     true,
+			fee:       leftAtom,
+			blockTime: now,
+			accept:    false,
+			remove:    true,
+		},
+	}
+
+	for name, stc := range cases {
+		tc := stc // to make scopelint happy
+		t.Run(name, func(t *testing.T) {
+			err := tc.allow.ValidateBasic()
+			if !tc.valid {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			ctx := app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockTime(tc.blockTime)
+			// now try to deduct
+			remove, err := tc.allow.Accept(ctx, tc.fee, []sdk.Msg{})
+			if !tc.accept {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			require.Equal(t, tc.remove, remove)
+			if !remove {
+				assert.Equal(t, tc.remains, tc.allow.Basic.SpendLimit)
+				assert.Equal(t, tc.remainsPeriod, tc.allow.PeriodCanSpend)
+				assert.Equal(t, tc.periodReset.String(), tc.allow.PeriodReset.String())
+			}
+		})
+	}
+}