feat(x/feegrant): Add limits to grant pruning and enable message to a…

…id manually (backport #18047) (#18128) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Julien Robert <[email protected]>
cosmos · Oct 23, 2023 · 026631c · 026631c
1 parent aa72f10
commit 026631c
Show file tree

Hide file tree

Showing 19 changed files with 1,466 additions and 77 deletions.
diff --git a/api/cosmos/feegrant/v1beta1/tx.pulsar.go b/api/cosmos/feegrant/v1beta1/tx.pulsar.go
diff --git a/api/cosmos/feegrant/v1beta1/tx_grpc.pb.go b/api/cosmos/feegrant/v1beta1/tx_grpc.pb.go
diff --git a/proto/cosmos/feegrant/v1beta1/tx.proto b/proto/cosmos/feegrant/v1beta1/tx.proto
@@ -20,6 +20,11 @@ service Msg {
   // RevokeAllowance revokes any fee allowance of granter's account that
   // has been granted to the grantee.
   rpc RevokeAllowance(MsgRevokeAllowance) returns (MsgRevokeAllowanceResponse);
+
+  // PruneAllowances prunes expired fee allowances, currently up to 75 at a time.
+  //
+  // Since cosmos-sdk 0.50
+  rpc PruneAllowances(MsgPruneAllowances) returns (MsgPruneAllowancesResponse);
 }
 
 // MsgGrantAllowance adds permission for Grantee to spend up to Allowance
@@ -55,3 +60,18 @@ message MsgRevokeAllowance {
 
 // MsgRevokeAllowanceResponse defines the Msg/RevokeAllowanceResponse response type.
 message MsgRevokeAllowanceResponse {}
+
+// MsgPruneAllowances prunes expired fee allowances.
+//
+// Since cosmos-sdk 0.50
+message MsgPruneAllowances {
+  option (cosmos.msg.v1.signer) = "pruner";
+
+  // pruner is the address of the user pruning expired allowances.
+  string pruner = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+}
+
+// MsgPruneAllowancesResponse defines the Msg/PruneAllowancesResponse response type.
+//
+// Since cosmos-sdk 0.50
+message MsgPruneAllowancesResponse {}
diff --git a/simapp/go.mod b/simapp/go.mod
@@ -14,7 +14,7 @@ require (
 	cosmossdk.io/tools/confix v0.0.0-20230925151519-64e0e8980834
 	cosmossdk.io/x/circuit v0.0.0-20231006095526-33390754f9fe
 	cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834
-	cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8
+	cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca
 	cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe
 	cosmossdk.io/x/tx v0.11.0
 	cosmossdk.io/x/upgrade v0.0.0-20230925151519-64e0e8980834

diff --git a/simapp/go.sum b/simapp/go.sum
@@ -211,8 +211,8 @@ cosmossdk.io/x/circuit v0.0.0-20231006095526-33390754f9fe h1:xcQTAlbv1l8PBHXI5/x
 cosmossdk.io/x/circuit v0.0.0-20231006095526-33390754f9fe/go.mod h1:syo6njNlaE1KLXRFd1gZQr/7pMstANp2zMqVUC+u4h4=
 cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834 h1:h4ooSV3X5BxEfl3EUbOlXNFMnEc/mXTXF5mdl17CQLg=
 cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834/go.mod h1:yUgv71a56ZEJu7c8BXWCliDrQ7Ag+FCZ//rYKw9S93U=
-cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8 h1:e3JUrisu36fIf+EzppkSeryOocs7oUue4e1IepdhMMI=
-cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8/go.mod h1:nnIKdJKz1Os+sOlJHrjgMOh1WAlle0aV7Y+0x434OyI=
+cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca h1:YuagGZRMuE6lg913jEVGHTf6b4cWjt+gJdnhTmFQ1LA=
+cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca/go.mod h1:vmiW+2cIfGZULh6ReN03sawbhn8UUGrttNMxiIiduKI=
 cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe h1:xDWsbJp/9Tdwh2w8/KT7E0sxb52ASTn1M+UO4pq6kko=
 cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe/go.mod h1:QQROAXjZWeJzH6dNHUNCPhs7zHrYjknH8gYynf0IvXs=
 cosmossdk.io/x/tx v0.11.0 h1:Ak2LIC06bXqPbpMIEorkQbwVddRvRys1sL3Cjm+KPfs=

diff --git a/tests/go.mod b/tests/go.mod
@@ -12,7 +12,7 @@ require (
 	cosmossdk.io/simapp v0.0.0-20230620040119-e078f1a49e8b
 	cosmossdk.io/store v1.0.0-rc.0
 	cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834
-	cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8
+	cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca
 	cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe // indirect
 	cosmossdk.io/x/tx v0.11.0
 	cosmossdk.io/x/upgrade v0.0.0-20230925151519-64e0e8980834

diff --git a/tests/go.sum b/tests/go.sum
@@ -209,8 +209,8 @@ cosmossdk.io/x/circuit v0.0.0-20231006095526-33390754f9fe h1:xcQTAlbv1l8PBHXI5/x
 cosmossdk.io/x/circuit v0.0.0-20231006095526-33390754f9fe/go.mod h1:syo6njNlaE1KLXRFd1gZQr/7pMstANp2zMqVUC+u4h4=
 cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834 h1:h4ooSV3X5BxEfl3EUbOlXNFMnEc/mXTXF5mdl17CQLg=
 cosmossdk.io/x/evidence v0.0.0-20230925151519-64e0e8980834/go.mod h1:yUgv71a56ZEJu7c8BXWCliDrQ7Ag+FCZ//rYKw9S93U=
-cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8 h1:e3JUrisu36fIf+EzppkSeryOocs7oUue4e1IepdhMMI=
-cosmossdk.io/x/feegrant v0.0.0-20231009114728-5259373edec8/go.mod h1:nnIKdJKz1Os+sOlJHrjgMOh1WAlle0aV7Y+0x434OyI=
+cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca h1:YuagGZRMuE6lg913jEVGHTf6b4cWjt+gJdnhTmFQ1LA=
+cosmossdk.io/x/feegrant v0.0.0-20231020161330-8acf4f8853ca/go.mod h1:vmiW+2cIfGZULh6ReN03sawbhn8UUGrttNMxiIiduKI=
 cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe h1:xDWsbJp/9Tdwh2w8/KT7E0sxb52ASTn1M+UO4pq6kko=
 cosmossdk.io/x/nft v0.0.0-20231006095526-33390754f9fe/go.mod h1:QQROAXjZWeJzH6dNHUNCPhs7zHrYjknH8gYynf0IvXs=
 cosmossdk.io/x/tx v0.11.0 h1:Ak2LIC06bXqPbpMIEorkQbwVddRvRys1sL3Cjm+KPfs=

diff --git a/x/feegrant/CHANGELOG.md b/x/feegrant/CHANGELOG.md
@@ -27,6 +27,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Features
 
+* [#18047](https://github.com/cosmos/cosmos-sdk/pull/18047) Added a limit of 200 grants pruned per EndBlock and the method PruneAllowances that prunes 75 expired grants on every run.
 * [#14649](https://github.com/cosmos/cosmos-sdk/pull/14649) The `x/feegrant` module is extracted to have a separate go.mod file which allows it to be a standalone module.
 
 ### API Breaking Changes

diff --git a/x/feegrant/README.md b/x/feegrant/README.md
@@ -206,6 +206,14 @@ The feegrant module emits the following events:
 | message | granter       | {granterAddress} |
 | message | grantee       | {granteeAddress} |
 
+### Prune fee allowances
+
+| Type    | Attribute Key | Attribute Value  |
+| ------- | ------------- | ---------------- |
+| message | action        |  prune_feegrant  |
+| message | pruner        | {prunerAddress}  |
+
+
 ## Client
 
 ### CLI

diff --git a/x/feegrant/events.go b/x/feegrant/events.go
@@ -6,7 +6,9 @@ const (
 	EventTypeRevokeFeeGrant = "revoke_feegrant"
 	EventTypeSetFeeGrant    = "set_feegrant"
 	EventTypeUpdateFeeGrant = "update_feegrant"
+	EventTypePruneFeeGrant  = "prune_feegrant"
 
 	AttributeKeyGranter = "granter"
 	AttributeKeyGrantee = "grantee"
+	AttributeKeyPruner  = "pruner"
 )
diff --git a/x/feegrant/keeper/keeper.go b/x/feegrant/keeper/keeper.go
@@ -233,12 +233,7 @@ func (k Keeper) IterateAllFeeAllowances(ctx context.Context, cb func(grant feegr
 
 // UseGrantedFees will try to pay the given fee from the granter's account as requested by the grantee
 func (k Keeper) UseGrantedFees(ctx context.Context, granter, grantee sdk.AccAddress, fee sdk.Coins, msgs []sdk.Msg) error {
-	f, err := k.getGrant(ctx, granter, grantee)
-	if err != nil {
-		return err
-	}
-
-	grant, err := f.GetGrant()
+	grant, err := k.GetAllowance(ctx, granter, grantee)
 	if err != nil {
 		return err
 	}
@@ -322,10 +317,11 @@ func (k Keeper) addToFeeAllowanceQueue(ctx context.Context, grantKey []byte, exp
 }
 
 // RemoveExpiredAllowances iterates grantsByExpiryQueue and deletes the expired grants.
-func (k Keeper) RemoveExpiredAllowances(ctx context.Context) error {
+func (k Keeper) RemoveExpiredAllowances(ctx context.Context, limit int32) error {
 	exp := sdk.UnwrapSDKContext(ctx).BlockTime()
 	store := k.storeService.OpenKVStore(ctx)
 	iterator, err := store.Iterator(feegrant.FeeAllowanceQueueKeyPrefix, storetypes.InclusiveEndBytes(feegrant.AllowanceByExpTimeKey(&exp)))
+	var count int32
 	if err != nil {
 		return err
 	}
@@ -342,6 +338,12 @@ func (k Keeper) RemoveExpiredAllowances(ctx context.Context) error {
 		if err != nil {
 			return err
 		}
+
+		// limit the amount of iterations to avoid taking too much time
+		count++
+		if count == limit {
+			return nil
+		}
 	}
 	return nil
 }
diff --git a/x/feegrant/keeper/keeper_test.go b/x/feegrant/keeper/keeper_test.go
@@ -38,18 +38,17 @@ func TestKeeperTestSuite(t *testing.T) {
 }
 
 func (suite *KeeperTestSuite) SetupTest() {
-	suite.addrs = simtestutil.CreateIncrementalAccounts(4)
+	suite.addrs = simtestutil.CreateIncrementalAccounts(20)
 	key := storetypes.NewKVStoreKey(feegrant.StoreKey)
 	testCtx := testutil.DefaultContextWithDB(suite.T(), key, storetypes.NewTransientStoreKey("transient_test"))
 	encCfg := moduletestutil.MakeTestEncodingConfig(module.AppModuleBasic{})
 
 	// setup gomock and initialize some globally expected executions
 	ctrl := gomock.NewController(suite.T())
 	suite.accountKeeper = feegranttestutil.NewMockAccountKeeper(ctrl)
-	suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[0]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[0])).AnyTimes()
-	suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[1]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[1])).AnyTimes()
-	suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[2]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[2])).AnyTimes()
-	suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[3]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[3])).AnyTimes()
+	for i := 0; i < len(suite.addrs); i++ {
+		suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[i]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[i])).AnyTimes()
+	}
 
 	suite.accountKeeper.EXPECT().AddressCodec().Return(codecaddress.NewBech32Codec("cosmos")).AnyTimes()
 
@@ -395,7 +394,9 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 			}
 			err := suite.feegrantKeeper.GrantAllowance(suite.ctx, tc.granter, tc.grantee, tc.allowance)
 			suite.NoError(err)
-			suite.feegrantKeeper.RemoveExpiredAllowances(tc.ctx)
+			err = suite.feegrantKeeper.RemoveExpiredAllowances(tc.ctx, 5)
+			suite.NoError(err)
+
 			grant, err := suite.feegrantKeeper.GetAllowance(tc.ctx, tc.granter, tc.grantee)
 			if tc.expErrMsg != "" {
 				suite.Error(err)

diff --git a/x/feegrant/keeper/msg_server.go b/x/feegrant/keeper/msg_server.go
@@ -89,3 +89,22 @@ func (k msgServer) RevokeAllowance(goCtx context.Context, msg *feegrant.MsgRevok
 
 	return &feegrant.MsgRevokeAllowanceResponse{}, nil
 }
+
+// PruneAllowances removes expired allowances from the store.
+func (k msgServer) PruneAllowances(ctx context.Context, req *feegrant.MsgPruneAllowances) (*feegrant.MsgPruneAllowancesResponse, error) {
+	// 75 is an arbitrary value, we can change it later if needed
+	err := k.RemoveExpiredAllowances(ctx, 75)
+	if err != nil {
+		return nil, err
+	}
+
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	sdkCtx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			feegrant.EventTypePruneFeeGrant,
+			sdk.NewAttribute(feegrant.AttributeKeyPruner, req.Pruner),
+		),
+	)
+
+	return &feegrant.MsgPruneAllowancesResponse{}, nil
+}
diff --git a/x/feegrant/keeper/msg_server_test.go b/x/feegrant/keeper/msg_server_test.go
@@ -284,3 +284,66 @@ func (suite *KeeperTestSuite) TestRevokeAllowance() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestPruneAllowances() {
+	ctx := suite.ctx.WithBlockTime(time.Now())
+	oneYear := ctx.BlockTime().AddDate(1, 0, 0)
+
+	// We create 76 allowances, all expiring in one year
+	count := 0
+	for i := 0; i < len(suite.addrs); i++ {
+		for j := 0; j < len(suite.addrs); j++ {
+			if count == 76 {
+				break
+			}
+			if suite.addrs[i].String() == suite.addrs[j].String() {
+				continue
+			}
+
+			any, err := codectypes.NewAnyWithValue(&feegrant.BasicAllowance{
+				SpendLimit: suite.atom,
+				Expiration: &oneYear,
+			})
+			suite.Require().NoError(err)
+			req := &feegrant.MsgGrantAllowance{
+				Granter:   suite.addrs[i].String(),
+				Grantee:   suite.addrs[j].String(),
+				Allowance: any,
+			}
+
+			_, err = suite.msgSrvr.GrantAllowance(ctx, req)
+			if err != nil {
+				// do not fail, just try with another pair
+				continue
+			}
+
+			count++
+		}
+	}
+
+	// we have 76 allowances
+	count = 0
+	err := suite.feegrantKeeper.IterateAllFeeAllowances(ctx, func(grant feegrant.Grant) bool {
+		count++
+		return false
+	})
+	suite.Require().NoError(err)
+	suite.Require().Equal(76, count)
+
+	// after a year and one day passes, they are all expired
+	oneYearAndADay := ctx.BlockTime().AddDate(1, 0, 1)
+	ctx = suite.ctx.WithBlockTime(oneYearAndADay)
+
+	// we prune them, but currently only 75 will be pruned
+	_, err = suite.msgSrvr.PruneAllowances(ctx, &feegrant.MsgPruneAllowances{})
+	suite.Require().NoError(err)
+
+	// we have 1 allowance left
+	count = 0
+	err = suite.feegrantKeeper.IterateAllFeeAllowances(ctx, func(grant feegrant.Grant) bool {
+		count++
+		return false
+	})
+	suite.Require().NoError(err)
+	suite.Require().Equal(1, count)
+}
diff --git a/x/feegrant/module/abci.go b/x/feegrant/module/abci.go
@@ -1,14 +1,12 @@
 package module
 
 import (
-	"cosmossdk.io/x/feegrant/keeper"
+	"context"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
+	"cosmossdk.io/x/feegrant/keeper"
 )
 
-func EndBlocker(ctx sdk.Context, k keeper.Keeper) {
-	err := k.RemoveExpiredAllowances(ctx)
-	if err != nil {
-		panic(err)
-	}
+func EndBlocker(ctx context.Context, k keeper.Keeper) error {
+	// 200 is an arbitrary value, we can change it later if needed
+	return k.RemoveExpiredAllowances(ctx, 200)
 }
diff --git a/x/feegrant/module/abci_test.go b/x/feegrant/module/abci_test.go
@@ -77,7 +77,7 @@ func TestFeegrantPruning(t *testing.T) {
 	feegrant.RegisterQueryServer(queryHelper, feegrantKeeper)
 	queryClient := feegrant.NewQueryClient(queryHelper)
 
-	module.EndBlocker(testCtx.Ctx, feegrantKeeper)
+	require.NoError(t, module.EndBlocker(testCtx.Ctx, feegrantKeeper))
 
 	res, err := queryClient.Allowances(testCtx.Ctx.Context(), &feegrant.QueryAllowancesRequest{
 		Grantee: grantee.String(),