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

Rename AccessControl's roles to actions #985

Closed
nventuro opened this issue Nov 3, 2021 · 2 comments · Fixed by #1116
Closed

Rename AccessControl's roles to actions #985

nventuro opened this issue Nov 3, 2021 · 2 comments · Fixed by #1116

Comments

@nventuro
Copy link
Contributor

nventuro commented Nov 3, 2021

OZ's AccessControl is role-based: it can be used to designate roles that can perform multiple actions.

Due to a number of reasons, including bytecode size restrictions, our own authorization mechanism maps external functions to these 'roles' on a 1:1 basis, so we renamed these to 'actions' for more clarity. Instead of an account having a role that can let it perform an action, it simply can perform the action - roles are superfluous.

The initial version of the Authorizer was simply built on top of AccessControl as it didn't add much, but now that we're more thoroughly changing its internals (see #588 and #980) I suggest we drop OZ's version for a more customized one that supports our advanced features natively, and uses our own naming scheme.

Suggested changes:

  • hasRole -> canPerform (we already use this getter in all contracts)
  • grantRole -> grantPermission
  • revokeRole -> revokePermission

The only bit that doesn't quite fit this model is that of admins (to authorize grant and revoke), likely because those are much closer to being roles.

We could support these by having each action not only keep track of who can perform it, but also of a list of its admins, who can freely grant and remove permission both locally and globally. This might mean adding isAdminFor, which also helps draw the distinction between admin and non-admin 'roles'.

@Ramarti
Copy link
Contributor

Ramarti commented Nov 3, 2021

I'd propose just moving the functionality from AccessControl to the Authorizer and remove redundancies.
+1 to having 2 methods to check if Admin or specific permission

@nventuro
Copy link
Contributor Author

nventuro commented Nov 4, 2021

For reference, see this review comment about how lacking isAdminFor might look a bit weird.

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 a pull request may close this issue.

2 participants