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

Add ERC165 interface detection to AccessControl #2562

Merged
merged 4 commits into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Rename `UpgradeableProxy` to `ERC1967Proxy`. ([#2547](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2547))
* `ERC777`: Optimize the gas costs of the constructor. ([#2551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2551))
* `ERC721URIStorage`: Add a new extension that implements the `_setTokenURI` behavior as it was available in 3.4.0. ([#2555](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2555))
* `AccessControl`: Added ERC165 interface detection. ([#2562](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2562))

### How to upgrade from 3.x

Expand Down
32 changes: 26 additions & 6 deletions contracts/access/AccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,18 @@
pragma solidity ^0.8.0;

import "../utils/Context.sol";
import "../utils/introspection/ERC165.sol";

/**
* @dev External interface of AccessControl declared to support ERC165 detection.
*/
interface IAccessControl {
function hasRole(bytes32 role, address account) external view returns (bool);
function getRoleAdmin(bytes32 role) external view returns (bytes32);
function grantRole(bytes32 role, address account) external;
function revokeRole(bytes32 role, address account) external;
function renounceRole(bytes32 role, address account) external;
}

/**
* @dev Contract module that allows children to implement role-based access
Expand Down Expand Up @@ -42,7 +54,7 @@ import "../utils/Context.sol";
* grant and revoke this role. Extra precautions should be taken to secure
* accounts that have been granted it.
*/
abstract contract AccessControl is Context {
abstract contract AccessControl is Context, IAccessControl, ERC165 {
struct RoleData {
mapping (address => bool) members;
bytes32 adminRole;
Expand Down Expand Up @@ -79,10 +91,18 @@ abstract contract AccessControl is Context {
*/
event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);

/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
return interfaceId == type(IAccessControl).interfaceId
|| super.supportsInterface(interfaceId);
}

/**
* @dev Returns `true` if `account` has been granted `role`.
*/
function hasRole(bytes32 role, address account) public view returns (bool) {
function hasRole(bytes32 role, address account) public view override returns (bool) {
return _roles[role].members[account];
}

Expand All @@ -92,7 +112,7 @@ abstract contract AccessControl is Context {
*
* To change a role's admin, use {_setRoleAdmin}.
*/
function getRoleAdmin(bytes32 role) public view returns (bytes32) {
function getRoleAdmin(bytes32 role) public view override returns (bytes32) {
return _roles[role].adminRole;
}

Expand All @@ -106,7 +126,7 @@ abstract contract AccessControl is Context {
*
* - the caller must have ``role``'s admin role.
*/
function grantRole(bytes32 role, address account) public virtual {
function grantRole(bytes32 role, address account) public virtual override {
require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to grant");

_grantRole(role, account);
Expand All @@ -121,7 +141,7 @@ abstract contract AccessControl is Context {
*
* - the caller must have ``role``'s admin role.
*/
function revokeRole(bytes32 role, address account) public virtual {
function revokeRole(bytes32 role, address account) public virtual override {
require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to revoke");

_revokeRole(role, account);
Expand All @@ -141,7 +161,7 @@ abstract contract AccessControl is Context {
*
* - the caller must be `account`.
*/
function renounceRole(bytes32 role, address account) public virtual {
function renounceRole(bytes32 role, address account) public virtual override {
require(account == _msgSender(), "AccessControl: can only renounce roles for self");

_revokeRole(role, account);
Expand Down
22 changes: 19 additions & 3 deletions contracts/access/AccessControlEnumerable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,30 @@ pragma solidity ^0.8.0;
import "./AccessControl.sol";
import "../utils/structs/EnumerableSet.sol";

/**
* @dev External interface of AccessControlEnumerable declared to support ERC165 detection.
*/
interface IAccessControlEnumerable {
function getRoleMember(bytes32 role, uint256 index) external view returns (address);
function getRoleMemberCount(bytes32 role) external view returns (uint256);
}

/**
* @dev Extension of {AccessControl} that allows enumerating the members of each role.
*/
abstract contract AccessControlEnumerable is AccessControl {
abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessControl {
using EnumerableSet for EnumerableSet.AddressSet;

mapping (bytes32 => EnumerableSet.AddressSet) private _roleMembers;

/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
return interfaceId == type(IAccessControlEnumerable).interfaceId
|| super.supportsInterface(interfaceId);
}

/**
* @dev Returns one of the accounts that have `role`. `index` must be a
* value between 0 and {getRoleMemberCount}, non-inclusive.
Expand All @@ -25,15 +41,15 @@ abstract contract AccessControlEnumerable is AccessControl {
* https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
* for more information.
*/
function getRoleMember(bytes32 role, uint256 index) public view returns (address) {
function getRoleMember(bytes32 role, uint256 index) public view override returns (address) {
return _roleMembers[role].at(index);
}

/**
* @dev Returns the number of accounts that have `role`. Can be used
* together with {getRoleMember} to enumerate all bearers of a role.
*/
function getRoleMemberCount(bytes32 role) public view returns (uint256) {
function getRoleMemberCount(bytes32 role) public view override returns (uint256) {
return _roleMembers[role].length();
}

Expand Down
7 changes: 7 additions & 0 deletions contracts/token/ERC1155/presets/ERC1155PresetMinterPauser.sol
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ contract ERC1155PresetMinterPauser is Context, AccessControlEnumerable, ERC1155B
_unpause();
}

/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override(AccessControlEnumerable, ERC1155) returns (bool) {
return super.supportsInterface(interfaceId);
}

function _beforeTokenTransfer(
address operator,
address from,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ contract ERC721PresetMinterPauserAutoId is Context, AccessControlEnumerable, ERC
/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721, ERC721Enumerable) returns (bool) {
function supportsInterface(bytes4 interfaceId) public view virtual override(AccessControlEnumerable, ERC721, ERC721Enumerable) returns (bool) {
return super.supportsInterface(interfaceId);
}
}
6 changes: 6 additions & 0 deletions test/access/AccessControl.behavior.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');

const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior');

const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
const ROLE = web3.utils.soliditySha3('ROLE');
const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');

function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, otherAdmin, otherAuthorized) {
shouldSupportInterfaces(['AccessControl']);

describe('default admin', function () {
it('deployer has default admin role', async function () {
expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.equal(true);
Expand Down Expand Up @@ -156,6 +160,8 @@ function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, o
}

function shouldBehaveLikeAccessControlEnumerable (errorPrefix, admin, authorized, other, otherAdmin, otherAuthorized) {
shouldSupportInterfaces(['AccessControlEnumerable']);

describe('enumerating', function () {
it('role bearers can be enumerated', async function () {
await this.accessControl.grantRole(ROLE, authorized, { from: admin });
Expand Down
3 changes: 3 additions & 0 deletions test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { shouldSupportInterfaces } = require('../../../utils/introspection/SupportsInterface.behavior');

const { expect } = require('chai');

Expand All @@ -24,6 +25,8 @@ contract('ERC1155PresetMinterPauser', function (accounts) {
this.token = await ERC1155PresetMinterPauser.new(uri, { from: deployer });
});

shouldSupportInterfaces(['ERC1155', 'AccessControl', 'AccessControlEnumerable']);

it('deployer has the default admin role', async function () {
expect(await this.token.getRoleMemberCount(DEFAULT_ADMIN_ROLE)).to.be.bignumber.equal('1');
expect(await this.token.getRoleMember(DEFAULT_ADMIN_ROLE, 0)).to.equal(deployer);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { shouldSupportInterfaces } = require('../../../utils/introspection/SupportsInterface.behavior');

const { expect } = require('chai');

Expand All @@ -19,6 +20,8 @@ contract('ERC721PresetMinterPauserAutoId', function (accounts) {
this.token = await ERC721PresetMinterPauserAutoId.new(name, symbol, baseURI, { from: deployer });
});

shouldSupportInterfaces(['ERC721', 'ERC721Enumerable', 'AccessControl', 'AccessControlEnumerable']);

it('token has correct name', async function () {
expect(await this.token.name()).to.equal(name);
});
Expand Down
13 changes: 12 additions & 1 deletion test/utils/introspection/SupportsInterface.behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ const INTERFACES = {
'onERC1155Received(address,address,uint256,uint256,bytes)',
'onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)',
],
AccessControl: [
'hasRole(bytes32,address)',
'getRoleAdmin(bytes32)',
'grantRole(bytes32,address)',
'revokeRole(bytes32,address)',
'renounceRole(bytes32,address)',
],
AccessControlEnumerable: [
'getRoleMember(bytes32,uint256)',
'getRoleMemberCount(bytes32)',
],
};

const INTERFACE_IDS = {};
Expand All @@ -54,7 +65,7 @@ for (const k of Object.getOwnPropertyNames(INTERFACES)) {
function shouldSupportInterfaces (interfaces = []) {
describe('Contract interface', function () {
beforeEach(function () {
this.contractUnderTest = this.mock || this.token || this.holder;
this.contractUnderTest = this.mock || this.token || this.holder || this.accessControl;
});

for (const k of interfaces) {
Expand Down