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.

Sign up for GitHub

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

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
02c8e13
fix: update allowance inside AllowedMsgAllowance
0Tech Nov 17, 2021
408f75c
move the log entry into state-machine breaking section
0Tech Dec 20, 2021
4568717
remove redundant func
0Tech Dec 20, 2021
52eed35
use a primitive msg for the test case
0Tech Dec 20, 2021
143c5d9
remove hardcoded strings
0Tech Dec 20, 2021
cb8c84e
remove the noise
0Tech Dec 20, 2021
4206827
Merge branch 'master' into fix/feegrant_allowedmsg
amaury1093 Jan 3, 2022
de4745d
remove the unused field
0Tech Jan 4, 2022
092d219
fix newgrant into loadedgrant and add comments
0Tech Jan 4, 2022
0ad23de
Update x/feegrant/filtered_fee_test.go
0Tech Jan 4, 2022
8a556a6
add SetAllowance() method to AllowedMsgAllowance
0Tech Jan 4, 2022
67a7064
fix comment
0Tech Jan 4, 2022
999dcdd
Update CHANGELOG.md
0Tech Jan 4, 2022
6008d98
Merge branch 'master' into fix/feegrant_allowedmsg
amaury1093 Jan 4, 2022
bf6645b
Merge branch 'master' into fix/feegrant_allowedmsg
0Tech Jan 5, 2022
6e84e7a
Merge branch 'master' into fix/feegrant_allowedmsg
atheeshp Jan 5, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +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
0Tech marked this conversation as resolved.
Show resolved Hide resolved
* [\#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.
Expand Down
9 changes: 8 additions & 1 deletion x/feegrant/filtered_fee.go
View file
Open in desktop
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
193 changes: 193 additions & 0 deletions x/feegrant/filtered_fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
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"

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{
FromAddress: "",
ToAddress: "",
Amount: sdk.Coins{},
}
0Tech marked this conversation as resolved.
Show resolved Hide resolved
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{sdk.MsgTypeURL(&call)},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msgs: []string{sdk.MsgTypeURL(&call)},
msgs: []string{sdk.MsgTypeURL(&banktypes.MsgSend{})},

we can remove call if this works.

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 think it could be more straight-forward to use the variable call, because we don't need to compare the msgs in the cases with the actual calling msg in the loop. To be more pedantic, I have to make sure that the msg type of "msg not contained" case is not identical to that of call, but I think it's not necessary.

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)},
valid: true,
fee: smallAtom,
blockTime: now,
accept: true,
remove: false,
remains: leftAtom,
},
"expired": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
Expiration: &now,
},
msgs: []string{sdk.MsgTypeURL(&call)},
valid: true,
fee: smallAtom,
blockTime: oneHour,
accept: false,
remove: true,
},
"fee more than allowed": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
Expiration: &oneHour,
},
msgs: []string{sdk.MsgTypeURL(&call)},
valid: true,
fee: bigAtom,
blockTime: now,
accept: false,
},
"with out spend limit": {
allowance: &feegrant.BasicAllowance{
Expiration: &oneHour,
},
msgs: []string{sdk.MsgTypeURL(&call)},
valid: true,
fee: bigAtom,
blockTime: now,
accept: true,
},
"expired no spend limit": {
allowance: &feegrant.BasicAllowance{
Expiration: &now,
},
msgs: []string{sdk.MsgTypeURL(&call)},
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
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)
Comment on lines +145 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 {
// 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)
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.

}
Copy link
Contributor

@amaury1093 amaury1093 Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I actually don't understand this block so well, still.

Can we simply do:

			if !removed {
				assert.Equal(t, tc.remains, allowance.Allowance.GetCachedValue().(*feegrant.BasicAllowance).SpendLimit)
			}

Copy link
Contributor Author

@0Tech 0Tech Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This block is the whole point of this fix. With the simplified version, we cannot check whether the allowance has been updated correctly, because the cached value was correct even before the fix.
This patch is forcing the cached value applied to the state. We must marshal & unmarshal it, which simulates the actual process.
That was the last redundant func() meant for. If you don't mind, I can wrap it with func() again (or a block with curly brackets suffice?), providing meaningful name and comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, there was a mistake during the modification. I think this has confused you.
newGrant at line 185 must be loadedGrant.
I will also apply the fix after your response.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK Understood. Yes loadedGrant on L185 sounds already clearer.

Could you add some comments, e.g. around marshaling to simulate the actual process of putting in block?

If you don't mind, I can wrap it with func() again (or a block with curly brackets suffice?), providing meaningful name and comments.

I don't think that's idiomatic go. Using newlines and comments should be enough to describe what you're achieving.

Copy link
Contributor

Choose a reason for hiding this comment

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

if !removed {
	assert.Equal(t, tc.remains, allowance.GetAllowance().(*feegrant.BasicAllowance).SpendLimit)
}

I align with @AmauryM here, this thing works I guess, can you verify that this assert should be fail with the earlier code and passes with the changes in this PR. if that works no need of creating the Grant and marshals.

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 verified that the simplified version does not fail with both of the earlier code and the new code. And I also verified that the longer version with marshaling fails with the earlier code.

})
}
}