fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

0Tech · 2021-11-17T10:12:13Z

Description

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...

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 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)

0Tech

I have reviewed all of my changes, and added some comments.

0Tech · 2021-11-17T10:26:37Z

x/feegrant/filtered_fee.go

-	return allowance.Accept(ctx, fee, msgs)
+	remove, reserr := allowance.Accept(ctx, fee, msgs)
+	if !remove {
+		a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message))


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.

0Tech · 2021-11-17T10:27:18Z

x/feegrant/filtered_fee_test.go

@@ -0,0 +1,208 @@
+package feegrant_test


The test has been copied from basic_fee_test.go.

0Tech · 2021-11-17T10:29:22Z

x/feegrant/filtered_fee_test.go

+		"msg contained": {
+			allowance: &feegrant.BasicAllowance{},
+			msgs:      []string{"/cosmos.feegrant.v1beta1.MsgRevokeAllowance"},
+			accept:    true,
+		},


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

0Tech · 2021-11-17T10:33:32Z

x/feegrant/filtered_fee_test.go

+		"msg not contained": {
+			allowance: &feegrant.BasicAllowance{},
+			msgs:      []string{"/cosmos.feegrant.v1beta1.MsgGrantAllowance"},
+			accept:    false,
+		},


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.

0Tech · 2021-11-17T10:39:45Z

x/feegrant/filtered_fee_test.go

+				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
+				}


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().

0Tech · 2021-11-17T10:40:52Z

x/feegrant/filtered_fee_test.go

+				require.NoError(t, err)
+				feeAllowance, err := newAllowance.(*feegrant.AllowedMsgAllowance).GetAllowance()
+				require.NoError(t, err)
+				assert.Equal(t, tc.remains, feeAllowance.(*feegrant.BasicAllowance).SpendLimit)


Finally, the assert against SpendLimit has been made.

atheeshp · 2021-11-22T09:49:05Z

The idea sounds fine to me, what are your thoughts on this @anilcse , @AmauryM , @blushi

alexanderbez · 2021-11-30T15:11:16Z

Is spend_limit meant to be mutated though?

0Tech · 2021-12-01T02:14:23Z

Is spend_limit meant to be mutated though?

According to the documents and implementation, yes, it has to be mutated.

cosmos-sdk/proto/cosmos/feegrant/v1beta1/feegrant.proto

Lines 19 to 22 in 0a3660d

    
           // spend_limit specifies the maximum amount of tokens that can be spent 
        
           // by this allowance and will be updated as tokens are spent. If it is 
        
           // empty, there is no spend limit and any amount of coins can be spent. 
        
           repeated cosmos.base.v1beta1.Coin spend_limit = 1

cosmos-sdk/x/feegrant/basic_fee.go

Lines 20 to 36 in 3582cce

    
           func (a *BasicAllowance) Accept(ctx sdk.Context, fee sdk.Coins, _ []sdk.Msg) (bool, error) { 
        
           	if a.Expiration != nil && a.Expiration.Before(ctx.BlockTime()) { 
        
           		return true, sdkerrors.Wrap(ErrFeeLimitExpired, "basic allowance") 
        
           	} 
        
           	if a.SpendLimit != nil { 
        
           		left, invalid := a.SpendLimit.SafeSub(fee) 
        
           		if invalid { 
        
           			return false, sdkerrors.Wrap(ErrFeeLimitExceeded, "basic allowance") 
        
           		} 
        
           		a.SpendLimit = left 
        
           		return left.IsZero(), nil 
        
           	} 
        
           	return false, nil 
        
           }

The modification I'm adding is just saving the change into the state correctly.

alexanderbez

Changes LGTM, but I just don't have enough context about this module to give it an ACK. @AmauryM who designed or implemented the feegrant module?

amaury1093

Thanks @0Tech! the fix in x/feegrant/filtered_fee.go LGTM

Could you clean up the tests a little bit? they are really hard to follow

CHANGELOG.md

x/feegrant/filtered_fee_test.go

0Tech · 2021-12-20T05:31:28Z

Thanks @0Tech! the fix in x/feegrant/filtered_fee.go LGTM

Could you clean up the tests a little bit? they are really hard to follow

Thank you for your kind feedback @AmauryM! I have applied all the feedback you made 😄

amaury1093

actually, let's make the test simpler before merging.

x/feegrant/filtered_fee_test.go

amaury1093 · 2022-01-03T14:21:40Z

x/feegrant/filtered_fee_test.go

+			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)
+			}


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) }

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.

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.

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.

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.

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.

atheeshp

looks good, few nits!

x/feegrant/filtered_fee_test.go

CHANGELOG.md

atheeshp · 2022-01-04T10:44:36Z

x/feegrant/filtered_fee_test.go

+	}{
+		"msg contained": {
+			allowance: &feegrant.BasicAllowance{},
+			msgs:      []string{sdk.MsgTypeURL(&call)},


Suggested change

msgs: []string{sdk.MsgTypeURL(&call)},

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

we can remove call if this works.

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.

atheeshp · 2022-01-04T11:09:49Z

x/feegrant/filtered_fee.go

+		a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message))
+		if err != nil {
+			return false, sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance)
+		}


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

atheeshp · 2022-01-04T11:48:32Z

x/feegrant/filtered_fee_test.go

+			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)
+			}


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.

atheeshp · 2022-01-04T11:50:26Z

x/feegrant/filtered_fee_test.go

+			grant, err := feegrant.NewGrant(granter, grantee, allowance)
+			require.NoError(t, err)


Suggested change

grant, err := feegrant.NewGrant(granter, grantee, allowance)

require.NoError(t, err)

Co-authored-by: atheeshp <[email protected]>

fix: update allowance inside AllowedMsgAllowance

02c8e13

github-actions bot added the C:x/feegrant label Nov 17, 2021

0Tech mentioned this pull request Nov 17, 2021

fix: update allowance inside AllowedMsgAllowance Finschia/finschia-sdk#383

Merged

9 tasks

0Tech commented Nov 17, 2021

View reviewed changes

0Tech marked this pull request as ready for review November 17, 2021 10:42

0Tech requested review from aaronc and alexanderbez as code owners November 17, 2021 10:42

atheeshp self-assigned this Nov 19, 2021

amaury1093 self-assigned this Nov 25, 2021

alexanderbez reviewed Dec 1, 2021

View reviewed changes

amaury1093 reviewed Dec 13, 2021

View reviewed changes

0Tech added 5 commits December 20, 2021 05:24

move the log entry into state-machine breaking section

408f75c

remove redundant func

4568717

use a primitive msg for the test case

52eed35

remove hardcoded strings

143c5d9

remove the noise

cb8c84e

amaury1093 approved these changes Jan 3, 2022

View reviewed changes

Merge branch 'master' into fix/feegrant_allowedmsg

4206827

amaury1093 suggested changes Jan 3, 2022

View reviewed changes

0Tech added 2 commits January 4, 2022 11:49

remove the unused field

de4745d

fix newgrant into loadedgrant and add comments

092d219

atheeshp approved these changes Jan 4, 2022

View reviewed changes

0Tech and others added 4 commits January 4, 2022 21:14

Update x/feegrant/filtered_fee_test.go

0ad23de

Co-authored-by: atheeshp <[email protected]>

add SetAllowance() method to AllowedMsgAllowance

8a556a6

fix comment

67a7064

Update CHANGELOG.md

999dcdd

Co-authored-by: atheeshp <[email protected]>

0Tech changed the title ~~fix: update allowance inside AllowedMsgAllowance~~ fix: fix bug when updating allowance inside AllowedMsgAllowance Jan 4, 2022

amaury1093 approved these changes Jan 4, 2022

View reviewed changes

amaury1093 and others added 3 commits January 4, 2022 14:04

Merge branch 'master' into fix/feegrant_allowedmsg

6008d98

Merge branch 'master' into fix/feegrant_allowedmsg

bf6645b

Merge branch 'master' into fix/feegrant_allowedmsg

6e84e7a

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 5, 2022

mergify bot merged commit 651ef9e into cosmos:master Jan 5, 2022

ryanchristo mentioned this pull request Jul 1, 2022

Add warning to feegrant documentation v0.45 and before #12419

Closed

4 tasks

assafmo mentioned this pull request May 11, 2023

Fix Infinite Feegrant Bug #16097

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

0Tech commented Nov 17, 2021 •

edited

Loading

0Tech left a comment

0Tech Nov 17, 2021

0Tech Nov 17, 2021

0Tech Nov 17, 2021

0Tech Nov 17, 2021

0Tech Nov 17, 2021

0Tech Nov 17, 2021

atheeshp commented Nov 22, 2021

alexanderbez commented Nov 30, 2021

0Tech commented Dec 1, 2021

alexanderbez left a comment

amaury1093 left a comment •

edited

Loading

0Tech commented Dec 20, 2021

amaury1093 left a comment

amaury1093 Jan 3, 2022 •

edited

Loading

0Tech Jan 4, 2022 •

edited

Loading

0Tech Jan 4, 2022

amaury1093 Jan 4, 2022

atheeshp Jan 4, 2022

0Tech Jan 4, 2022

atheeshp left a comment

atheeshp Jan 4, 2022

0Tech Jan 4, 2022

atheeshp Jan 4, 2022

atheeshp Jan 4, 2022

atheeshp Jan 4, 2022

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

		grant, err := feegrant.NewGrant(granter, grantee, allowance)
		require.NoError(t, err)

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

fix: fix bug when updating allowance inside AllowedMsgAllowance #10564

Conversation

0Tech commented Nov 17, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

0Tech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atheeshp commented Nov 22, 2021

alexanderbez commented Nov 30, 2021

0Tech commented Dec 1, 2021

alexanderbez left a comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

0Tech commented Dec 20, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

0Tech Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atheeshp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0Tech commented Nov 17, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading

amaury1093 Jan 3, 2022 •

edited

Loading

0Tech Jan 4, 2022 •

edited

Loading