fix: (x/feegrant) expiration when overwriting an existing grant (#11845)
## Description This PR couple of bugs with expiration not being updated when we overwrite an existing grant with different expiry. **Bug 1** `grantee`, `granter` passed wrongly to the `getGrant` method here:a187a1c321/x/feegrant/keeper/keeper.go (L55)
`grantee` is passed at `granter`'s place and vice versaa187a1c321/x/feegrant/keeper/keeper.go (L181-L196)
**Bug 2** The following `if` condition never gets satisfied, `getGrant` returns `err`, `grant` with two possible out comes which are `nil`, `grant` or `err`, `nil`a187a1c321/x/feegrant/keeper/keeper.go (L56)
and also there is not only one possible error from `getGrant` with `fee-grant not found` and also one another possible error is unmarshal error, when there is an unmarshal error we should return the error. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
This commit is contained in:
parent
8416394864
commit
b1725bf151
|
@ -663,7 +663,7 @@ func (s *IntegrationTestSuite) TestNewCmdRevokeFeegrant() {
|
|||
},
|
||||
commonFlags...,
|
||||
),
|
||||
false, 4, &sdk.TxResponse{},
|
||||
false, 38, &sdk.TxResponse{},
|
||||
},
|
||||
{
|
||||
"Valid revoke",
|
||||
|
|
|
@ -52,8 +52,15 @@ func (k Keeper) GrantAllowance(ctx sdk.Context, granter, grantee sdk.AccAddress,
|
|||
key := feegrant.FeeAllowanceKey(granter, grantee)
|
||||
|
||||
var oldExp *time.Time
|
||||
existingGrant, err := k.getGrant(ctx, grantee, granter)
|
||||
if err != nil && existingGrant != nil && existingGrant.GetAllowance() != nil {
|
||||
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) {
|
||||
return err
|
||||
}
|
||||
|
||||
if existingGrant != nil && existingGrant.GetAllowance() != nil {
|
||||
grantInfo, err := existingGrant.GetGrant()
|
||||
if err != nil {
|
||||
return err
|
||||
|
@ -184,7 +191,7 @@ func (k Keeper) getGrant(ctx sdk.Context, granter sdk.AccAddress, grantee sdk.Ac
|
|||
key := feegrant.FeeAllowanceKey(granter, grantee)
|
||||
bz := store.Get(key)
|
||||
if len(bz) == 0 {
|
||||
return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "fee-grant not found")
|
||||
return nil, sdkerrors.ErrNotFound.Wrap("fee-grant not found")
|
||||
}
|
||||
|
||||
var feegrant feegrant.Grant
|
||||
|
|
|
@ -46,6 +46,7 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
|
|||
// some helpers
|
||||
eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
|
||||
exp := suite.sdkCtx.BlockTime().AddDate(1, 0, 0)
|
||||
exp2 := suite.sdkCtx.BlockTime().AddDate(2, 0, 0)
|
||||
basic := &feegrant.BasicAllowance{
|
||||
SpendLimit: suite.atom,
|
||||
Expiration: &exp,
|
||||
|
@ -56,6 +57,11 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
|
|||
Expiration: &exp,
|
||||
}
|
||||
|
||||
basic3 := &feegrant.BasicAllowance{
|
||||
SpendLimit: eth,
|
||||
Expiration: &exp2,
|
||||
}
|
||||
|
||||
// let's set up some initial state here
|
||||
err := suite.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[1], basic)
|
||||
suite.Require().NoError(err)
|
||||
|
@ -85,7 +91,7 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
|
|||
err = suite.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[0], suite.addrs[2], basic)
|
||||
suite.Require().NoError(err)
|
||||
|
||||
err = suite.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[1], suite.addrs[2], basic2)
|
||||
err = suite.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[1], suite.addrs[2], basic3)
|
||||
suite.Require().NoError(err)
|
||||
|
||||
// end state:
|
||||
|
@ -115,7 +121,7 @@ func (suite *KeeperTestSuite) TestKeeperCrud() {
|
|||
"addr modified": {
|
||||
granter: suite.addrs[1],
|
||||
grantee: suite.addrs[2],
|
||||
allowance: basic2,
|
||||
allowance: basic3,
|
||||
},
|
||||
}
|
||||
|
||||
|
@ -265,6 +271,7 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
|
|||
eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))
|
||||
now := suite.sdkCtx.BlockTime()
|
||||
oneYearExpiry := now.AddDate(1, 0, 0)
|
||||
oneDay := now.AddDate(0, 0, 1)
|
||||
|
||||
testCases := []struct {
|
||||
name string
|
||||
|
@ -273,6 +280,8 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
|
|||
grantee sdk.AccAddress
|
||||
allowance feegrant.FeeAllowanceI
|
||||
expErrMsg string
|
||||
preRun func()
|
||||
postRun func()
|
||||
}{
|
||||
{
|
||||
name: "grant not pruned from state",
|
||||
|
@ -326,12 +335,108 @@ 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.sdkCtx.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.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[2], suite.addrs[1], allowance)
|
||||
suite.NoError(err)
|
||||
},
|
||||
postRun: func() {
|
||||
_, err := suite.msgSrvr.RevokeAllowance(suite.sdkCtx, &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.sdkCtx.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.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[2], suite.addrs[1], allowance)
|
||||
suite.NoError(err)
|
||||
},
|
||||
postRun: func() {
|
||||
_, err := suite.msgSrvr.RevokeAllowance(suite.sdkCtx, &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.sdkCtx.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.keeper.GrantAllowance(suite.sdkCtx, 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.sdkCtx.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.keeper.GrantAllowance(suite.sdkCtx, suite.addrs[2], suite.addrs[1], allowance)
|
||||
suite.NoError(err)
|
||||
},
|
||||
postRun: func() {},
|
||||
expErrMsg: "not found",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
tc := tc
|
||||
suite.Run(tc.name, func() {
|
||||
suite.keeper.GrantAllowance(suite.sdkCtx, tc.granter, tc.grantee, tc.allowance)
|
||||
if tc.preRun != nil {
|
||||
tc.preRun()
|
||||
}
|
||||
err := suite.keeper.GrantAllowance(suite.sdkCtx, tc.granter, tc.grantee, tc.allowance)
|
||||
suite.NoError(err)
|
||||
suite.app.FeeGrantKeeper.RemoveExpiredAllowances(tc.ctx)
|
||||
grant, err := suite.keeper.GetAllowance(tc.ctx, tc.granter, tc.grantee)
|
||||
if tc.expErrMsg != "" {
|
||||
|
@ -340,6 +445,9 @@ func (suite *KeeperTestSuite) TestPruneGrants() {
|
|||
} else {
|
||||
suite.NotNil(grant)
|
||||
}
|
||||
if tc.postRun != nil {
|
||||
tc.postRun()
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue