-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
Codecov Report
@@ 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
|
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. |
There was a problem hiding this 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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@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). |
Can you re-review @robert-zaremba ? You have changes requested still. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
// 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 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
// 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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes