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

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

Closed
4 tasks
ryanchristo opened this issue Jul 1, 2022 · 5 comments
Closed
4 tasks

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

ryanchristo opened this issue Jul 1, 2022 · 5 comments
Labels
C:x/feegrant T:Docs Changes and features related to documentation.

Comments

@ryanchristo
Copy link
Contributor

Summary

The feegrant module was introduced in v0.44 with a bug when using AllowedMsgAllowance in which case the fee is not subtracted from the spend limit (#10563). This issue has been resolved in #10564 and the changes will be included in v0.46.

Applications using the feegrant module with v0.44 and v0.45 are still affected by the bug meaning spend limit is not updated when using AllowedMsgAllowance and therefore spend limit only limits spending on a per transaction basis and not overall.

This bug should be clearly documented with a warning in the v0.44 and v0.45 feegrant documentation.

Version

v0.44 and v0.45


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ryanchristo ryanchristo added T:Docs Changes and features related to documentation. C:x/feegrant labels Jul 1, 2022
@paul121
Copy link
Contributor

paul121 commented Jul 1, 2022

This bug should be clearly documented with a warning in the v0.44 and v0.45 feegrant documentation.

v0.44 and v0.45 actually don't include documentation for the AllowedMsgAllowanceallowance type (#11346). This was fixed with #11344 for v0.46 but should be included for these previous versions as well, with additional documentation and warning for the bug.

Considering that v0.46 is not yet released, should the latest version of the docs include a warning describing that previous versions are affected by the bug and will behave differently?

@paul121
Copy link
Contributor

paul121 commented Jul 1, 2022

Considering that AllowedMsgAllowance can be one of either BasicAllowance or PeriodicAllowance - as per current docs:

## AllowedMsgAllowance
`AllowedMsgAllowance` is a fee allowance, it can be any of `BasicFeeAllowance`, `PeriodicAllowance` but restricted only to the allowed messages mentioned by the granter.
+++ https://github.com/cosmos/cosmos-sdk/blob/v0.46.0-rc1/proto/cosmos/feegrant/v1beta1/feegrant.proto#L56-L66
* `allowance` is either `BasicAllowance` or `PeriodicAllowance`.
* `allowed_messages` is array of messages allowed to execute the given allowance.

I believe an AllowedMsgAllowance using PeriodicAllowance would have an additional bug that the period_can_spend value would not be updated either. This is effectively the same bug, meaning the spend_limit and period_spend_limit only limits spending on a per-transaction basis and not overall.

So perhaps this warning message can be generalized a bit, maybe something like:

 ## AllowedMsgAllowance 
  
 `AllowedMsgAllowance` is a fee allowance, it can be any of `BasicFeeAllowance`, `PeriodicAllowance` but restricted only to the allowed messages mentioned by the granter.
 
 NOTE: A bug exists in v0.44 and v0.45 where `AllowedMsgAllowance` does not update the state of the `allowance` used. The result of this is that when a `spend_limit` is configured, `AllowedMsgAllowance` will only limit spending on a per transaction basis, and not limit the total number of coins the `grantee` can use from the `granter. More specifically the `spend_limit` will not be updated in neither the `BasicFeeAllowance` nor the `PeriodicAllowance` allowance, and the `period_can_spend` will not be updated in the `PeriodicAllowance`.

Are there any examples of this type of warning in the sdk documentation? If so I'm happy to start a PR for this.

@paul121
Copy link
Contributor

paul121 commented Jul 1, 2022

I've just opened an issue and PR for two related bugs in feegrant docs:

#12421
#12422

@alexanderbez
Copy link
Contributor

@AmauryM or @blushi would you by any chance have the ability to look into this?

@tac0turtle
Copy link
Member

45 and before is no longer maintained we can close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/feegrant T:Docs Changes and features related to documentation.
Projects
None yet
Development

No branches or pull requests

4 participants