-
Notifications
You must be signed in to change notification settings - Fork 36
feat : more tracking in AccessManagerEnumerable #1063
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: dev
Are you sure you want to change the base?
Changes from 6 commits
d396a10
d7c0885
59d3875
9ed9dc8
444b0d2
865a591
2b57bff
6b7d84e
9856bbc
79beb08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,16 +12,48 @@ import {IAccessManagerEnumerable} from 'src/access/interfaces/IAccessManagerEnum | |||||||||||||||||||||||||
| contract AccessManagerEnumerable is AccessManager, IAccessManagerEnumerable { | ||||||||||||||||||||||||||
| using EnumerableSet for EnumerableSet.AddressSet; | ||||||||||||||||||||||||||
| using EnumerableSet for EnumerableSet.Bytes32Set; | ||||||||||||||||||||||||||
| using EnumerableSet for EnumerableSet.UintSet; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Set of all role identifiers. | ||||||||||||||||||||||||||
| EnumerableSet.UintSet private _rolesSet; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Map of role identifiers to their respective member sets. | ||||||||||||||||||||||||||
| mapping(uint64 roleId => EnumerableSet.AddressSet) private _roleMembers; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Map of role identifiers to their respective target contract addresses. | ||||||||||||||||||||||||||
| mapping(uint64 roleId => EnumerableSet.AddressSet) private _roleTargets; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Map of target contract addresses to their current role identifiers. | ||||||||||||||||||||||||||
| mapping(address target => uint64 roleId) private _targetRoles; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Map of role identifiers and target contract addresses to their respective set of function selectors. | ||||||||||||||||||||||||||
| mapping(uint64 roleId => mapping(address target => EnumerableSet.Bytes32Set)) | ||||||||||||||||||||||||||
| private _roleTargetFunctions; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Constructor. | ||||||||||||||||||||||||||
| /// @param initialAdmin_ The address of the initial admin. | ||||||||||||||||||||||||||
| constructor(address initialAdmin_) AccessManager(initialAdmin_) {} | ||||||||||||||||||||||||||
Kogaroshi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRole(uint256 index) external view returns (uint64) { | ||||||||||||||||||||||||||
| return uint64(_rolesSet.at(index)); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we do safecast here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the RoleId that is returned here is always passed as uint64 before being added in the Set, so I don't see a scenario were we can overflow the uint64 when fetching it back. |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleCount() external view returns (uint256) { | ||||||||||||||||||||||||||
| return _rolesSet.length(); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoles(uint256 start, uint256 end) external view returns (uint64[] memory) { | ||||||||||||||||||||||||||
| uint256[] memory listedRoles = _rolesSet.values(start, end); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we not just return
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if yes, do we need this fn anymore?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||
| uint64[] memory roles; | ||||||||||||||||||||||||||
| assembly ('memory-safe') { | ||||||||||||||||||||||||||
| roles := listedRoles | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return roles; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleMember(uint64 roleId, uint256 index) external view returns (address) { | ||||||||||||||||||||||||||
| return _roleMembers[roleId].at(index); | ||||||||||||||||||||||||||
|
|
@@ -41,6 +73,25 @@ contract AccessManagerEnumerable is AccessManager, IAccessManagerEnumerable { | |||||||||||||||||||||||||
| return _roleMembers[roleId].values(start, end); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleTarget(uint64 roleId, uint256 index) external view returns (address) { | ||||||||||||||||||||||||||
| return _roleTargets[roleId].at(index); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleTargetCount(uint64 roleId) external view returns (uint256) { | ||||||||||||||||||||||||||
| return _roleTargets[roleId].length(); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleTargets( | ||||||||||||||||||||||||||
| uint64 roleId, | ||||||||||||||||||||||||||
| uint256 start, | ||||||||||||||||||||||||||
| uint256 end | ||||||||||||||||||||||||||
| ) external view returns (address[] memory) { | ||||||||||||||||||||||||||
| return _roleTargets[roleId].values(start, end); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @inheritdoc IAccessManagerEnumerable | ||||||||||||||||||||||||||
| function getRoleTargetFunction( | ||||||||||||||||||||||||||
| uint64 roleId, | ||||||||||||||||||||||||||
|
|
@@ -73,7 +124,39 @@ contract AccessManagerEnumerable is AccessManager, IAccessManagerEnumerable { | |||||||||||||||||||||||||
| return targetFunctionSelectors; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Override AccessManager `_grantRole` function to track role members. | ||||||||||||||||||||||||||
| /// @dev Tracks all role identifiers when a new role is created. | ||||||||||||||||||||||||||
| function _trackRole(uint64 roleId) internal { | ||||||||||||||||||||||||||
| _rolesSet.add(uint256(roleId)); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
Kogaroshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Tracks all targets where a selector was assigned to a role. | ||||||||||||||||||||||||||
| function _trackRoleTarget(uint64 roleId, address target) internal { | ||||||||||||||||||||||||||
| uint64 oldRole = _targetRoles[target]; | ||||||||||||||||||||||||||
| if (oldRole == roleId) { | ||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if (oldRole != ADMIN_ROLE && _roleTargetFunctions[oldRole][target].length() == 0) { | ||||||||||||||||||||||||||
| _roleTargets[oldRole].remove(target); | ||||||||||||||||||||||||||
Kogaroshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if (roleId != ADMIN_ROLE) { | ||||||||||||||||||||||||||
| _roleTargets[roleId].add(target); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| _targetRoles[target] = roleId; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Override AccessManager `_setRoleAdmin` function to track created roles. | ||||||||||||||||||||||||||
| function _setRoleAdmin(uint64 roleId, uint64 admin) internal override { | ||||||||||||||||||||||||||
| _trackRole(roleId); | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, but we do not know which role is the admin role of a given role.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aave-v4/src/dependencies/openzeppelin/AccessManager.sol Lines 81 to 90 in 9ed9dc8
=> can fetch with :
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so we will have all roles stored in a single mapping, no matter whether they are admin/guardian roles or not. Then, it's only possible to detect if the role is admin/guardian role by fetching all existing roles and calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's currently the only way. |
||||||||||||||||||||||||||
| super._setRoleAdmin(roleId, admin); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Override AccessManager `_setRoleGuardian` function to track created roles. | ||||||||||||||||||||||||||
| function _setRoleGuardian(uint64 roleId, uint64 guardian) internal override { | ||||||||||||||||||||||||||
| _trackRole(roleId); | ||||||||||||||||||||||||||
| super._setRoleGuardian(roleId, guardian); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Override AccessManager `_grantRole` function to track roles' membership. | ||||||||||||||||||||||||||
| function _grantRole( | ||||||||||||||||||||||||||
| uint64 roleId, | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will track the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you are right. |
||||||||||||||||||||||||||
| address account, | ||||||||||||||||||||||||||
|
|
@@ -82,12 +165,13 @@ contract AccessManagerEnumerable is AccessManager, IAccessManagerEnumerable { | |||||||||||||||||||||||||
| ) internal override returns (bool) { | ||||||||||||||||||||||||||
| bool granted = super._grantRole(roleId, account, grantDelay, executionDelay); | ||||||||||||||||||||||||||
| if (granted) { | ||||||||||||||||||||||||||
| _trackRole(roleId); | ||||||||||||||||||||||||||
| _roleMembers[roleId].add(account); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return granted; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// @dev Override AccessManager `_revokeRole` function to remove from tracked role members. | ||||||||||||||||||||||||||
| /// @dev Override AccessManager `_revokeRole` function to remove from tracked roles' membership. | ||||||||||||||||||||||||||
| function _revokeRole(uint64 roleId, address account) internal override returns (bool) { | ||||||||||||||||||||||||||
Kogaroshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| bool revoked = super._revokeRole(roleId, account); | ||||||||||||||||||||||||||
| if (revoked) { | ||||||||||||||||||||||||||
|
|
@@ -110,5 +194,7 @@ contract AccessManagerEnumerable is AccessManager, IAccessManagerEnumerable { | |||||||||||||||||||||||||
| if (roleId != ADMIN_ROLE) { | ||||||||||||||||||||||||||
| _roleTargetFunctions[roleId][target].add(bytes32(selector)); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // also track the target under the role (will be added if not already present) | ||||||||||||||||||||||||||
| _trackRoleTarget(roleId, target); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,22 @@ import {IAccessManager} from 'src/dependencies/openzeppelin/IAccessManager.sol'; | |
| /// @author Aave Labs | ||
| /// @notice Interface for AccessManagerEnumerable extension. | ||
| interface IAccessManagerEnumerable is IAccessManager { | ||
| /// @notice Returns the indentifier of the role at a specified index. | ||
Kogaroshi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// @param index The index in the role list. | ||
| /// @return The identifier of the role. | ||
| function getRole(uint256 index) external view returns (uint64); | ||
|
|
||
| /// @notice Returns the total number of existing roles. | ||
| /// @dev Does not account for the built-in `PUBLIC_ROLE` role. | ||
| /// @return The number of roles. | ||
| function getRoleCount() external view returns (uint256); | ||
|
|
||
| /// @notice Returns the list of role identifiers between the specified indexes. | ||
| /// @param start The starting index for the role list. | ||
| /// @param end The ending index for the role list. | ||
| /// @return The list of role identifiers. | ||
| function getRoles(uint256 start, uint256 end) external view returns (uint64[] memory); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the main motivation that this just comes for free with the data structure? I imagine that it will be used either to get a specific role or all roles. What if we just made these private variables public and just expose the simple getters like at an index, or all the roles? Then integrators can just do this kind of access on their own
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could remove given that we have getters for items and length, but I'm fine adding this getter for ease of integration (the set structure provides a function to do it and makes sense to make integrators' lives easier) |
||
|
|
||
| /// @notice Returns the address of the role member at a specified index. | ||
| /// @param roleId The identifier of the role. | ||
| /// @param index The index in the role member list. | ||
|
|
@@ -19,7 +35,7 @@ interface IAccessManagerEnumerable is IAccessManager { | |
| /// @return The number of members for the role. | ||
| function getRoleMemberCount(uint64 roleId) external view returns (uint256); | ||
|
|
||
| /// @notice Returns the list of members for a specified role. | ||
| /// @notice Returns the list of members for a specified role between the specified indexes. | ||
| /// @param roleId The identifier of the role. | ||
| /// @param start The starting index for the member list. | ||
| /// @param end The ending index for the member list. | ||
|
|
@@ -30,6 +46,28 @@ interface IAccessManagerEnumerable is IAccessManager { | |
| uint256 end | ||
| ) external view returns (address[] memory); | ||
|
|
||
| /// @notice Returns the address of the target contract for a specified role and index. | ||
| /// @param roleId The identifier of the role. | ||
| /// @param index The index in the role target list. | ||
| /// @return The address of the target contract. | ||
| function getRoleTarget(uint64 roleId, uint256 index) external view returns (address); | ||
|
|
||
| /// @notice Returns the number of targets for a specified role. | ||
Kogaroshi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// @param roleId The identifier of the role. | ||
| /// @return The number of targets for the role. | ||
| function getRoleTargetCount(uint64 roleId) external view returns (uint256); | ||
|
|
||
| /// @notice Returns the list of targets for a specified role between the specified indexes. | ||
| /// @param roleId The identifier of the role. | ||
| /// @param start The starting index for the role target list. | ||
| /// @param end The ending index for the role target list. | ||
| /// @return The list of targets for the role. | ||
| function getRoleTargets( | ||
| uint64 roleId, | ||
| uint256 start, | ||
| uint256 end | ||
| ) external view returns (address[] memory); | ||
|
|
||
| /// @notice Returns the function selector assigned to a given role at the specified index. | ||
| /// @param roleId The identifier of the role. | ||
| /// @param target The address of the target contract. | ||
|
|
||
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.
consistency across codebase
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.
disagree i think this is needed here