-
Notifications
You must be signed in to change notification settings - Fork 12.3k
Add AccessManagerEnumerable #6053
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
base: master
Are you sure you want to change the base?
Add AccessManagerEnumerable #6053
Conversation
🦋 Changeset detectedLatest commit: ece797a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces a new abstract contract Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/access/manager/extensions/AccessManagerEnumerable.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/crosschain/README.adoc:1-1
Timestamp: 2025-08-28T15:48:30.716Z
Learning: ernestognw prefers "Cross chain" without hyphenation rather than "Cross-chain" in documentation titles.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: halmos
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (2)
contracts/access/manager/extensions/AccessManagerEnumerable.sol (2)
25-31: The implementation is correct—all role revocation paths call_revokeRole.Verification confirms:
revokeRole()→ calls_revokeRole()✓renounceRole()→ calls_revokeRole()✓cancel()→ handles scheduled operation cancellation, not role revocation- No role expiration mechanism exists in the codebase
- The enumerable override properly guards set removal with the return value from
super._revokeRole()No desynchronization risk exists.
Likely an incorrect or invalid review comment.
12-23: Clarify intent: should enumerable set track scheduled or only effective role grants?The concern is partially valid. Analysis confirms:
getRoleMembers()includes accounts with scheduled grants (wheresince > now) that are not yet effectivehasRole()correctly returnsfalsefor these same accounts until the grant becomes effective_revokeRole()is properly synchronized and removes accounts from the enumerable setThis creates an API inconsistency:
getRoleMembers()andhasRole()disagree on whether an account "has" a role during thegrantDelaywindow. This appears intentional but should be clarified:
- Is
getRoleMembers()meant to enumerate all grant holders (scheduled or effective)?- Should the docs explicitly explain this distinction?
- Or should the enumerable set only track effective members?
contracts/access/manager/extensions/AccessManagerEnumerable.sol
Outdated
Show resolved
Hide resolved
contracts/access/manager/extensions/AccessManagerEnumerable.sol
Outdated
Show resolved
Hide resolved
contracts/access/manager/extensions/AccessManagerEnumerable.sol
Outdated
Show resolved
Hide resolved
|
Related to #6091 |
Testing how an AccessManagerEnumerable would look like
PR Checklist
npx changeset add)