EIP-5982 Role-based Access Control

Hi all, we are proposing a Role-based ACL EIP

1 Like

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

1. Why “incompatible” events?

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

  1. Compliant contracts MUST implement IERC_ACL_CORE
  2. 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

2. Why “bytes” field in methods?

What is the motivation to add bytes data arguments 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 in IERC_ACL_GENERAL conform to EIP-5750 to allow extension.

So they could support future expansions, for example:

  • Compared to GovernerAlpha, The new GovernerBravo adds new methods castVoteWithReason and castVoteWithSig which can be supported without change of method if they have an extra bytes.
  • Compared to ERC-20’s transferm, ERC-721 and ERC-1155’s transferFrom has an extra bytes to support future extensions.

Example of of possible future extension include:

function 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:

  • with 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.

Why are behavior of functions not specified?

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. :heart:

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:

  1. You think there is editorial unclarity that EIP-5982 wants to comply with OZ’s IAccessControl but creates events with different name / parameters than what’s already in OZ IAccessControl.sol v4.4.1 and you thought the EIP Author is not conscious that such different parameters will cause OZ’s IAccessControl event to be non-compliant with EIP-5982’s optional interface IERC_GENERAL at current snapshot,
  2. You understand the EIP is consciously making a design choice to propose a different event definition debating the design choices made in EIP-5982 to use different name and parameters for these events and is choose not to mandate it so OZ IAccessControl.sol v4.4.1 can remain compatible with EIP.

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

Design Choice A: what shall event signature of RoleGranted be?

Option 1. same as of OZv4.4.1-OZv.4.8.0

event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
  • Pros: simple, backward compatible with OZv.4.41-v4.8.0 which is widely adopted
  • Cons: didn’t indicate who granted the role, and didn’t add bytes for future extension.

Option 2. use new events

event RoleGranted(address indexed grantor, bytes32 indexed role, address indexed grantee, bytes _data);
  • Pros:
    • indicate who granted the role, allowing use cases e.g. off-chain approval signatures
    • add bytes for future extension, allowing use cases .e.g reasoning, commit-reveal, endorsement-approval
  • Cons: aren’t the same as OZv.4.41-v4.8.0. Will require OZ to change IAccessControl if they want to also conform to IERC_ACL_GENERAL

Is this a good summary of our diverging preferences and pros and cons? @frangio