Proposal to revamp Roles

[nventuro]

RBAC was removed after #1090 due to issues with having strings be keys in the mapping, and replaces with Roles in #1291. Integer ids were considered, but ultimately discarded, partly because of a lack of an id allocation mechanism (hash of strings?).

The new scheme works, but it has a couple wrinkles. Each new role requires the creation of a new role contract, which will be almost a carbon copy of all other roles. Additionally, they all add their own functions, polluting the contract's ABI and leading to duplicated bytecode. We've considered autogenerating these contracts (#1290), but that requires new tooling, documentation, a pre-compile step, etc., all of which are best avoided. Finally, role reuse may lead to unexpected situations (e.g. both ERC20Mintable and ERC721Mintable use the same MinterRole).

I propose an alternative scheme, where we opt for an approach similar to RBAC's (although I'd propose a more friendly name, perhaps RoleChecker?), but get rid of the id issue altogether by using automatically generated ids.

contract RoleChecker {
  mapping (uint256 => Roles.Role) roles;
  Counters.Counter counter;

  function hasRole(uint256 id) view returns (bool) { return roles[id]; }

  function newRole() returns (uint256) {
    counter.increment();
    return keccak(address(this), counter.current());
  }
}

Each contract that requires roles simply inherits from RoleChecker and creates whichever roles it needs.

contract ERC20Mintable is RoleChecker {
   uint256 _erc20mintRoleId;

   constructor() {
     _erc20mintRoleId = newRole();
   } 

  function mint(...) onlyRole(_erc20mintRoleId) {
     ...
  }

  function erc20mintRoleId() view returns(uint256) {
    return _erc20mintRoleId;
  }
}

The only added boilerplate is a public getter for this id, so that users can retrieve it and use it to e.g. call addRole(id), hasRole(id, account), etc.

And since each role id is created with a hash of an integer (autoincremented by Counter) and the contract's address, they will be unique for each contract. Preventing these clashes will reduce user error, where someone may mistakenly use a different contract's role ids.

[alanarvelo]

Hi @nventuro @frangio I will take a deeper look into this and revert with questions.

[alanarvelo]

Hi @nventuro and @frangio made a PR with the creation of RoleChecker.sol, and a slight addition to Roles.sol, so we can discuss the implementation of this over the code — with inline comments and all that good stuff.

Combined nventuro's suggestion with some of the RCAB principles. Let me know what you guys think.

[nventuro]

We'll probably want to use immutable to store the role ids, making the library both safer and cheaper.

[nventuro]

A simple alternative to avoid the getRoleId() boilerplate is to attach a string identifier to each new role:

contract RoleChecker {
    mapping (string => uint256) roleIds;

    function newRole(string memory name) internal returns (uint256) {
        uint256 id = _getNewId();
        roleIds[name] = id;

        return id;
    }

    function getRoleId(string memory name) external view returns (uint256) {
        uint256 id = roleIds[name];
        require(id != 0);
        return id;
    }
}

contract ERC20Mintable is ERC20, RoleChecker {
    uint256 minterRoleId;

    constructor() public {
        minterRoleId = RoleChecker.newRole('minter');
    }

    function mint() external onlyRole(minterRoleId) {
        ...
    }
}

An external agent would call contract.getRoleId('minter') in order to check if an account has the role, or enumerate role bearers.

A possible downside of this approach is that it opens up a subset of the issues from #1090: an malicious party could write onlyRole(getRoleId('minter')) and achieve a similar result. Making getRoleId external instead of public could alleviate this.

[nventuro]

With the migration to Solidity v0.6 arriving soon, we will need to tackle this issue as part of the upcoming v3.0 release.

Since there are still multiple ideas out there, I created Redesigning Access Control on our forum to gather feedback about the different proposals, and settle on a high-level design we're comfortable with. Once we get to that point, we can continue work here.

Please participate in this process by sharing your ideas and experience, so we can design the best solution possible!