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 <[email protected]> * review changes Co-authored-by: Marie Gauthier <[email protected]>
cosmos · Apr 28, 2021 · a2911d0 · a2911d0
1 parent 0cbed20
commit a2911d0
Show file tree

Hide file tree

Showing 9 changed files with 665 additions and 135 deletions.
diff --git a/x/feegrant/client/testutil/suite.go b/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,
 		},
 		{

diff --git a/x/feegrant/genesis_test.go b/x/feegrant/genesis_test.go
@@ -39,17 +39,23 @@ 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)
 	suite.Require().NoError(err)
 
 	genesis, err := feegrant.ExportGenesis(suite.ctx, suite.keeper)
 	suite.Require().NoError(err)
-	// Clear keeper
-	suite.keeper.RevokeFeeAllowance(suite.ctx, granterAddr, granteeAddr)
+	// revoke fee allowance
+	_, 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)

diff --git a/x/feegrant/keeper/grpc_query_test.go b/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),
 	})

diff --git a/x/feegrant/keeper/keeper.go b/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
-func (k Keeper) RevokeFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress) error {
-	store := ctx.KVStore(k.storeKey)
-	key := types.FeeAllowanceKey(granter, grantee)
-	_, found := k.GetFeeGrant(ctx, granter, grantee)
-	if !found {
-		return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "fee-grant not found")
+// revokeFeeAllowance removes an existing grant
+func (k Keeper) revokeFeeAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress) error {
+	_, err := k.getFeeGrant(ctx, granter, grantee)
+	if err != nil {
+		return err
 	}
 
+	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)
-	if !found {
-		return nil, sdkerrors.Wrapf(types.ErrNoAllowance, "grant missing")
+	grant, err := k.getFeeGrant(ctx, granter, grantee)
+	if err != nil {
+		return nil, err
 	}
 
 	return grant.GetFeeGrant()
 }
 
-// GetFeeGrant returns entire grant between both accounts
-func (k Keeper) GetFeeGrant(ctx sdk.Context, granter sdk.AccAddress, grantee sdk.AccAddress) (types.FeeAllowanceGrant, bool) {
+// getFeeGrant returns entire grant between both accounts
+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)
-
-	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)
+	if err := k.cdc.UnmarshalBinaryBare(bz, &feegrant); err != nil {
+		return nil, err
 	}
 
-	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)
-	if !found {
-		return sdkerrors.Wrapf(types.ErrNoAllowance, "grant missing")
+	f, err := k.getFeeGrant(ctx, granter, grantee)
+	if err != nil {
+		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)
-		// note this returns nil if err == nil
-		return sdkerrors.Wrap(err, "removed grant")
+		// Ignoring the `revokeFeeAllowance` error, because the user has enough grants to perform this transaction.
+		k.revokeFeeAllowance(ctx, granter, grantee)
+		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),
+		),
+	)
+}