refactor(x/feegrant)!: audit QA v0.52

tests/sims/feegrant/operations_test.go

@@ -28,7 +28,6 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/codec"
-	codecaddress "github.com/cosmos/cosmos-sdk/codec/address"
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/runtime"
 	"github.com/cosmos/cosmos-sdk/testutil/configurator"
@@ -101,9 +100,8 @@ func (suite *SimTestSuite) TestWeightedOperations() {
 	appParams := make(simtypes.AppParams)
 
 	weightedOps := simulation.WeightedOperations(
-		suite.interfaceRegistry,
-		appParams, suite.cdc, suite.txConfig, suite.accountKeeper,
-		suite.bankKeeper, suite.feegrantKeeper, codecaddress.NewBech32Codec("cosmos"),
+		appParams, suite.txConfig, suite.accountKeeper,
+		suite.bankKeeper, suite.feegrantKeeper,
 	)
 
 	s := rand.NewSource(1)
@@ -153,7 +151,7 @@ func (suite *SimTestSuite) TestSimulateMsgGrantAllowance() {
 	require.NoError(err)
 
 	// execute operation
-	op := simulation.SimulateMsgGrantAllowance(codec.NewProtoCodec(suite.interfaceRegistry), suite.txConfig, suite.accountKeeper, suite.bankKeeper, suite.feegrantKeeper)
+	op := simulation.SimulateMsgGrantAllowance(suite.txConfig, suite.accountKeeper, suite.bankKeeper, suite.feegrantKeeper)
 	operationMsg, futureOperations, err := op(r, app.BaseApp, ctx.WithHeaderInfo(header.Info{Time: time.Now()}), accounts, "")
 	require.NoError(err)
 
@@ -197,7 +195,7 @@ func (suite *SimTestSuite) TestSimulateMsgRevokeAllowance() {
 	require.NoError(err)
 
 	// execute operation
-	op := simulation.SimulateMsgRevokeAllowance(codec.NewProtoCodec(suite.interfaceRegistry), suite.txConfig, suite.accountKeeper, suite.bankKeeper, suite.feegrantKeeper)
+	op := simulation.SimulateMsgRevokeAllowance(suite.txConfig, suite.accountKeeper, suite.bankKeeper, suite.feegrantKeeper)
 	operationMsg, futureOperations, err := op(r, app.BaseApp, ctx, accounts, "")
 	require.NoError(err)
 

x/feegrant/CHANGELOG.md

@@ -31,12 +31,15 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking Changes
 
+* [#21377](https://github.com/cosmos/cosmos-sdk/pull/21377) Simulation API breaking changes:
+  * `SimulateMsgGrantAllowance` and `SimulateMsgRevokeAllowance` no longer require a `ProtoCodec` parameter.
+  * `WeightedOperations` functions no longer require `ProtoCodec`, `JSONCodec`, or `address.Codec` parameters.
 * [#20529](https://github.com/cosmos/cosmos-sdk/pull/20529) `Accept` on the `FeeAllowanceI` interface now expects the feegrant environment in the `context.Context`.
 * [#19450](https://github.com/cosmos/cosmos-sdk/pull/19450) Migrate module to use `appmodule.Environment` instead of passing individual services.
 
 ### Consensus Breaking Changes
 
-* [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist
+* [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist.
 
 ## [v0.1.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/feegrant/v0.1.1) - 2024-04-22
 

x/feegrant/basic_fee.go

@@ -72,4 +72,4 @@ func (a BasicAllowance) ExpiresAt() (*time.Time, error) {
 }
 
 // UpdatePeriodReset BasicAllowance does not update "PeriodReset"
-func (a BasicAllowance) UpdatePeriodReset(validTime time.Time) error { return nil }
+func (a BasicAllowance) UpdatePeriodReset(_ time.Time) error { return nil }

x/feegrant/basic_fee_test.go

@@ -130,8 +130,7 @@ func TestBasicFeeValidAllow(t *testing.T) {
 		},
 	}
 
-	for name, stc := range cases {
-		tc := stc // to make scopelint happy
+	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			err := tc.allowance.UpdatePeriodReset(tc.blockTime)
 			require.NoError(t, err)

x/feegrant/client/cli/tx_test.go

@@ -446,7 +446,7 @@ func (s *CLITestSuite) TestTxWithFeeGrant() {
 	granterAddr, err := s.baseCtx.AddressCodec.BytesToString(granter)
 	s.Require().NoError(err)
 
-	// creating an account manually (This account won't be exist in state)
+	// creating an account manually (This account won't exist in state)
 	k, _, err := s.baseCtx.Keyring.NewMnemonic("grantee", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
 	s.Require().NoError(err)
 	pub, err := k.GetPubKey()

x/feegrant/fees.go

@@ -19,7 +19,7 @@ type FeeAllowanceI interface {
 	// and will be saved again after an acceptance.
 	//
 	// If remove is true (regardless of the error), the FeeAllowance will be deleted from storage
-	// (eg. when it is used up). (See call to RevokeAllowance in Keeper.UseGrantedFees)
+	// (e.g. when it is used up). (See call to RevokeAllowance in Keeper.UseGrantedFees)
 	Accept(ctx context.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error)
 
 	// ValidateBasic should evaluate this FeeAllowance for internal consistency.

x/feegrant/filtered_fee.go

@@ -63,12 +63,13 @@ func (a *AllowedMsgAllowance) GetAllowance() (FeeAllowanceI, error) {
 
 // SetAllowance sets allowed fee allowance.
 func (a *AllowedMsgAllowance) SetAllowance(allowance FeeAllowanceI) error {
-	var err error
-	a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message))
+	newAllowance, err := types.NewAnyWithValue(allowance.(proto.Message))
 	if err != nil {
 		return errorsmod.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance)
 	}
 
+	a.Allowance = newAllowance
+
 	return nil
 }
 
@@ -96,8 +97,8 @@ func (a *AllowedMsgAllowance) Accept(ctx context.Context, fee sdk.Coins, msgs []
 	return remove, err
 }
 
-func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx context.Context) (map[string]bool, error) {
-	msgsMap := make(map[string]bool, len(a.AllowedMessages))
+func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx context.Context) (map[string]struct{}, error) {
+	msgsMap := make(map[string]struct{}, len(a.AllowedMessages))
 	environment, ok := ctx.Value(corecontext.EnvironmentContextKey).(appmodule.Environment)
 	if !ok {
 		return nil, errors.New("environment not set")
@@ -107,7 +108,7 @@ func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx context.Context) (map[string]
 		if err := gasMeter.Consume(gasCostPerIteration, "check msg"); err != nil {
 			return nil, err
 		}
-		msgsMap[msg] = true
+		msgsMap[msg] = struct{}{}
 	}
 
 	return msgsMap, nil
@@ -127,7 +128,7 @@ func (a *AllowedMsgAllowance) allMsgTypesAllowed(ctx context.Context, msgs []sdk
 		if err := gasMeter.Consume(gasCostPerIteration, "check msg"); err != nil {
 			return false, err
 		}
-		if !msgsMap[sdk.MsgTypeURL(msg)] {
+		if _, allowed := msgsMap[sdk.MsgTypeURL(msg)]; !allowed {
 			return false, nil
 		}
 	}

x/feegrant/filtered_fee_test.go

@@ -141,16 +141,15 @@ func TestFilteredFeeValidAllow(t *testing.T) {
 		},
 	}
 
-	for name, stc := range cases {
-		tc := stc // to make scopelint happy
+	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			err := tc.allowance.ValidateBasic()
 			require.NoError(t, err)
 
 			ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: tc.blockTime})
 
 			// create grant
-			var granter, grantee sdk.AccAddress
+			granter, grantee := sdk.AccAddress("granter"), sdk.AccAddress("grantee")
 			allowance, err := feegrant.NewAllowedMsgAllowance(tc.allowance, tc.msgs)
 			require.NoError(t, err)
 			granterStr, err := ac.BytesToString(granter)

x/feegrant/keeper/keeper.go

@@ -190,7 +190,7 @@ func (k Keeper) revokeAllowance(ctx context.Context, granter, grantee sdk.AccAdd
 }
 
 // GetAllowance returns the allowance between the granter and grantee.
-// If there is none, it returns nil, nil.
+// If there is none, it returns nil, collections.ErrNotFound.
 // Returns an error on parsing issues
 func (k Keeper) GetAllowance(ctx context.Context, granter, grantee sdk.AccAddress) (feegrant.FeeAllowanceI, error) {
 	grant, err := k.FeeAllowance.Get(ctx, collections.Join(grantee, granter))

x/feegrant/keeper/keeper_test.go

@@ -167,7 +167,6 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
 	}
 
 	for name, tc := range cases {
-		tc := tc
 		suite.Run(name, func() {
 			allow, _ := suite.feegrantKeeper.GetAllowance(suite.ctx, tc.granter, tc.grantee)
 
@@ -261,7 +260,6 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 	}
 
 	for name, tc := range cases {
-		tc := tc
 		suite.Run(name, func() {
 			err := suite.feegrantKeeper.GrantAllowance(suite.ctx, tc.granter, tc.grantee, future)
 			suite.Require().NoError(err)
@@ -298,9 +296,9 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 	suite.Contains(err.Error(), "fee allowance expired")
 
 	// verify: feegrant is not revoked
-	// Because using the past feegrant will return err, data will be rolled back in actual scenarios.
-	// Only when the feegrant allowance used up in a certain transaction feegrant will revoked success due to err is nil
-	// abci's EndBlocker will remove the expired feegrant.
+	// The expired feegrant is not automatically revoked when attempting to use it.
+	// This is because the transaction using an expired feegrant would fail and be rolled back.
+	// Expired feegrants are typically cleaned up by the ABCI EndBlocker, not by failed usage attempts.
 	_, err = suite.feegrantKeeper.GetAllowance(ctx, suite.addrs[0], suite.addrs[2])
 	suite.Require().NoError(err)
 }
@@ -344,8 +342,6 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 		grantee   sdk.AccAddress
 		allowance feegrant.FeeAllowanceI
 		expErrMsg string
-		preRun    func()
-		postRun   func()
 	}{
 		{
 			name:      "grant pruned from state after a block: error",
@@ -412,11 +408,7 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 	}
 
 	for _, tc := range testCases {
-		tc := tc
 		suite.Run(tc.name, func() {
-			if tc.preRun != nil {
-				tc.preRun()
-			}
 			err := suite.feegrantKeeper.GrantAllowance(suite.ctx, tc.granter, tc.grantee, tc.allowance)
 			suite.NoError(err)
 			err = suite.feegrantKeeper.RemoveExpiredAllowances(tc.ctx, 5)
@@ -430,10 +422,6 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
 				suite.NoError(err)
 				suite.NotNil(grant)
 			}
-
-			if tc.postRun != nil {
-				tc.postRun()
-			}
 		})
 	}
 }