Add AccessManagerEnumerable

[ernestognw]

Testing how an AccessManagerEnumerable would look like

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: d39400a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

Introduces a new abstract contract AccessManagerEnumerable that extends the AccessManager contract. The contract tracks role members by maintaining an EnumerableSet for each role. It overrides the _grantRole and _revokeRole internal methods to manage role member tracking. Two new public view functions are added: getRoleMembers to retrieve a paginated slice of addresses for a given role, and getRoleMemberCount to return the total member count for a role.

Suggested labels

ignore-changeset

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Add AccessManagerEnumerable' clearly and concisely summarizes the main change: introduction of a new contract extension.
Description check	✅ Passed	The description explains the purpose of the PR (testing a prototype) and references the PR checklist with pending items, which is related to the changeset.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04e683b and 4374b53.

📒 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 (where since > now) that are not yet effective
• hasRole() correctly returns false for these same accounts until the grant becomes effective
• _revokeRole() is properly synchronized and removes accounts from the enumerable set

This creates an API inconsistency: getRoleMembers() and hasRole() disagree on whether an account "has" a role during the grantDelay window. 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?

[ernestognw]

Related to #6091

[Amxx]

Are we interrested by having an enumeration of all the roles (that have at least one member) ?

Its nice to be able to list the memebers of a role, but you first need to know which role exist, don't you ?

[Amxx]

I don't think this should be internal virtual:

• I see little reason for anyone to override it (they could override _setTargetFunctionRole directly)
• if its called directly, it would possibly mess up stuff.

I would be in favor of either:

• inline it in side the _setTargetFunctionRole override (my prefered option)
• (alternativelly) make it a private function.

[Amxx]

Note that the comment:

 * Developers who wish to explicitly track {ADMIN_ROLE} can override this function. For example:
 *
 * ```solidity
 * function _updateRoleTargetFunction(address target, bytes4 selector, uint64 oldRoleId, uint64 newRoleId) internal virtual override {
 *     if (oldRoleId != 0) {
 *         _roleTargetFunctions[oldRoleId][target].remove(selector);
 *     }
 *     if (newRoleId != 0) {
 *         _roleTargetFunctions[newRoleId][target].add(selector);
 *     }
 * }
 * ```

assumes access to the _roleTargetFunctions storage mapping, which is private and not accessible in overrides

[Amxx]

Additionally, ADMIN_ROLE is equal to 0, so the proposed override would exactly match the current behavior of the function.

[Amxx]

@james-toussaint WDYT ?

[james-toussaint]

1. Your preferred option was what Ernest had in the first place before my comment, please have a look. I believe Natspec has not been properly updated though ("proposed override would exactly match the current behavior").
2. Private _roleTargetFunctions: good point 👍

[ernestognw]

1. Yes, the proposed override was wrong because the mapping is private anywa

Another option would be:

function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual override {
    uint64 oldRoleId = getTargetFunctionRole(target, selector);
    super._setTargetFunctionRole(target, selector, roleId);
    _updateRoleTargetFunction(target, selector, oldRoleId, roleId);
    if (oldRoleId != ADMIN_ROLE) _removeTargetFunction(target, selector, oldRoleId);
    if (roleId != ADMIN_ROLE) _addTargetFunction(target, selector, roleId);
}

function _removeTargetFunction(address target, bytes4 selector, uint64 roleId) internal virtual {
    _roleTargetFunctions[roleId][target].remove(selector);
}

function _addTargetFunction(address target, bytes4 selector, uint64 roleId) internal virtual {
    _roleTargetFunctions[roleId][target].add(selector);
}

So that users can override _setTargetFunctionRole and do:

function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual override {
  uint64 oldRoleId = getTargetFunctionRole(target, selector);
  super._setTargetFunctionRole(target, selector, roleId);
  if(oldRoleId == ADMIN_ROLE) _removeTargetFunction(target, selector, oldRoleId);
  if(roleId == ADMIN_ROLE) _addTargetFunction(target, selector, roleId);
}

But @Amxx's argument is that having these functions internal virtual will really mess things up if someone uses them directly. I somewhat agree

[Amxx]

I think here we should also run all the other "shouldBehaliveLikeXxx" to prove that the enumerable extension doesn't break any of the other expected behaviors.

[Amxx]

Given that we could not reach consensus on what exactly to enumerate (include roleIds that have more than one member ?), and that the main user of this contract mentionned they would NOT use an implementation if we provided one, it was decided that we should probably not provide that as part of the library, and commit to supporting it.

Still, because it works, and it might be valuable to some users, we should keep this code available to our users through the docs, similarly to ERC4626Fees.

A follow up PR should fix the testing that we have in commit 0b981fd

[ernestognw]

I changed the title of the new docs section, otherwise it would collide with other section that has the same name under Access Control.

LGTM