[Proposal] AccessControl grant and revoke function more useful

Support Contracts
1

Hello everyone,

I was making a contract using AccessControl and a question came up when I looked at grant and revoke functions.

With a pre-check for error return in my contract:

require(!hasRole(PARTNER_ROLE, _msgSender()), "Partner already stored");
// Do stuff with partner
_grantRole(PARTNER_ROLE, _msgSender());

Actual AccessControl _grantRole() function:

function _grantRole(bytes32 role, address account) internal virtual {
        if (!hasRole(role, account)) {
            _roles[role].members[account] = true;
            emit RoleGranted(role, account, _msgSender());
        }
}

My proposal:

function _grantRole(bytes32 role, address account) internal virtual {
        require(!hasRole(role, account), "AccessControl: Account already with role");
        _roles[role].members[account] = true;
        emit RoleGranted(role, account, _msgSender());
}

With this change there would be only one call to hasRole in contracts that inherit and want to give a role. There are a security/optimization issues with this? What do you think?

Thanks in advance,
Iván M.M

2

This way you could not call _grantRole for a address which has already a role and it would fail anytime fataly ...
Means you exclude use cases where the transaction should not fail even when the role is already set.

3

Hi,

You are absolutely right. My mistake was thinking that _grantRole should always be called by a case and not as specific.

4

To be fair, wanting the transaction to not fail even when the role is already set only makes sense in the context of another smart contract calling _grantRole, and there are other ways to prevent a reverting contract call to revert the whole transaction. I personally think both approaches are perfectly valid, maybe the if one saves gas, which would be why they went with it