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

x/authz module spec is not up-to-date with implementation #11566

Closed
4 tasks
dalmirel opened this issue Apr 7, 2022 · 2 comments · Fixed by #11671
Closed
4 tasks

x/authz module spec is not up-to-date with implementation #11566

dalmirel opened this issue Apr 7, 2022 · 2 comments · Fixed by #11671
Assignees
Labels
C:x/authz T:Docs Changes and features related to documentation.

Comments

@dalmirel
Copy link

dalmirel commented Apr 7, 2022

Summary of Bug

During the specification and code inspection of authz module (as a part of audit process that includes model-based testing) I have found that several specification markdown files are out of date or missing (in my opinion) some important info on authorization feature and implementation itself.
The following are just recommendations, so please if I'm missing some context needed for better understanding of authz module, feel free to tell me so.

01_concepts.md:

  • I think it would be useful to note the version of Cosmos SDK version that introduced Authz module.
  • Gas handling: 20 Gas is incurred also when removing grantQueueKey from Grant Queue, as I saw here: consider adding this info here in the spec.
  • Staking Authorizations: additional explanation for allow and deny lists would be helpful here.
    -I have found an explanation on issue Authorization logic of staking module: denyList never used? #11391 reported: “If deny list is provided, it should be possible to use any validator not on the deny list” and I think adding this to spec would explain the idea.
    -Also, consider changing and to or in: "Msg takes an AllowList and a DenyList " since it is not possible to create staking authorization with both lists.

02_state.md:

  • Grant Queue - second index in the store is not documented and explained in spec. I believe info on usage in Begin blocker would be useful to add and a code snippet.
  • Grant Code snippets references old code, where expiration was required, now we have: (gogoproto.nullable) = true with optional expirations feature implemented.

03_messages.md

  • Optional expiration time for grants: recommendation to add info here about expiration time for grant and block time, since grant will not be created if time is in the “past”, but will be created if no expiration time is provided (optional expiration time)

ADR30:

  • SpendLimit is explained here in a way, that one can conclude that is possible to have Send Authorization with no spending limits defined, even though the actual implementation is that SpendLimit in Send Authorizations can not be nil and limit must be defined.

Version

master (commit: 8800d2e)
auditing is performed for tag: v0.46.0-alpha4 (commit: 354faa8)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added T:Docs Changes and features related to documentation. C:x/authz labels Apr 7, 2022
@atheeshp
Copy link
Contributor

SpendLimit is explained here in a way, that one can conclude that is possible to have Send Authorization with no spending limits defined, even though the actual implementation is that SpendLimit in Send Authorizations can not be nil and limit must be defined.

I think this should be implemented in a way that SpendLimit is optional as mentioned in the ADR, if not provided it should be limit less but the logic is not as documented.
cc: @anilcse @AmauryM @robert-zaremba

@atheeshp
Copy link
Contributor

atheeshp commented Apr 18, 2022

I think this should be implemented in a way that SpendLimit is optional as mentioned in the ADR, if not provided it should be limit less but the logic is not as documented. cc: @anilcse @AmauryM @robert-zaremba

after having a small discussion with @aleem1314 & @AmauryM we just need to modify the ADR here, if we need limit less bank authz we can use generic authz.

@atheeshp atheeshp self-assigned this Apr 18, 2022
@atheeshp atheeshp mentioned this issue Apr 18, 2022
19 tasks
@mergify mergify bot closed this as completed in #11671 Apr 20, 2022
mergify bot pushed a commit that referenced this issue Apr 20, 2022
## Description

Closes: #11566 



---

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

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/authz T:Docs Changes and features related to documentation.
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants