diff --git a/CHANGELOG.md b/CHANGELOG.md index 738bd3fd9070..2000262634b4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/feegrant/keeper/keeper.go b/x/feegrant/keeper/keeper.go index 302e5f1cd243..5442b609682f 100644 --- a/x/feegrant/keeper/keeper.go +++ b/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{}) diff --git a/x/feegrant/keeper/keeper_test.go b/x/feegrant/keeper/keeper_test.go index 16d10a532cd6..6daeaec04b7d 100644 --- a/x/feegrant/keeper/keeper_test.go +++ b/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,6 +203,7 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() { fee sdk.Coins allowed bool final feegrant.FeeAllowanceI + postRun func() }{ "use entire pot": { granter: suite.addrs[0], @@ -195,6 +211,7 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() { fee: suite.atom, allowed: true, final: nil, + postRun: func() {}, }, "too high": { granter: suite.addrs[0], @@ -202,6 +219,13 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() { 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], @@ -209,13 +233,20 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() { 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 { diff --git a/x/feegrant/keeper/msg_server.go b/x/feegrant/keeper/msg_server.go index 044e7b57e170..3816a9775fe2 100644 --- a/x/feegrant/keeper/msg_server.go +++ b/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