From 02c8e13b536527d05e5294f2f78d222caff7663b Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Wed, 17 Nov 2021 16:20:31 +0900 Subject: [PATCH 01/12] fix: update allowance inside AllowedMsgAllowance --- CHANGELOG.md | 1 + x/feegrant/filtered_fee.go | 9 +- x/feegrant/filtered_fee_test.go | 208 ++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 x/feegrant/filtered_fee_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e3e0868081a..a70157405b83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Update allowance inside AllowedMsgAllowance * [\#10414](https://github.com/cosmos/cosmos-sdk/pull/10414) Use `sdk.GetConfig().GetFullBIP44Path()` instead `sdk.FullFundraiserPath` to generate key * (rosetta) [\#10340](https://github.com/cosmos/cosmos-sdk/pull/10340) Use `GenesisChunked(ctx)` instead `Genesis(ctx)` to get genesis block height * [#10180](https://github.com/cosmos/cosmos-sdk/issues/10180) Documentation: make references to Cosmos SDK consistent diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index d842e40852af..45d7e77842e2 100644 --- a/x/feegrant/filtered_fee.go +++ b/x/feegrant/filtered_fee.go @@ -61,7 +61,14 @@ func (a *AllowedMsgAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk. return false, err } - return allowance.Accept(ctx, fee, msgs) + remove, reserr := allowance.Accept(ctx, fee, msgs) + if !remove { + a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message)) + if err != nil { + return false, sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance) + } + } + return remove, reserr } 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 000000000000..405bcf796700 --- /dev/null +++ b/x/feegrant/filtered_fee_test.go @@ -0,0 +1,208 @@ +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" +) + +func TestFilteredFeeValidAllow(t *testing.T) { + app := simapp.Setup(t, false) + + ctx := app.BaseApp.NewContext(false, ocproto.Header{}) + badTime := ctx.BlockTime().AddDate(0, 0, -1) + allowace := &feegrant.BasicAllowance{ + Expiration: &badTime, + } + require.Error(t, allowace.ValidateBasic()) + + 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) + + cases := map[string]struct { + allowance *feegrant.BasicAllowance + msgs []string + // all other checks are ignored if valid=false + fee sdk.Coins + blockTime time.Time + valid bool + accept bool + remove bool + remains sdk.Coins + }{ + "msg contained": { + allowance: &feegrant.BasicAllowance{}, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + accept: true, + }, + "msg not contained": { + allowance: &feegrant.BasicAllowance{}, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgGrantAllowance"}, + accept: false, + }, + "small fee without expire": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + fee: smallAtom, + accept: true, + remove: false, + remains: leftAtom, + }, + "all fee without expire": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: smallAtom, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + fee: smallAtom, + accept: true, + remove: true, + }, + "wrong fee": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: smallAtom, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + fee: eth, + accept: false, + }, + "non-expired": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &oneHour, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + valid: true, + fee: smallAtom, + blockTime: now, + accept: true, + remove: false, + remains: leftAtom, + }, + "expired": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &now, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + valid: true, + fee: smallAtom, + blockTime: oneHour, + accept: false, + remove: true, + }, + "fee more than allowed": { + allowance: &feegrant.BasicAllowance{ + SpendLimit: atom, + Expiration: &oneHour, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + valid: true, + fee: bigAtom, + blockTime: now, + accept: false, + }, + "with out spend limit": { + allowance: &feegrant.BasicAllowance{ + Expiration: &oneHour, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + valid: true, + fee: bigAtom, + blockTime: now, + accept: true, + }, + "expired no spend limit": { + allowance: &feegrant.BasicAllowance{ + Expiration: &now, + }, + msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + valid: true, + 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 + createGrant := func() feegrant.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) + return grant + } + grant := createGrant() + + // create some msg + call := feegrant.MsgRevokeAllowance{ + Granter: "", + Grantee: "", + } + + // now try to deduct + allowance, err := grant.GetGrant() + require.NoError(t, err) + 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 { + updatedGrant := func(granter, grantee sdk.AccAddress, + allowance feegrant.FeeAllowanceI) feegrant.Grant { + newGrant, err := feegrant.NewGrant( + granter, + grantee, + allowance) + require.NoError(t, err) + + cdc := simapp.MakeTestEncodingConfig().Codec + bz, err := cdc.Marshal(&newGrant) + require.NoError(t, err) + + var loaded feegrant.Grant + err = cdc.Unmarshal(bz, &loaded) + require.NoError(t, err) + return loaded + } + newGrant := updatedGrant(sdk.AccAddress(grant.Granter), + sdk.AccAddress(grant.Grantee), allowance) + + newAllowance, err := newGrant.GetGrant() + require.NoError(t, err) + feeAllowance, err := newAllowance.(*feegrant.AllowedMsgAllowance).GetAllowance() + require.NoError(t, err) + assert.Equal(t, tc.remains, feeAllowance.(*feegrant.BasicAllowance).SpendLimit) + } + }) + } +} From 408f75c6848b683f3ad185a7c7b291aa1b2e5486 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 20 Dec 2021 04:27:19 +0000 Subject: [PATCH 02/12] move the log entry into state-machine breaking section --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a70157405b83..ef3a7ed0d99c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,7 +137,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Update allowance inside AllowedMsgAllowance * [\#10414](https://github.com/cosmos/cosmos-sdk/pull/10414) Use `sdk.GetConfig().GetFullBIP44Path()` instead `sdk.FullFundraiserPath` to generate key * (rosetta) [\#10340](https://github.com/cosmos/cosmos-sdk/pull/10340) Use `GenesisChunked(ctx)` instead `Genesis(ctx)` to get genesis block height * [#10180](https://github.com/cosmos/cosmos-sdk/issues/10180) Documentation: make references to Cosmos SDK consistent @@ -163,6 +162,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Update allowance inside AllowedMsgAllowance * [\#10536](https://github.com/cosmos/cosmos-sdk/pull/10536]) Enable `SetSequence` for `ModuleAccount`. * (x/staking) [#10254](https://github.com/cosmos/cosmos-sdk/pull/10254) Instead of using the shares to determine if a delegation should be removed, use the truncated (token) amount. * (store) [#10247](https://github.com/cosmos/cosmos-sdk/pull/10247) Charge gas for the key length in gas meter. From 4568717b97562a84f134427d2048f97b335208a5 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 20 Dec 2021 04:42:34 +0000 Subject: [PATCH 03/12] remove redundant func --- x/feegrant/filtered_fee_test.go | 50 +++++++++++++-------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index 405bcf796700..7c6ea72e809c 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -149,15 +149,11 @@ func TestFilteredFeeValidAllow(t *testing.T) { ctx := app.BaseApp.NewContext(false, ocproto.Header{}).WithBlockTime(tc.blockTime) // create grant - createGrant := func() feegrant.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) - return grant - } - grant := createGrant() + 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) // create some msg call := feegrant.MsgRevokeAllowance{ @@ -166,8 +162,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { } // now try to deduct - allowance, err := grant.GetGrant() - require.NoError(t, err) removed, err := allowance.Accept(ctx, tc.fee, []sdk.Msg{&call}) if !tc.accept { require.Error(t, err) @@ -177,25 +171,21 @@ func TestFilteredFeeValidAllow(t *testing.T) { require.Equal(t, tc.remove, removed) if !removed { - updatedGrant := func(granter, grantee sdk.AccAddress, - allowance feegrant.FeeAllowanceI) feegrant.Grant { - newGrant, err := feegrant.NewGrant( - granter, - grantee, - allowance) - require.NoError(t, err) - - cdc := simapp.MakeTestEncodingConfig().Codec - bz, err := cdc.Marshal(&newGrant) - require.NoError(t, err) - - var loaded feegrant.Grant - err = cdc.Unmarshal(bz, &loaded) - require.NoError(t, err) - return loaded - } - newGrant := updatedGrant(sdk.AccAddress(grant.Granter), - sdk.AccAddress(grant.Grantee), allowance) + // save the new grant + newGrant, err := feegrant.NewGrant( + sdk.AccAddress(grant.Granter), + sdk.AccAddress(grant.Grantee), + allowance) + require.NoError(t, err) + + 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 := newGrant.GetGrant() require.NoError(t, err) From 52eed35f86ad2f59df6f9102a376b83f2b00b67a Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 20 Dec 2021 04:44:20 +0000 Subject: [PATCH 04/12] use a primitive msg for the test case --- x/feegrant/filtered_fee_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index 7c6ea72e809c..a9d9634cb6d4 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -52,7 +52,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { }, "msg not contained": { allowance: &feegrant.BasicAllowance{}, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgGrantAllowance"}, + msgs: []string{"/cosmos.bank.v1beta1.MsgSend"}, accept: false, }, "small fee without expire": { From 143c5d9daef6448561b4402ca5796d276cfb8a4c Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 20 Dec 2021 04:57:58 +0000 Subject: [PATCH 05/12] remove hardcoded strings --- x/feegrant/filtered_fee_test.go | 34 +++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index a9d9634cb6d4..cc1bc6f3abe9 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -11,6 +11,8 @@ import ( "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) { @@ -34,6 +36,12 @@ func TestFilteredFeeValidAllow(t *testing.T) { now := ctx.BlockTime() oneHour := now.Add(1 * time.Hour) + // msg we will call in the all cases + call := banktypes.MsgSend{ + FromAddress: "", + ToAddress: "", + Amount: sdk.Coins{}, + } cases := map[string]struct { allowance *feegrant.BasicAllowance msgs []string @@ -47,19 +55,19 @@ func TestFilteredFeeValidAllow(t *testing.T) { }{ "msg contained": { allowance: &feegrant.BasicAllowance{}, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, accept: true, }, "msg not contained": { allowance: &feegrant.BasicAllowance{}, - msgs: []string{"/cosmos.bank.v1beta1.MsgSend"}, + msgs: []string{"/cosmos.gov.v1beta1.MsgVote"}, accept: false, }, "small fee without expire": { allowance: &feegrant.BasicAllowance{ SpendLimit: atom, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, fee: smallAtom, accept: true, remove: false, @@ -69,7 +77,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { allowance: &feegrant.BasicAllowance{ SpendLimit: smallAtom, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, fee: smallAtom, accept: true, remove: true, @@ -78,7 +86,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { allowance: &feegrant.BasicAllowance{ SpendLimit: smallAtom, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, fee: eth, accept: false, }, @@ -87,7 +95,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { SpendLimit: atom, Expiration: &oneHour, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, valid: true, fee: smallAtom, blockTime: now, @@ -100,7 +108,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { SpendLimit: atom, Expiration: &now, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, valid: true, fee: smallAtom, blockTime: oneHour, @@ -112,7 +120,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { SpendLimit: atom, Expiration: &oneHour, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, valid: true, fee: bigAtom, blockTime: now, @@ -122,7 +130,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { allowance: &feegrant.BasicAllowance{ Expiration: &oneHour, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, valid: true, fee: bigAtom, blockTime: now, @@ -132,7 +140,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { allowance: &feegrant.BasicAllowance{ Expiration: &now, }, - msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"}, + msgs: []string{sdk.MsgTypeURL(&call)}, valid: true, fee: bigAtom, blockTime: oneHour, @@ -155,12 +163,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { grant, err := feegrant.NewGrant(granter, grantee, allowance) require.NoError(t, err) - // create some msg - call := feegrant.MsgRevokeAllowance{ - Granter: "", - Grantee: "", - } - // now try to deduct removed, err := allowance.Accept(ctx, tc.fee, []sdk.Msg{&call}) if !tc.accept { From cb8c84e6180f1866a138f857a16225d6d1e8b609 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 20 Dec 2021 05:13:04 +0000 Subject: [PATCH 06/12] remove the noise --- x/feegrant/filtered_fee_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index cc1bc6f3abe9..cf5dcd21a842 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -18,14 +18,7 @@ import ( func TestFilteredFeeValidAllow(t *testing.T) { app := simapp.Setup(t, false) - ctx := app.BaseApp.NewContext(false, ocproto.Header{}) - badTime := ctx.BlockTime().AddDate(0, 0, -1) - allowace := &feegrant.BasicAllowance{ - Expiration: &badTime, - } - require.Error(t, allowace.ValidateBasic()) - - ctx = app.BaseApp.NewContext(false, ocproto.Header{ + ctx := app.BaseApp.NewContext(false, ocproto.Header{ Time: time.Now(), }) eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 10)) From de4745d2f0135071c10701bdbec81ad58894018c Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 06:05:20 +0000 Subject: [PATCH 07/12] remove the unused field --- x/feegrant/filtered_fee_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index cf5dcd21a842..d94639e19587 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -38,10 +38,8 @@ func TestFilteredFeeValidAllow(t *testing.T) { cases := map[string]struct { allowance *feegrant.BasicAllowance msgs []string - // all other checks are ignored if valid=false fee sdk.Coins blockTime time.Time - valid bool accept bool remove bool remains sdk.Coins @@ -89,7 +87,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { Expiration: &oneHour, }, msgs: []string{sdk.MsgTypeURL(&call)}, - valid: true, fee: smallAtom, blockTime: now, accept: true, @@ -102,7 +99,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { Expiration: &now, }, msgs: []string{sdk.MsgTypeURL(&call)}, - valid: true, fee: smallAtom, blockTime: oneHour, accept: false, @@ -114,7 +110,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { Expiration: &oneHour, }, msgs: []string{sdk.MsgTypeURL(&call)}, - valid: true, fee: bigAtom, blockTime: now, accept: false, @@ -124,7 +119,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { Expiration: &oneHour, }, msgs: []string{sdk.MsgTypeURL(&call)}, - valid: true, fee: bigAtom, blockTime: now, accept: true, @@ -134,7 +128,6 @@ func TestFilteredFeeValidAllow(t *testing.T) { Expiration: &now, }, msgs: []string{sdk.MsgTypeURL(&call)}, - valid: true, fee: bigAtom, blockTime: oneHour, accept: false, From 092d219f5e16343af36d04e426920fc8f21efab3 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 11:33:44 +0000 Subject: [PATCH 08/12] fix newgrant into loadedgrant and add comments --- x/feegrant/filtered_fee_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index d94639e19587..cf98e03f0dba 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -159,13 +159,19 @@ func TestFilteredFeeValidAllow(t *testing.T) { require.Equal(t, tc.remove, removed) if !removed { - // save the new grant + // 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) @@ -175,7 +181,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { err = cdc.Unmarshal(bz, &loadedGrant) require.NoError(t, err) - newAllowance, err := newGrant.GetGrant() + newAllowance, err := loadedGrant.GetGrant() require.NoError(t, err) feeAllowance, err := newAllowance.(*feegrant.AllowedMsgAllowance).GetAllowance() require.NoError(t, err) From 0ad23dec23661c573716c24bec5528076ad12cae Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 21:14:15 +0900 Subject: [PATCH 09/12] Update x/feegrant/filtered_fee_test.go Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com> --- x/feegrant/filtered_fee_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index cf98e03f0dba..6da02a800a4d 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -30,11 +30,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { oneHour := now.Add(1 * time.Hour) // msg we will call in the all cases - call := banktypes.MsgSend{ - FromAddress: "", - ToAddress: "", - Amount: sdk.Coins{}, - } + call := banktypes.MsgSend{} cases := map[string]struct { allowance *feegrant.BasicAllowance msgs []string From 8a556a609b301999eea315cddd15e62a0279e6fb Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 12:34:43 +0000 Subject: [PATCH 10/12] add SetAllowance() method to AllowedMsgAllowance --- x/feegrant/filtered_fee.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index 45d7e77842e2..63c8c91eb97e 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 returns 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,14 +72,13 @@ func (a *AllowedMsgAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk. return false, err } - remove, reserr := allowance.Accept(ctx, fee, msgs) - if !remove { - a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message)) - if err != nil { - return false, sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance) + remove, err := allowance.Accept(ctx, fee, msgs) + if err == nil && !remove { + if err = a.SetAllowance(allowance); err != nil { + return false, err } } - return remove, reserr + return remove, err } func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx sdk.Context) map[string]bool { From 67a7064bb7c06c7ed2451a753ed5ba244d72c59e Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 12:55:10 +0000 Subject: [PATCH 11/12] fix comment --- x/feegrant/filtered_fee.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index 63c8c91eb97e..d1b5b0a81ee6 100644 --- a/x/feegrant/filtered_fee.go +++ b/x/feegrant/filtered_fee.go @@ -50,7 +50,7 @@ func (a *AllowedMsgAllowance) GetAllowance() (FeeAllowanceI, error) { return allowance, nil } -// SetAllowance returns allowed fee allowance. +// SetAllowance sets allowed fee allowance. func (a *AllowedMsgAllowance) SetAllowance(allowance FeeAllowanceI) error { var err error a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message)) From 999dcdd55674730b3a7a8704c5a67808dd5a2102 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Tue, 4 Jan 2022 22:02:54 +0900 Subject: [PATCH 12/12] Update CHANGELOG.md Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfdc186a0536..fe0779ea6180 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,7 +188,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking -* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Update allowance inside AllowedMsgAllowance +* [\#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.