From 651ef9ea6eadbe289e68916a5ff902de9c078921 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Wed, 5 Jan 2022 20:40:20 +0900 Subject: [PATCH] fix: fix bug when updating allowance inside AllowedMsgAllowance (#10564) ## Description Closes: #10563 By the fix, AllowedMsgAllowance updates its allowance after the change in `Accept()`. To catch the corresponding issue, the fix also adds new unit tests. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] 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) --- CHANGELOG.md | 1 + x/feegrant/filtered_fee.go | 19 +++- x/feegrant/filtered_fee_test.go | 188 ++++++++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 x/feegrant/filtered_fee_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cddab6efc..ee19e9cc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -190,6 +190,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Fix bug when updating allowance inside AllowedMsgAllowance * [\#10536](https://github.com/cosmos/cosmos-sdk/pull/10536]) Enable `SetSequence` for `ModuleAccount`. * (store) [#10247](https://github.com/cosmos/cosmos-sdk/pull/10247) Charge gas for the key length in gas meter. * (store) [#10218](https://github.com/cosmos/cosmos-sdk/pull/10218) Charge gas even when there are no entries while seeking. diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index d842e4085..d1b5b0a81 100644 --- a/x/feegrant/filtered_fee.go +++ b/x/feegrant/filtered_fee.go @@ -50,6 +50,17 @@ func (a *AllowedMsgAllowance) GetAllowance() (FeeAllowanceI, error) { return allowance, nil } +// SetAllowance sets allowed fee allowance. +func (a *AllowedMsgAllowance) SetAllowance(allowance FeeAllowanceI) error { + var err error + a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message)) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance) + } + + return nil +} + // Accept method checks for the filtered messages has valid expiry func (a *AllowedMsgAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (bool, error) { if !a.allMsgTypesAllowed(ctx, msgs) { @@ -61,7 +72,13 @@ func (a *AllowedMsgAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk. return false, err } - return allowance.Accept(ctx, fee, msgs) + remove, err := allowance.Accept(ctx, fee, msgs) + if err == nil && !remove { + if err = a.SetAllowance(allowance); err != nil { + return false, err + } + } + return remove, err } func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx sdk.Context) map[string]bool { diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go new file mode 100644 index 000000000..6da02a800 --- /dev/null +++ b/x/feegrant/filtered_fee_test.go @@ -0,0 +1,188 @@ +package feegrant_test + +import ( + "testing" + "time" + + "github.com/cosmos/cosmos-sdk/x/feegrant" + ocproto "github.com/tendermint/tendermint/proto/tendermint/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/cosmos/cosmos-sdk/simapp" + sdk "github.com/cosmos/cosmos-sdk/types" + + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" +) + +func TestFilteredFeeValidAllow(t *testing.T) { + app := simapp.Setup(t, false) + + ctx := app.BaseApp.NewContext(false, ocproto.Header{ + Time: time.Now(), + }) + eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 10)) + atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555)) + smallAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 43)) + bigAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 1000)) + leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512)) + now := ctx.BlockTime() + oneHour := now.Add(1 * time.Hour) + + // msg we will call in the all cases + call := banktypes.MsgSend{} + cases := map[string]struct { + allowance *feegrant.BasicAllowance + msgs []string + fee sdk.Coins + blockTime time.Time + accept bool + remove bool + remains sdk.Coins + }{ + "msg contained": { + allowance: &feegrant.BasicAllowance{}, + msgs: []string{sdk.MsgTypeURL(&call)}, + accept: true, + }, + "msg not contained": { + allowance: &feegrant.BasicAllowance{}, + msgs: []string{"/cosmos.gov.v1beta1.MsgVote"}, + accept: false, + }, + "small fee without expire": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: smallAtom, + accept: true, + remove: false, + remains: leftAtom, + }, + "all fee without expire": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: smallAtom, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: smallAtom, + accept: true, + remove: true, + }, + "wrong fee": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: smallAtom, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: eth, + accept: false, + }, + "non-expired": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &oneHour, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: smallAtom, + blockTime: now, + accept: true, + remove: false, + remains: leftAtom, + }, + "expired": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &now, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: smallAtom, + blockTime: oneHour, + accept: false, + remove: true, + }, + "fee more than allowed": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &oneHour, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: bigAtom, + blockTime: now, + accept: false, + }, + "with out spend limit": { + allowance: &feegrant.BasicAllowance{ + Expiration: &oneHour, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: bigAtom, + blockTime: now, + accept: true, + }, + "expired no spend limit": { + allowance: &feegrant.BasicAllowance{ + Expiration: &now, + }, + msgs: []string{sdk.MsgTypeURL(&call)}, + fee: bigAtom, + blockTime: oneHour, + accept: false, + }, + } + + for name, stc := range cases { + tc := stc // to make scopelint happy + t.Run(name, func(t *testing.T) { + err := tc.allowance.ValidateBasic() + require.NoError(t, err) + + ctx := app.BaseApp.NewContext(false, ocproto.Header{}).WithBlockTime(tc.blockTime) + + // create grant + var granter, grantee sdk.AccAddress + allowance, err := feegrant.NewAllowedMsgAllowance(tc.allowance, tc.msgs) + require.NoError(t, err) + grant, err := feegrant.NewGrant(granter, grantee, allowance) + require.NoError(t, err) + + // now try to deduct + removed, err := allowance.Accept(ctx, tc.fee, []sdk.Msg{&call}) + if !tc.accept { + require.Error(t, err) + return + } + require.NoError(t, err) + + require.Equal(t, tc.remove, removed) + if !removed { + // mimic save & load process (#10564) + // the cached allowance was correct even before the fix, + // however, the saved value was not. + // so we need this to catch the bug. + + // create a new updated grant + newGrant, err := feegrant.NewGrant( + sdk.AccAddress(grant.Granter), + sdk.AccAddress(grant.Grantee), + allowance) + require.NoError(t, err) + + // save the grant + cdc := simapp.MakeTestEncodingConfig().Codec + bz, err := cdc.Marshal(&newGrant) + require.NoError(t, err) + + // load the grant + var loadedGrant feegrant.Grant + err = cdc.Unmarshal(bz, &loadedGrant) + require.NoError(t, err) + + newAllowance, err := loadedGrant.GetGrant() + require.NoError(t, err) + feeAllowance, err := newAllowance.(*feegrant.AllowedMsgAllowance).GetAllowance() + require.NoError(t, err) + assert.Equal(t, tc.remains, feeAllowance.(*feegrant.BasicAllowance).SpendLimit) + } + }) + } +}