LaconicNetwork/cosmos-sdk
7
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 2 Wiki Activity

chore: (x/feegrant) update keeper logic to do not allow duplicate grant (#14294)

Browse Source
This commit is contained in:
atheeshp
2023-01-09 09:29:41 +00:00
committed by GitHub
parent ecdedf5db4
commit b8b34a68f5
4 changed files with 49 additions and 148 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+1
View File
@@ -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
x/feegrant/keeper/keeper.go
+14 -48
View File
@@ -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{})
x/feegrant/keeper/keeper_test.go
+34 -94
View File
@@ -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 {
x/feegrant/keeper/msg_server.go
-6
View File
@@ -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