Add AccessManagerEnumerable

[ernestognw]

Testing how an AccessManagerEnumerable would look like

PR Checklist

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 6d54710

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

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

[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

[james-toussaint]

Suggested change

	bool granted = super._grantRole(roleId, account, grantDelay, executionDelay);
	if (granted) {
	_roleMembers[roleId].add(account);
	}
	return granted;
	}
	/// @dev See {AccessManager-_revokeRole}. Removes the account from the role members set.
	function _revokeRole(uint64 roleId, address account) internal virtual override returns (bool) {
	bool revoked = super._revokeRole(roleId, account);
	if (revoked) {
	_roleMembers[roleId].remove(account);
	}
	return revoked;
	return super._grantRole(roleId, account, grantDelay, executionDelay) && _roleMembers[roleId].add(account);
	}
	/// @dev See {AccessManager-_revokeRole}. Removes the account from the role members set.
	function _revokeRole(uint64 roleId, address account) internal virtual override returns (bool) {
	return super._revokeRole(roleId, account) && _roleMembers[roleId].remove(account);

?

[ernestognw]

The purpose of checking the return booleans is to avoid reading the storage of the enumerable set. Although internally it checks contains(_roleMembers[roleId], account), that requires warming up the slot, which is another 2100 gas units. I would keep the if (granted) and if (revoked) blocks.