feat: access control contract #1758
Conversation
| bytes32 internal constant OWNER_ROLE = keccak256("OWNER"); | ||
| bytes32 internal constant QUORUM_OWNER_SEED = keccak256("QUORUM_OWNER"); | ||
|
|
||
| function QUORUM_OWNER_ROLE(uint64 quorumId) internal pure returns (bytes32) { |
There was a problem hiding this comment.
how should this be used? Is the idea that the caller would call
EigenDAAccessControl.setupRole(AccessControlConstants.QUORUM_OWNER_ROLE(quorumId))
? Or how else is this supposed to be used? should we add a quorum_setter in EigenDAAccessControl already?
There was a problem hiding this comment.
This function defines a protocol in which the role identifier for a quorum owner for a particular quorum ID is generated. It can be used in contracts, tests, and production as a library function. Your example is one use case, but it would also be necessary to have this kind of function for contracts to efficiently query for quorum owners.
There was a problem hiding this comment.
This intended use case should probably be documented in the contract then.
contracts/src/core/libraries/v3/access-control/AccessControlConstants.sol
Show resolved
Hide resolved
| _grantRole(AccessControlConstants.OWNER_ROLE, owner); | ||
| } | ||
|
|
||
| function setupRole(bytes32 role, address account) external { |
There was a problem hiding this comment.
| function setupRole(bytes32 role, address account) external { | |
| function grantRole(bytes32 role, address account) external { |
There was a problem hiding this comment.
think renaming to match the OZ function makes more sense?
There was a problem hiding this comment.
In this case, no because then we would be overloading the existing grantRole function inherited from AccessControl. This function is meant to use the DEFAULT_ADMIN_ROLE defined in the AccessControl contract which has ownership over all roles by default, but I leave this role unused.
There was a problem hiding this comment.
I'm already confused. We can't overwrite that function because... its used? or because of some other reason AND we should make sure that nobody ever uses the grantRole function which is unfortunately exposed because of inheritance?
All this is the kind of stuff that should be documented...
There was a problem hiding this comment.
I decided to simply just use the DEFAULT_ADMIN_ROLE to use the paradigm that openzeppelin intends, which is for their defined DEFAULT_ADMIN_ROLE to be able to grant roles. And so now the contract is basically just OZ's AccessControl with a constructor.
contracts/src/core/libraries/v3/access-control/AccessControlConstants.sol
Show resolved
Hide resolved
|
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
Why are these changes needed?
Adds a relatively vanilla access control contract to the eigenDA contracts, and integrates it into the Address Directory's access management.
Checks