Hi all, we are proposing a Role-based ACL EIP
Hi, the link that you included points to a page that doesn’t exist. Would you mind updating it (if your PR hasn’t been merged yet, I think that a link to it might be ok)?
Good point. I updated the links
There is a “core” interface that is compatible with OpenZeppelin’s AccessControl, but then the events are incompatible. Why is that?
What is the motivation to add bytes data arguments to each function? This isn’t explained.
The behavior of functions isn’t specified.
In my opinion this EIP is in a state where it shouldn’t even be accepted as a Draft.
GM @frangio thank you for the comment
There is a “core” interface that is compatible with OpenZeppelin’s
AccessControl, but then the events are incompatible. Why is that?
I assue you are referring to the ones in IERC_ACL_GENERAL
event RoleGranted(address indexed grantor, bytes32 indexed role, address indexed grantee, bytes _data);
event RoleRevoked(address indexed revoker, bytes32 indexed role, address indexed revokee, bytes _data);
These events helps indexers and off-chain users to get information about change of Roles.
Please note that the mandate in the specification is
- Compliant contracts MUST implement
IERC_ACL_CORE- It is RECOMMENDED for compliant contracts to implement the optional extension
IERC_ACL_GENERAL.
Which means OZ’s AccessControl.sol is considered compliant to this EIP (it complies with the MUST interfaces). It’s ok that a compliant ignore specs that’s marked as “RECOMMENDATION”.
The recommendations are left there so that future clients, e.g. MetaMask and future contracts, and start to implement better features provided by this new EIP
What is the motivation to add
bytes dataarguments to each function? This isn’t explained.
Thanks for the comment, I tried to explained that but maybe that’s not obvious enough. These extra bytes are meant to support EIP-5750 General Extensibility,
EIP-5982 Role-based Access Control
Rationale
…
2. The methods inIERC_ACL_GENERALconform to EIP-5750 to allow extension.
So they could support future expansions, for example:
castVoteWithReason and castVoteWithSig which can be supported without change of method if they have an extra bytes.transferm, ERC-721 and ERC-1155’s transferFrom has an extra bytes to support future extensions.Example of of possible future extension include:
bytes to supply a salt, see EIP-5732 and an Reference Implementation of CommitableERC721GovernerBravo.castVoteWithSig or ERC-2612: Permit Extension for EIP-20 Signed Approvalsfunction permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external
And since you () are the author of EIP-1271 and EIP-2612, I am hoping to put these examples hopefully resonate with these areas of your great efforts:
GovernerBravo.castVoteWithSig and ERC2612.permit, imaging in the future we need multi-sig to do threshold signing, they will need to create new methods. Using EIP-5750, we get the benefit of not needing to create new methods.The behavior of functions isn’t specified. In my opinion this EIP is in a state where it shouldn’t even be accepted as a Draft.
I agree with you we shall provide more details about behavior of functions as an EIP. I will add more details soon.
That said, this snapshot is to go for DRAFT status which is before REVIEW status. Hence we are putting here for early feedback, hopefully the name of these functions are largely self-explanatory. But I think based on the definition of DRAFT status in EIP-1 EIP Process Section, it should be ok for publication as Draft status,
@frangio, I greatly appreciate your feedback and looking for your advices in this EIP draft. 
If there is a goal of compatibility with OpenZeppelin, compatible events should be part of IERC_ACL_CORE.
We are definitely happy to add these events into IERC_ACL_CORE if getting your support, but if I may take sometime to clarify and make sure we are on the same page about the implications.
We want this new EIP to be compatible to OpenZeppelin, more specifically we are looking at IAccessControl.sol between OZ v4.4.1 - v.4.8.0
Therefore the methods and events of IERC_ACL_CORE needs to be a subset of AccessControl.sol
$$
S_{IERC_ACL_CORE} \subset S_{OZAcessControl.sol}
$$
The events of question are similar and inspired by openzeppelin-contracts/IAccessControl.sol at 8f8fd84f1e60426a5e785d6b5b2524938271bb05 · OpenZeppelin/openzeppelin-contracts · GitHub
But not entirely the same such as bytes and grantor to allow off-chain approval and extending behavior.
If OZ find these events critical and is interested in committing to support these new version of event definition and commit to change to new version, we are happy to consider moving such events to IERC_ACL_CORE to make them a mandatory.
My point is that if there is an interest in making this compatible with AccessControl from OpenZeppelin the event definitions should be changed to match that contract and moved into the core interface.
That is the purpose of this draft: to start early discussion of design choices like this.
OZ IAccessControl.sol v4.4.1-4.8.0
event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
EIP-5982 Draft I_ERC_GERNERAL
event RoleGranted(address indexed grantor, bytes32 indexed role, address indexed grantee, bytes _data);
The current snapshot EIP demonstrated a technical opinionated view that OZ’s current IAcessControl.sol 's events are not sufficiently designed to cover future cases, and hence, until other-wised convinced, this EIP made a design choice to go for different event definitions as demonstrated in its IERC_GENERAL.
Let me know which is the case:
I understand this might be a choice but I am questioning that choice: I believe function-only compatibility is too reduced a form of compatibility. Event compatibility is also important, otherwise the functions might as well be entirely different. Additionally, I don’t think forgoing event compatibility is justified in this case: personally, I doubt the usefulness of EIP-5750, but even if we take it as a given to be a good idea, I don’t see strong reasons for including it in the events, in fact that EIP doesn’t mention events.
Per private discussion with @frangio, we are align on the terminology of compliance. Going to (2), we are debating design choicesof
event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
Option 2. use new events
event RoleGranted(address indexed grantor, bytes32 indexed role, address indexed grantee, bytes _data);
IERC_ACL_GENERAL
Is this a good summary of our diverging preferences and pros and cons? @frangio
Something looks odd about that. In Option 1 you list “Cons: didn’t indicate who granted the role”, but the event includes a sender parameter which is in fact who granted the role, isn’t it?
You also say “didn’t add bytes for future extension”, but as I’ve expressed in the discussion for EIP-5750 I still don’t see any compelling reasons to include bytes data.
I’m biased but I don’t think these are strong enough reasons to break compatibility.
If the concern about the sender parameter is that it doesn’t account for the case where the grant was made via a signature instead of via a transaction (so msg.sender is some other account who just has a signed approval), I’d be open to repurpose the sender parameter and extend its meaning so that it isn’t necessarily the msg.sender and is whoever allowed the role grant through whatever means. I think that was the original intended meaning for the parameter anyway.
@frangio appreciate your comment.
With the trend of account abstraction / smart contract wallet, or permit-based actions, it will be more and more the case when sender is not the grantor. Just like ERC20 first start off with transfer(to, amount) assuming the transferer is always the sender, and then later shift to a model with approve + transferFrom(from, to, amount). I believe this is going to be a general trend. Therefore, I specifically propose this design choice of specifying a grantor.
–
I think we are on the same page: we both think sender is going to not aways be grantor. I propose to call it grantor, and I am also open to use the same format with existing OZ 4.4.1’s RoleGranted but repurpose sender to grantor.
EIP-5750 opens up all sorts of extensionability, e.g.
uint256[] extensionsLet’s continue the discussion on EIP-5750.
Is this a pure goal? Frankly speaking OZ’s RBAC is space inefficient for its purpose.
Esp. the bytes32 roles. Look at Solmate / Solady / Huffmate, that uses bitmapping for roles. Thus having 256 roles, which can be assumed enough for most cases.
Can this standard be generic enough to capture such cases? Or is that only OZ specific standard?
Standard is different from Implementation. Efficiency in this case is largely determined by the implementation.
I think the ERC-5982 is generic enough to support the bitmap roles, you could just ignore the first 24bits and only assume the input will be uint8 or bytes8.
The interface ERC-5982 only provides “hasRole”, “grantRole”, “revokeRole” but doesn’t care how it was implemented.
For example
interface IERC_ACL_CORE {
function hasRole(bytes32 role, address account) external view returns (bool);
function grantRole(bytes32 role, address account) external;
function revokeRole(bytes32 role, address account) external;
}
interface BitmapAuth is IERC_ACL_CORE {
mapping(address => bytes32) _roles;
function hasRole(bytes32 role, address account) external view returns (bool) {
require(role <= 0xffffffff);
return (_roles[account] >> role) & 1 != 0;
}
function grantRole(bytes32 role, address account) external {
require(role <= 0xffffffff);
_roles[account] |= bytes32(1 << role);
// emit
}
function revokeRole(bytes32 role, address account) external {
require(role <= 0xffffffff);
_roles[account] &= ~bytes32(1 << role);
// emit
}
}
Just like people can just assume there will be at most 10000 NFTs and use ERC721 with internally an array of 10000 rather than a map. This interface of ERC-5982 is generalized enough to be compatible with when people use uint8 bitmap roles.
I appreciate that you elaborated the mapping approach. 
Would you mind to adjust the wording in the EIP to reflect such option?
- A role in a compliant smart contract is represented in the format of
bytes32. It’s RECOMMENDED the value of such role is computed as akeccak256hash of a string of the role name, in this format:bytes32 role = keccak256("<role_name>"). such asbytes32 role = keccak256("MINTER").
e.g. to
- A role in a compliant smart contract is represented in the format of
bytes32. It’s RECOMMENDED the value of such role is computed as akeccak256hash of a string of the role name, in this format:bytes32 role = keccak256("<role_name>"). such asbytes32 role = keccak256("MINTER"). To support the bitmap based roles, it’s RECOMMENDED to ignore the first 24bits and only assume the input will be uint8 or bytes8.
Here is a reference implementation for “createRole”
mapping<bytes32/*role*/, bytes32/*adminOrRole*/> roleRegistry;
function createRole(bytes32 role, bytes32 adminOfRole, string name, string desc, string uri, bytes32 calldata _data) external {
roleRegistry[role] = adminOfRole;
// also store name, desc, uri in each own storage
}
function roleName(bytes32 role) returns (string) {
return roleNames[role];
}
You can also do ENS-based Role / NFT-based Role
function createRole(bytes32 role, bytes32 adminOfRole, string name, string desc, string uri, bytes32 calldata _data) external {
_mintNFT(role/*nftID*/, adminOfRole/*address of Owner / parentNFTId*/);
}