-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Remove enumerable from AccessControl and add an AccessControlEnumerable extension #2512
Conversation
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 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. |
Costs (from eth-gas-reporter):
part of the saving for the deployment is from the smalest bytecode, and part is from granting role to msg.sender in the constructor |
Latest commit fixes #2139 |
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
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! I left an improvement for the docs.
We also need to add {{AccessControlEnumerable}}
in access/README.adoc
.
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
…elin-contracts into feature/AccessControlLight
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