Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

Merged
merged 16 commits into from
Jan 5, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: update allowance inside AllowedMsgAllowance
  • Loading branch information
0Tech committed Nov 17, 2021
commit 02c8e13b536527d05e5294f2f78d222caff7663b
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
* [\#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
Expand Down
9 changes: 8 additions & 1 deletion x/feegrant/filtered_fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assign the changed allowance to the old one, so the change could be applied.
There is no need to do this job if remove is true apparently.

if err != nil {
return false, sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you separate this into a separate method AllowedMsgAllowance.SetAllowance(allowance)

}
return remove, reserr
}

func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx sdk.Context) map[string]bool {
Expand Down
208 changes: 208 additions & 0 deletions x/feegrant/filtered_fee_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
package feegrant_test
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test has been copied from basic_fee_test.go.


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())
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

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
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
accept bool
remove bool
remains sdk.Coins
}{
"msg contained": {
allowance: &feegrant.BasicAllowance{},
msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"},
accept: true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the simplest case. In the test, the called message would be /cosmos.feegrant.v1beta1.MsgRevokeAllowance, so it must be accepted.

"msg not contained": {
allowance: &feegrant.BasicAllowance{},
msgs: []string{"/cosmos.feegrant.v1beta1.MsgGrantAllowance"},
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
accept: false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this case to confirm that Accept() rejects the message if it is not in its list.
Other than this case, these are just borrowed cases from basic_fee_test.go.

"small fee without expire": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
},
msgs: []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"},
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
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()
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// create some msg
call := feegrant.MsgRevokeAllowance{
Granter: "",
Grantee: "",
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// 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,
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
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
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mimic the logic as best as I can.
After Accept() has been called, the updated grant should be saved, so I added Marshal().
To check the values are all correct, we must load the grant again, so I added Unmarshal().

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, the assert against SpendLimit has been made.

}
})
}
}