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

Remove enumerable from AccessControl and add an AccessControlEnumerable extension #2512

Merged
merged 20 commits into from Feb 19, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 5, 2021

Fixes #2139

EnumerableSet and EnumerableMap heavily rely on sstore & sload, which are very expensive operations. At the same time, the Enumerable aspect can be tracked offchain using tools like thegraph.

Developer should have the choice between having this enumerable functionality (and pay the gas price), or drop it if onchain access to these information is sufficient.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Feb 10, 2021

Before making this decision we need to know what is the effect of having enumerability in AccessControl. What operations are affected and by how much.

For example, it seems that hasRole using EnumerableMap is only doing a single sload, which is the optimum.

Granting and revoking roles will be a lot more expensive with enumerability but these operations are not done often so it may be okay if they are not optimized for that.

Lastly, we also need to know the effect on contract size.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 12, 2021

Costs (from eth-gas-reporter):

Version deploy mock grant role revoke role
master 580,730 84,860 24,827
non-enumerable 376,083 46,762 21,027

part of the saving for the deployment is from the smalest bytecode, and part is from granting role to msg.sender in the constructor

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 17, 2021

Latest commit fixes #2139

@frangio frangio self-requested a review February 17, 2021 21:36
contracts/access/AccessControlEnumerable.sol Outdated Show resolved Hide resolved
contracts/access/AccessControlEnumerable2.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
frangio
frangio previously approved these changes Feb 19, 2021
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.

Looks good! I left an improvement for the docs.

We also need to add {{AccessControlEnumerable}} in access/README.adoc.

contracts/access/AccessControl.sol Outdated Show resolved Hide resolved
@frangio frangio marked this pull request as ready for review February 19, 2021 17:25
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
@frangio frangio self-requested a review February 19, 2021 17:27
frangio
frangio previously approved these changes Feb 19, 2021
@frangio frangio enabled auto-merge (squash) February 19, 2021 17:27
@frangio frangio merged commit e341bdc into OpenZeppelin:master Feb 19, 2021
@Amxx Amxx deleted the feature/AccessControlLight branch February 19, 2021 18:20
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.

Query AccessControl roles per account
2 participants