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

ADR 030: Authorization Module #7105

Merged
merged 23 commits into from
Jan 13, 2021
Merged

ADR 030: Authorization Module #7105

merged 23 commits into from
Jan 13, 2021

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Aug 19, 2020

ref: #7074
replaces #5235


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@aaronc aaronc mentioned this pull request Aug 19, 2020
4 tasks
@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #7105 (fdeaa27) into master (3257618) will decrease coverage by 7.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7105      +/-   ##
==========================================
- Coverage   61.91%   54.60%   -7.31%     
==========================================
  Files         609      547      -62     
  Lines       35101    37121    +2020     
==========================================
- Hits        21732    20270    -1462     
- Misses      11085    15192    +4107     
+ Partials     2284     1659     -625     
Impacted Files Coverage Δ
x/genutil/client/cli/validate_genesis.go 0.00% <0.00%> (-77.42%) ⬇️
x/staking/client/cli/query.go 0.00% <0.00%> (-73.28%) ⬇️
x/gov/client/cli/query.go 0.00% <0.00%> (-69.15%) ⬇️
crypto/keys/ed25519/ed25519.go 0.00% <0.00%> (-67.35%) ⬇️
x/upgrade/types/codec.go 40.00% <0.00%> (-60.00%) ⬇️
crypto/keys/secp256k1/secp256k1.go 32.72% <0.00%> (-54.07%) ⬇️
x/staking/types/codec.go 55.55% <0.00%> (-44.45%) ⬇️
x/gov/client/cli/tx.go 30.46% <0.00%> (-37.91%) ⬇️
x/auth/client/rest/query.go 4.93% <0.00%> (-33.90%) ⬇️
x/genutil/utils.go 25.00% <0.00%> (-32.90%) ⬇️
... and 721 more

@clevinson clevinson added the T: ADR An issue or PR relating to an architectural decision record label Aug 21, 2020
@clevinson clevinson added this to the v0.41 milestone Aug 21, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 6, 2020
@aaronc aaronc added pinned and removed stale labels Oct 6, 2020
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

I like the gist of this ADR 👍

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

docs/architecture/adr-030-msg-auth-module.md Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Jan 11, 2021

@alessio didn't mean to dismiss your approval - I was trying re-request reviews from our team. Can you re-approve? (Feel free to review the minor changes, but really not much has changed).

@aaronc
Copy link
Member Author

aaronc commented Jan 11, 2021

Can you re-review @robert-zaremba ? You have changes requested still.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Approving, but would love to see more details. I made few suggestions.

## Changelog

- 2019-11-06: Initial Draft
- 2020-10-12: Updated Draft
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's update a changelog - it's accepted now.

docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
// by this authorization 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.
SpendLimit sdk.Coins
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would make sense to add an account which is allowed to spend the coins. We can add it in pseudocode or a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain a little more what you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just thinking about having a more complete example, were we don't authorize everyone to spend from an account, but only one specific user.

docs/architecture/adr-030-authz-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-030-authz-module.md Show resolved Hide resolved
// ExecAuthorized attempts to execute the provided messages using
// authorizations granted to the grantee. Each message should have only
// one signer corresponding to the granter of the authorization.
rpc ExecAuthorized(MsgExecAuthorized) returns (MsgExecAuthorizedResponse)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you specify how this method will call other services? Let's use the SendAuthorization example. Will x/authz have clients to all other modules? How it will inform other module that a message is authorized?

I thought that DispatchActions will do that. But it seams that the DispatchActions is used for other modules to call x/authz.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we addressed this when we chatted right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I can add more details in other PR.

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 13, 2021
@mergify mergify bot merged commit 2a2c2ec into master Jan 13, 2021
@mergify mergify bot deleted the aaronc/adr-msg-auth branch January 13, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants