Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 'external' functions #2162

Merged
merged 11 commits into from
Apr 2, 2020
50 changes: 26 additions & 24 deletions contracts/access/AccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ import "../GSN/Context.sol";
* }
* ```
*
* Roles can be granted and revoked programatically by calling the `internal`
* {_grantRole} and {_revokeRole} functions.
*
* This can also be achieved dynamically via the `external` {grantRole} and
* Roles can be granted and revoked dynamically via the {grantRole} and
* {revokeRole} functions. Each role has an associated admin role, and only
* accounts that have a role's admin role can call {grantRole} and {revokeRoke}.
*
Expand All @@ -52,9 +49,8 @@ abstract contract AccessControl is Context {
/**
* @dev Emitted when `account` is granted `role`.
*
* `sender` is the account that originated the contract call:
* - if using `grantRole`, it is the admin role bearer
* - if using `_grantRole`, its meaning is system-dependent
* `sender` is the account that originated the contract call, an admin role
* bearer except when using {_setupRole}.
*/
event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);

Expand All @@ -64,7 +60,6 @@ abstract contract AccessControl is Context {
* `sender` is the account that originated the contract call:
* - if using `revokeRole`, it is the admin role bearer
* - if using `renounceRole`, it is the role bearer (i.e. `account`)
* - if using `_renounceRole`, its meaning is system-dependent
*/
event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);

Expand Down Expand Up @@ -112,7 +107,8 @@ abstract contract AccessControl is Context {
/**
* @dev Grants `role` to `account`.
*
* Calls {_grantRole} internally.
* If `account` had not been already granted `role`, emits a {RoleGranted}
* event.
*
* Requirements:
*
Expand All @@ -127,7 +123,7 @@ abstract contract AccessControl is Context {
/**
* @dev Revokes `role` from `account`.
*
* Calls {_revokeRole} internally.
* If `account` had been granted `role`, emits a {RoleRevoked} event.
*
* Requirements:
*
Expand All @@ -146,6 +142,9 @@ abstract contract AccessControl is Context {
* purpose is to provide a mechanism for accounts to lose their privileges
* if they are compromised (such as when a trusted device is misplaced).
*
* If the calling account had been granted `role`, emits a {RoleRevoked}
* event.
*
* Requirements:
*
* - the caller must be `account`.
Expand All @@ -161,22 +160,13 @@ abstract contract AccessControl is Context {
*
* If `account` had not been already granted `role`, emits a {RoleGranted}
* event.
*/
function _grantRole(bytes32 role, address account) internal virtual {
if (_roles[role].members.add(account)) {
emit RoleGranted(role, account, _msgSender());
}
}

/**
* @dev Revokes `role` from `account`.
*
* If `account` had been granted `role`, emits a {RoleRevoked} event.
* NOTE: Unlike {grantRole}, this function doesn't perform any checks on the
* calling account. Its usage should be restricted to grating the initial
nventuro marked this conversation as resolved.
Show resolved Hide resolved
* set of rules during contract construction.
*/
function _revokeRole(bytes32 role, address account) internal virtual {
if (_roles[role].members.remove(account)) {
emit RoleRevoked(role, account, _msgSender());
}
function _setupRole(bytes32 role, address account) internal virtual {
nventuro marked this conversation as resolved.
Show resolved Hide resolved
_grantRole(role, account);
}

/**
Expand All @@ -185,4 +175,16 @@ abstract contract AccessControl is Context {
function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual {
_roles[role].adminRole = adminRole;
}

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

function _revokeRole(bytes32 role, address account) private {
if (_roles[role].members.remove(account)) {
emit RoleRevoked(role, account, _msgSender());
}
}
}
2 changes: 1 addition & 1 deletion contracts/mocks/AccessControlMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import "../access/AccessControl.sol";

contract AccessControlMock is AccessControl {
constructor() public {
_grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
_setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
}

function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
Expand Down
12 changes: 6 additions & 6 deletions docs/modules/ROOT/pages/access-control.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ contract MyToken is ERC20, AccessControl {

constructor(address minter) public {
// Grant the minter role to a specified account
_grantRole(MINTER_ROLE, minter);
_setupRole(MINTER_ROLE, minter);
}

function mint(address to, uint256 amount) public {
Expand Down Expand Up @@ -98,8 +98,8 @@ contract MyToken is ERC20, AccessControl {
bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

constructor(address minter, address burner) public {
_grantRole(MINTER_ROLE, minter);
_grantRole(BURNER_ROLE, burner);
_setupRole(MINTER_ROLE, minter);
_setupRole(BURNER_ROLE, burner);
}

function mint(address to, uint256 amount) public {
Expand All @@ -119,11 +119,11 @@ So clean! By splitting concerns this way, more granular levels of permission may
[[granting-and-revoking]]
=== Granting and Revoking Roles

The ERC20 token example above uses `\_grantRole`, an `internal` function that is useful when programmatically asigning roles (such as during construction). But what if we later want to grant the 'minter' role to additional accounts?
The ERC20 token example above uses `\_setupRole`, an `internal` function that is useful when programmatically asigning roles (such as during construction). But what if we later want to grant the 'minter' role to additional accounts?

By default, **accounts with a role cannot grant it or revoke it from other accounts**: all having a role does is making the `hasRole` check pass. To grant and revoke roles dynamically, you will need help from the _role's admin_.

Every role has an associated admin role, which grants permission to call the `grantRole` and `revokeRole` `external` functions. A role can be granted or revoked by using these if the calling account has the corresponding admin role. Multiple roles may have the same admin role to make management easier. A role's admin can even be the same role itself, which would cause accounts with that role to be able to also grant and revoke it.
Every role has an associated admin role, which grants permission to call the `grantRole` and `revokeRole` functions. A role can be granted or revoked by using these if the calling account has the corresponding admin role. Multiple roles may have the same admin role to make management easier. A role's admin can even be the same role itself, which would cause accounts with that role to be able to also grant and revoke it.

This mechanism can be used to create complex permissioning structures resembling organizational charts, but it also provides an easy way to manage simpler applications. `AccessControl` includes a special role, called `DEFAULT_ADMIN_ROLE`, which acts as the **default admin role for all roles**. An account with this role will be able to manage any other role, unless `\_setRoleAdmin` is used to select a new admin role.

Expand All @@ -143,7 +143,7 @@ contract MyToken is ERC20, AccessControl {
constructor() public {
// Grant the contract deployer the default admin role: it will be able
// to grant and revoke any roles
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
}

function mint(address to, uint256 amount) public {
Expand Down