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 allowed_msg uncapped spend limit bug #284

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Fix allowed_msg uncapped spend limit bug #284

merged 5 commits into from
Jun 13, 2023

Conversation

LCyson
Copy link

@LCyson LCyson commented Jun 13, 2023

Describe your changes and provide context

A grantee can use uncapped grant from a grantee when a grant is using AllowedMsgAllowance type, even with the SpendLimit specified. This issue is resulted from the types.Any Allowance object inside the AllowedMsgAllowance is not updated correctly.

This PR fixes this issue and adds according unit tests to the AllowedMsgAllowance.

Testing performed to validate your change

  • unit test
  • e2e test

@LCyson LCyson requested review from philipsu522 and yzang2019 June 13, 2023 06:07
Copy link
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

@LCyson
Copy link
Author

LCyson commented Jun 13, 2023

Why is this different than OSS? https://github.com/cosmos/cosmos-sdk/blob/main/x/feegrant/filtered_fee.go#L70-L87

OSS version has the same feegrant allowed_msg implementation as the one we have, so it's likely that the OSS version also has this bug

@LCyson LCyson force-pushed the fix-allowed-msg branch from ffebe54 to 670af4f Compare June 13, 2023 18:57
@LCyson
Copy link
Author

LCyson commented Jun 13, 2023

Apply the OSS fix: cosmos/cosmos-sdk#16097

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #284 (71bec5b) into main (3046382) will increase coverage by 0.04%.
The diff coverage is 37.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
+ Coverage   54.93%   54.98%   +0.04%     
==========================================
  Files         619      619              
  Lines       51509    51524      +15     
==========================================
+ Hits        28297    28329      +32     
+ Misses      21185    21162      -23     
- Partials     2027     2033       +6     
Impacted Files Coverage Δ
x/feegrant/filtered_fee.go 44.59% <37.50%> (+44.59%) ⬆️

... and 1 file with indirect coverage changes

@LCyson LCyson merged commit df2cf22 into main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants