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

Revamped Access Control #2112

Merged
merged 25 commits into from
Mar 16, 2020
Merged

Revamped Access Control #2112

merged 25 commits into from
Mar 16, 2020

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Mar 9, 2020

This PR removes Roles and introduces AccessControl, its replacement. We will need to update the access mini readme and access control guides in a later PR.

Fixes #1772. Fixes #1602. Part of #2086. See this discussion on the forum for more context.

We're removing Roles to do away with the boilerplate associated with creating new roles, as well as the overhead introduced by the many external functions added by each. To achieve this purpose, roles are now referred to by a unique ID, and all functions are parametrized by this identifier.

We're also changing some of the rules regarding how roles are granted and revoked:

  • Roles cannot be tranferred
  • Each role has an associated admin role, and a role can only be granted or revoked by accounts that have it’s admin role
  • DEFAULT_ADMIN_ROLE is the default admin role for all roles
  • A role’s admin role can be changed via an internal function
  • The overhead is kept minimal: contracts that use AccessControl only add an external pure getter for each newly defined role

Note that this does not separate grant and revoke concerns: a role’s admin can perform both tasks. I haven’t found scenarios in which this distinction is important.

Finally, it should be noted that AccessControl can also be easily used as a standalone contract queried by multiple other contracts in a system by inheriting from it, setting up initial roles in a constructor and exposing _setRoleAdmin externally.

@nventuro nventuro requested a review from frangio March 9, 2020 16:00
@nventuro nventuro changed the title Access control Revamped Access Control Mar 9, 2020
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Lovely test suite! Great work.

contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
test/access/AccessControl.test.js Show resolved Hide resolved
test/access/AccessControl.test.js Outdated Show resolved Hide resolved
test/access/AccessControl.test.js Outdated Show resolved Hide resolved
test/access/AccessControl.test.js Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
@nventuro
Copy link
Contributor Author

I commented out the tests for expectEvent.not due to an issue with how semver works: test-environment doesn't pick up test-helpers v0.5.5-rc.0 as a valid version (it doesn't satisfy ^0.5.4), so the helpers are not configured and tests fail. Once we make the actual release we'll be able to uncomment them.

I manually changed the version on the package.json to make the semver check pass and verified that the tests indeed pass.

test/access/AccessControl.test.js Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Mar 13, 2020

Going through previous comments I found that the issue with using RoleRevoked for renounce was only partially resolved. I mentioned that the name may be misleading. Do you think it's fine?

I've been trying to think of alternatives anyway and actually revoked might be the best one.

@nventuro
Copy link
Contributor Author

nventuro commented Mar 13, 2020

I understood your previous comment as meaning that the third event argument would make RoleRevoked good enough for renounce. Is that not the case? If we were to make a change, I'd add a new event and emit both, instead of not emitting RoleRevoked.

frangio
frangio previously approved these changes Mar 16, 2020
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Ship it.

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 this pull request may close these issues.

Proposal to revamp Roles Make roles iterable.
2 participants