Query AccessControl roles per account

[nventuro]

Proposed by @rumkin on the forum.

It might be useful to be able to list all roles an account has. In a system with a fixed number of roles, this can be achieved by querying the account for all known roles, but this doesn't work as well if dynamic roles are involved.

Note that the role ids returned by this query will not be very useful, since they won't indicate what the role does. This would likely be used instead to answer the question 'does this account have any special permissions?'.

We could achieve this by having a second EnumerableSet per account. Only the gas cost of grantRole and revokeRole would be increased, which may not be a big deal, considering these are not frequent operations.

[frangio]

I think this is reasonable and the gas costs sound acceptable to me. Do we need more information before someone can begin to work on this?

[frangio]

Note that the role ids returned by this query will not be very useful, since they won't indicate what the role does.

It allows then querying who are the admin accounts for each of the roles. This information could be useful.

I also can imagine a future where we have a way to extract from the bytecode which functions require a a certain role. This can be displayed in a UI for access control.

[julianmrodri]

@nventuro @frangio I'm looking into this enhancement and wanted to check with you some doubts I have on how to implement.

1) First, in order to use an EnumerableSet for the Roles by Account I had to create a new implementation named Bytes32Set. I noticed the current implementations only support types address and uint and we need bytes32 to store the Roles. Let me know if this is OK or I should take a different path.

AccessControl.sol

using EnumerableSet for EnumerableSet.Bytes32Set;
    
mapping (address => EnumerableSet.Bytes32Set) private _accountRoles;

function _grantRole(bytes32 role, address account) private {
        if (_roles[role].members.add(account) && _accountRoles[account].add(role)) {
            emit RoleGranted(role, account, _msgSender());
        }
  }

EnumerableSet.sol

  // Bytes32Set

   struct Bytes32Set {
        Set _inner;
    }

   function add(Bytes32Set storage set, bytes32 value) internal returns (bool);
   function remove(Bytes32Set storage set, bytes32 value) internal returns (bool);
   function contains(Bytes32Set storage set, bytes32 value) internal view returns (bool);
   function length(Bytes32Set storage set) internal view returns (uint256) ;
   function at(Bytes32Set storage set, uint256 index) internal view returns (bytes32); 

2) Second, I was looking on how we can implement the functionality that allows to query Account Roles and I see two options, for which I need your clarification and expertise on which one is the best to implement. These are:

AccessControl.sol

Option A: Similar to current approach for getting all role members. We implement two functions that need to be used together. One to get the count of roles for a specific account and then another one to get each individual Role. (Requires to iterate from outside the contract).

function getAccountRoleCount(address account) public view returns (uint256)
function getAccountRole(address account, uint256 index) public view returns (bytes32)

Option B: Returns directly an array of Roles for a given account.

function getAccountRoles(address account) public view returns (bytes32[] memory)

Please, let me know if on the right track and which of the options would be desirable and I can certainly take care of this Thanks for your support!

[nventuro]

Wow, thanks for being so thorough!

1. First, in order to use an EnumerableSet for the Roles by Account I had to create a new implementation named Bytes32Set.

Great! We should also add tests once #2254 is in.

Option B: Returns directly an array of Roles for a given account.

We should always avoid returning arrays directly and prefer an enumeration-based approach as you first suggested. The API you proposed looks great 👌

[frangio]

Regarding this point from the initial suggestion:

Only the gas cost of grantRole and revokeRole would be increased

This is not really true, as this will also increase the bytecode size and thus the deployment cost. I think we should have a clear idea of how much this increase is before deciding to implement it. @julianmrodri Do you think you could do some quick tests of how much the feature increases these things, once you have a rough implementation of them?

[frangio]

As a side note, it's true that it's impossible to retrieve the list of roles per account on-chain, but it's certainly possible to build this list based on the contract events. This is something that can weigh in the discussion on whether the additional costs are worth it.

[julianmrodri]

Sure! Makes sense @frangio. I'll look into that and keep you posted guys.

[rumkin]

@frangio When you build an event based system and then use thee data on-chain, then you'll be needed to enter this data in the chain with transactions. For example if you want to add a new employee or remove all roles from existing one, you will be required to send all the data repeatedly. Each tx would take at least 21000 gas. So to lower gas cost methods should be able to accept arrays as input.

To reduce deployment code Libraries should be used.

[rumkin]

@nventuro It seems reasonable to implement array slicing to reduce the cost of the roles iteration. Calling such method from outer contract will significantly decrease the execution cost even for array of two items, comparing to combination of getAccountRoleCount and getAccountRole:

function getAccountRoleSlice(address userId, uint offset, uint limit) public view returns(byte32[] memory)

Code I used to measure gas usage.

[julianmrodri]

@nventuro @frangio I submitted the PR with the changes and also the analysis of the additional Gas costs for deployment and execution if we implement this. Please feel free to review and let me know if there is something else we should look upon or modify. Thanks!

[frangio]

We've decided to not implement this feature for now to keep the contract slimmer and cheaper to use.

If you're a user that wants this feature, make sure to post in this issue and let us know so we can reconsider.