From b301381471e9e035fc3fac86c9c42e12d98d365c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 12:58:11 -0300
Subject: [PATCH 01/24] Remove Roles

---
 contracts/access/Roles.sol    | 36 -------------------
 contracts/mocks/RolesMock.sol | 21 -----------
 test/access/Roles.test.js     | 68 -----------------------------------
 3 files changed, 125 deletions(-)
 delete mode 100644 contracts/access/Roles.sol
 delete mode 100644 contracts/mocks/RolesMock.sol
 delete mode 100644 test/access/Roles.test.js

diff --git a/contracts/access/Roles.sol b/contracts/access/Roles.sol
deleted file mode 100644
index e31debbe3bb..00000000000
--- a/contracts/access/Roles.sol
+++ /dev/null
@@ -1,36 +0,0 @@
-pragma solidity ^0.6.0;
-
-/**
- * @title Roles
- * @dev Library for managing addresses assigned to a Role.
- */
-library Roles {
-    struct Role {
-        mapping (address => bool) bearer;
-    }
-
-    /**
-     * @dev Give an account access to this role.
-     */
-    function add(Role storage role, address account) internal {
-        require(!has(role, account), "Roles: account already has role");
-        role.bearer[account] = true;
-    }
-
-    /**
-     * @dev Remove an account's access to this role.
-     */
-    function remove(Role storage role, address account) internal {
-        require(has(role, account), "Roles: account does not have role");
-        role.bearer[account] = false;
-    }
-
-    /**
-     * @dev Check if an account has this role.
-     * @return bool
-     */
-    function has(Role storage role, address account) internal view returns (bool) {
-        require(account != address(0), "Roles: account is the zero address");
-        return role.bearer[account];
-    }
-}
diff --git a/contracts/mocks/RolesMock.sol b/contracts/mocks/RolesMock.sol
deleted file mode 100644
index 586342a788d..00000000000
--- a/contracts/mocks/RolesMock.sol
+++ /dev/null
@@ -1,21 +0,0 @@
-pragma solidity ^0.6.0;
-
-import "../access/Roles.sol";
-
-contract RolesMock {
-    using Roles for Roles.Role;
-
-    Roles.Role private dummyRole;
-
-    function add(address account) public {
-        dummyRole.add(account);
-    }
-
-    function remove(address account) public {
-        dummyRole.remove(account);
-    }
-
-    function has(address account) public view returns (bool) {
-        return dummyRole.has(account);
-    }
-}
diff --git a/test/access/Roles.test.js b/test/access/Roles.test.js
deleted file mode 100644
index ac95eebdce7..00000000000
--- a/test/access/Roles.test.js
+++ /dev/null
@@ -1,68 +0,0 @@
-const { accounts, contract } = require('@openzeppelin/test-environment');
-
-const { expectRevert, constants } = require('@openzeppelin/test-helpers');
-const { ZERO_ADDRESS } = constants;
-
-const { expect } = require('chai');
-
-const RolesMock = contract.fromArtifact('RolesMock');
-
-describe('Roles', function () {
-  const [ authorized, otherAuthorized, other ] = accounts;
-
-  beforeEach(async function () {
-    this.roles = await RolesMock.new();
-  });
-
-  it('reverts when querying roles for the zero account', async function () {
-    await expectRevert(this.roles.has(ZERO_ADDRESS), 'Roles: account is the zero address');
-  });
-
-  context('initially', function () {
-    it('doesn\'t pre-assign roles', async function () {
-      expect(await this.roles.has(authorized)).to.equal(false);
-      expect(await this.roles.has(otherAuthorized)).to.equal(false);
-      expect(await this.roles.has(other)).to.equal(false);
-    });
-
-    describe('adding roles', function () {
-      it('adds roles to a single account', async function () {
-        await this.roles.add(authorized);
-        expect(await this.roles.has(authorized)).to.equal(true);
-        expect(await this.roles.has(other)).to.equal(false);
-      });
-
-      it('reverts when adding roles to an already assigned account', async function () {
-        await this.roles.add(authorized);
-        await expectRevert(this.roles.add(authorized), 'Roles: account already has role');
-      });
-
-      it('reverts when adding roles to the zero account', async function () {
-        await expectRevert(this.roles.add(ZERO_ADDRESS), 'Roles: account is the zero address');
-      });
-    });
-  });
-
-  context('with added roles', function () {
-    beforeEach(async function () {
-      await this.roles.add(authorized);
-      await this.roles.add(otherAuthorized);
-    });
-
-    describe('removing roles', function () {
-      it('removes a single role', async function () {
-        await this.roles.remove(authorized);
-        expect(await this.roles.has(authorized)).to.equal(false);
-        expect(await this.roles.has(otherAuthorized)).to.equal(true);
-      });
-
-      it('reverts when removing unassigned roles', async function () {
-        await expectRevert(this.roles.remove(other), 'Roles: account does not have role');
-      });
-
-      it('reverts when removing roles from the zero account', async function () {
-        await expectRevert(this.roles.remove(ZERO_ADDRESS), 'Roles: account is the zero address');
-      });
-    });
-  });
-});

From 9f6b348d50f059813531208a5c11706ffb2dc3d5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 12:58:21 -0300
Subject: [PATCH 02/24] Add AccessControl and tests

---
 contracts/access/AccessControl.sol    |  65 ++++++++++++++
 contracts/access/IAccessControl.sol   |  28 ++++++
 contracts/mocks/AccessControlMock.sol |  13 +++
 test/access/AccessControl.test.js     | 124 ++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 contracts/access/AccessControl.sol
 create mode 100644 contracts/access/IAccessControl.sol
 create mode 100644 contracts/mocks/AccessControlMock.sol
 create mode 100644 test/access/AccessControl.test.js

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
new file mode 100644
index 00000000000..7eb8a29a72b
--- /dev/null
+++ b/contracts/access/AccessControl.sol
@@ -0,0 +1,65 @@
+pragma solidity ^0.6.0;
+
+import "./IAccessControl.sol";
+import "../utils/EnumerableSet.sol";
+
+abstract contract AccessControl is IAccessControl {
+    using EnumerableSet for EnumerableSet.AddressSet;
+
+    struct Role {
+        EnumerableSet.AddressSet members;
+        bytes32 admin;
+    }
+
+    mapping (bytes32 => Role) private _roles;
+
+    bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
+
+    function hasRole(bytes32 roleId, address account) public view override returns (bool) {
+        return _roles[roleId].members.contains(account);
+    }
+
+    function getRoleMembersCount(bytes32 roleId) public view override returns (uint256) {
+        return _roles[roleId].members.length();
+    }
+
+    function getRoleMember(bytes32 roleId, uint256 index) public view override returns (address) {
+        return _roles[roleId].members.get(index);
+    }
+
+    function getRoleAdmin(bytes32 roleId) external view override returns (bytes32) {
+        return _roles[roleId].admin;
+    }
+
+    function grantRole(bytes32 roleId, address account) external virtual override {
+        require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to grant");
+
+        _grantRole(roleId, account);
+    }
+
+    function revokeRole(bytes32 roleId, address account) external virtual override {
+        require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to revoke");
+
+        _revokeRole(roleId, account);
+    }
+
+    function renounceRole(bytes32 roleId, address account) external virtual override {
+        require(account == msg.sender, "AccessControl: can only renounce roles for self");
+
+        _revokeRole(roleId, account);
+    }
+
+    function _grantRole(bytes32 roleId, address account) internal virtual {
+        bool added = _roles[roleId].members.add(account);
+        require(added, "AccessControl: account already has granted role");
+    }
+
+    function _revokeRole(bytes32 roleId, address account) internal virtual {
+        bool removed = _roles[roleId].members.remove(account);
+        require(removed, "AccessControl: account does not have revoked role");
+    }
+
+    function _setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) internal virtual {
+        _roles[roleId].admin = adminRoleId;
+    }
+}
diff --git a/contracts/access/IAccessControl.sol b/contracts/access/IAccessControl.sol
new file mode 100644
index 00000000000..3daf3d2f876
--- /dev/null
+++ b/contracts/access/IAccessControl.sol
@@ -0,0 +1,28 @@
+pragma solidity ^0.6.0;
+
+interface IAccessControl {
+    // Queries
+
+    // Returns true if an account has a role
+    function hasRole(bytes32 roleId, address account) external view returns (bool);
+
+    // Returns the number of accounts with a role
+    function getRoleMembersCount(bytes32 roleId) external view returns (uint256);
+
+    // Returns an account with a role at index
+    function getRoleMember(bytes32 roleId, uint256 index) external view returns (address);
+
+    // Returns a role's admin role
+    function getRoleAdmin(bytes32 roleId) external view returns (bytes32);
+
+    // Operations
+
+    // Gives a role to an account. Caller must have its admin role
+    function grantRole(bytes32 roleId, address account) external;
+
+    // Revokes a role from an account. Caller must have its admin role
+    function revokeRole(bytes32 roleId, address account) external;
+
+    // Renounces a role. Caller must be `account`
+    function renounceRole(bytes32 roleId, address account) external;
+}
diff --git a/contracts/mocks/AccessControlMock.sol b/contracts/mocks/AccessControlMock.sol
new file mode 100644
index 00000000000..b3331e71fa8
--- /dev/null
+++ b/contracts/mocks/AccessControlMock.sol
@@ -0,0 +1,13 @@
+pragma solidity ^0.6.0;
+
+import "../access/AccessControl.sol";
+
+contract AccessControlMock is AccessControl {
+    constructor() public {
+        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
+    }
+
+    function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
+        _setRoleAdmin(roleId, adminRoleId);
+    }
+}
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
new file mode 100644
index 00000000000..1f1e40f0f7b
--- /dev/null
+++ b/test/access/AccessControl.test.js
@@ -0,0 +1,124 @@
+const { accounts, contract, web3 } = require('@openzeppelin/test-environment');
+
+const { expectRevert } = require('@openzeppelin/test-helpers');
+
+const { expect } = require('chai');
+
+const AccessControlMock = contract.fromArtifact('AccessControlMock');
+
+describe('AccessControl', function () {
+  const [ defaultAdmin, authorized, other ] = accounts;
+
+  const DEFAULT_ADMIN_ROLE_ID = '0x0000000000000000000000000000000000000000000000000000000000000000';
+  const OTHER_ROLE_ID = web3.utils.soliditySha3('OTHER_ROLE_ID');
+
+  beforeEach(async function () {
+    this.accessControl = await AccessControlMock.new({ from: defaultAdmin });
+  });
+
+  describe('default admin', function () {
+    it('deployer has default admin role', async function () {
+      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE_ID, defaultAdmin)).to.equal(true);
+    });
+
+    it('other roles admin\'s is the default admin role', async function () {
+      expect(await this.accessControl.getRoleAdmin(OTHER_ROLE_ID)).to.equal(DEFAULT_ADMIN_ROLE_ID);
+    });
+
+    it('default admin role\'s admin is itself', async function () {
+      expect(await this.accessControl.getRoleAdmin(DEFAULT_ADMIN_ROLE_ID)).to.equal(DEFAULT_ADMIN_ROLE_ID);
+    });
+  });
+
+  describe('granting', function () {
+    it('admin can grant role to other accounts', async function () {
+      await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(true);
+    });
+
+    it('non-admin cannot grant role to other accounts', async function () {
+      await expectRevert(
+        this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: other }),
+        'AccessControl: sender must be an admin to grant'
+      );
+    });
+
+    it('accounts with a role cannot have it granted', async function () {
+      await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      await expectRevert(
+        this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
+        'AccessControl: account already has granted role'
+      );
+    });
+  });
+
+  describe('revoking', function () {
+    it('roles that are not had cannot be revoked', async function () {
+      await expectRevert(
+        this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
+        'AccessControl: account does not have revoked role'
+      );
+    });
+
+    context('with granted role', function () {
+      beforeEach(async function () {
+        await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      });
+
+      it('admin can revoke role', async function () {
+        await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+      });
+
+      it('non-admin cannot revoke role', async function () {
+        await expectRevert(
+          this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: other }),
+          'AccessControl: sender must be an admin to revoke'
+        );
+      });
+
+      it('a role cannot be revoked multiple times', async function () {
+        await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        await expectRevert(
+          this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
+          'AccessControl: account does not have revoked role'
+        );
+      });
+    });
+  });
+
+  describe('renouncing', function () {
+    it('roles that are not had cannot be renounced', async function () {
+      await expectRevert(
+        this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized }),
+        'AccessControl: account does not have revoked role'
+      );
+    });
+
+    context('with granted role', function () {
+      beforeEach(async function () {
+        await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      });
+
+      it('bearer can renounce role', async function () {
+        await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+      });
+
+      it('only the sender can renounce their roles', async function () {
+        await expectRevert(
+          this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
+          'AccessControl: can only renounce roles for self'
+        );
+      });
+
+      it('a role cannot be renounced multiple times', async function () {
+        await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        await expectRevert(
+          this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized }),
+          'AccessControl: account does not have revoked role'
+        );
+      });
+    });
+  });
+});

From 7947ad3e0585287f4aefeea2fb9c7439a33c8169 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 15:28:25 -0300
Subject: [PATCH 03/24] Removed IAccessControl

---
 contracts/access/AccessControl.sol  | 24 +++++++++++++++---------
 contracts/access/IAccessControl.sol | 28 ----------------------------
 2 files changed, 15 insertions(+), 37 deletions(-)
 delete mode 100644 contracts/access/IAccessControl.sol

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 7eb8a29a72b..f8019160976 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -1,9 +1,8 @@
 pragma solidity ^0.6.0;
 
-import "./IAccessControl.sol";
 import "../utils/EnumerableSet.sol";
 
-abstract contract AccessControl is IAccessControl {
+abstract contract AccessControl {
     using EnumerableSet for EnumerableSet.AddressSet;
 
     struct Role {
@@ -15,35 +14,42 @@ abstract contract AccessControl is IAccessControl {
 
     bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
 
-    function hasRole(bytes32 roleId, address account) public view override returns (bool) {
+    // Returns true if an account has a role
+    function hasRole(bytes32 roleId, address account) public view returns (bool) {
         return _roles[roleId].members.contains(account);
     }
 
-    function getRoleMembersCount(bytes32 roleId) public view override returns (uint256) {
+    // Returns the number of accounts with a role
+    function getRoleMembersCount(bytes32 roleId) public view returns (uint256) {
         return _roles[roleId].members.length();
     }
 
-    function getRoleMember(bytes32 roleId, uint256 index) public view override returns (address) {
+    // Returns an account with a role at index
+    function getRoleMember(bytes32 roleId, uint256 index) public view returns (address) {
         return _roles[roleId].members.get(index);
     }
 
-    function getRoleAdmin(bytes32 roleId) external view override returns (bytes32) {
+    // Returns a role's admin role
+    function getRoleAdmin(bytes32 roleId) external view returns (bytes32) {
         return _roles[roleId].admin;
     }
 
-    function grantRole(bytes32 roleId, address account) external virtual override {
+    // Gives a role to an account. Caller must have its admin role
+    function grantRole(bytes32 roleId, address account) external virtual {
         require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to grant");
 
         _grantRole(roleId, account);
     }
 
-    function revokeRole(bytes32 roleId, address account) external virtual override {
+    // Revokes a role from an account. Caller must have its admin role
+    function revokeRole(bytes32 roleId, address account) external virtual {
         require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to revoke");
 
         _revokeRole(roleId, account);
     }
 
-    function renounceRole(bytes32 roleId, address account) external virtual override {
+    // Renounces a role. Caller must be `account`
+    function renounceRole(bytes32 roleId, address account) external virtual {
         require(account == msg.sender, "AccessControl: can only renounce roles for self");
 
         _revokeRole(roleId, account);
diff --git a/contracts/access/IAccessControl.sol b/contracts/access/IAccessControl.sol
deleted file mode 100644
index 3daf3d2f876..00000000000
--- a/contracts/access/IAccessControl.sol
+++ /dev/null
@@ -1,28 +0,0 @@
-pragma solidity ^0.6.0;
-
-interface IAccessControl {
-    // Queries
-
-    // Returns true if an account has a role
-    function hasRole(bytes32 roleId, address account) external view returns (bool);
-
-    // Returns the number of accounts with a role
-    function getRoleMembersCount(bytes32 roleId) external view returns (uint256);
-
-    // Returns an account with a role at index
-    function getRoleMember(bytes32 roleId, uint256 index) external view returns (address);
-
-    // Returns a role's admin role
-    function getRoleAdmin(bytes32 roleId) external view returns (bytes32);
-
-    // Operations
-
-    // Gives a role to an account. Caller must have its admin role
-    function grantRole(bytes32 roleId, address account) external;
-
-    // Revokes a role from an account. Caller must have its admin role
-    function revokeRole(bytes32 roleId, address account) external;
-
-    // Renounces a role. Caller must be `account`
-    function renounceRole(bytes32 roleId, address account) external;
-}

From f24302d4898e25fc19e58b5103faf0a38330342c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 15:33:36 -0300
Subject: [PATCH 04/24] Add RoleGranted and RoleRevoked events

---
 contracts/access/AccessControl.sol |  7 +++++++
 test/access/AccessControl.test.js  | 14 ++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index f8019160976..88e316e8ee3 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -14,6 +14,9 @@ abstract contract AccessControl {
 
     bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
 
+    event RoleGranted(bytes32 indexed roleId, address indexed account);
+    event RoleRevoked(bytes32 indexed roleId, address indexed account);
+
     // Returns true if an account has a role
     function hasRole(bytes32 roleId, address account) public view returns (bool) {
         return _roles[roleId].members.contains(account);
@@ -58,11 +61,15 @@ abstract contract AccessControl {
     function _grantRole(bytes32 roleId, address account) internal virtual {
         bool added = _roles[roleId].members.add(account);
         require(added, "AccessControl: account already has granted role");
+
+        emit RoleGranted(roleId, account);
     }
 
     function _revokeRole(bytes32 roleId, address account) internal virtual {
         bool removed = _roles[roleId].members.remove(account);
         require(removed, "AccessControl: account does not have revoked role");
+
+        emit RoleRevoked(roleId, account);
     }
 
     function _setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) internal virtual {
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 1f1e40f0f7b..daad4f317c2 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -1,6 +1,6 @@
 const { accounts, contract, web3 } = require('@openzeppelin/test-environment');
 
-const { expectRevert } = require('@openzeppelin/test-helpers');
+const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
 
 const { expect } = require('chai');
 
@@ -32,7 +32,9 @@ describe('AccessControl', function () {
 
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
-      await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      const receipt = await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE_ID });
+
       expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(true);
     });
 
@@ -66,7 +68,9 @@ describe('AccessControl', function () {
       });
 
       it('admin can revoke role', async function () {
-        await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+
         expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
       });
 
@@ -101,7 +105,9 @@ describe('AccessControl', function () {
       });
 
       it('bearer can renounce role', async function () {
-        await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+
         expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
       });
 

From 26052a55af7cfb6ee46c37cb994d3f451b28ca38 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 16:08:21 -0300
Subject: [PATCH 05/24] Make roles grantable and revokable regardless of their
 previous status

---
 contracts/access/AccessControl.sol |  6 ++--
 test/access/AccessControl.test.js  | 44 +++++++++++++-----------------
 2 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 88e316e8ee3..e96b6963d10 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -59,15 +59,13 @@ abstract contract AccessControl {
     }
 
     function _grantRole(bytes32 roleId, address account) internal virtual {
-        bool added = _roles[roleId].members.add(account);
-        require(added, "AccessControl: account already has granted role");
+        _roles[roleId].members.add(account);
 
         emit RoleGranted(roleId, account);
     }
 
     function _revokeRole(bytes32 roleId, address account) internal virtual {
-        bool removed = _roles[roleId].members.remove(account);
-        require(removed, "AccessControl: account does not have revoked role");
+        _roles[roleId].members.remove(account);
 
         emit RoleRevoked(roleId, account);
     }
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index daad4f317c2..0b07481867b 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -45,21 +45,19 @@ describe('AccessControl', function () {
       );
     });
 
-    it('accounts with a role cannot have it granted', async function () {
+    it('accounts can be granted a role multiple times', async function () {
       await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-      await expectRevert(
-        this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
-        'AccessControl: account already has granted role'
-      );
+      const receipt = await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE_ID });
     });
   });
 
   describe('revoking', function () {
-    it('roles that are not had cannot be revoked', async function () {
-      await expectRevert(
-        this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
-        'AccessControl: account does not have revoked role'
-      );
+    it('roles that are not had can be revoked', async function () {
+      expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+
+      const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
     });
 
     context('with granted role', function () {
@@ -81,22 +79,19 @@ describe('AccessControl', function () {
         );
       });
 
-      it('a role cannot be revoked multiple times', async function () {
+      it('a role can be revoked multiple times', async function () {
         await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-        await expectRevert(
-          this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
-          'AccessControl: account does not have revoked role'
-        );
+
+        const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
       });
     });
   });
 
   describe('renouncing', function () {
-    it('roles that are not had cannot be renounced', async function () {
-      await expectRevert(
-        this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized }),
-        'AccessControl: account does not have revoked role'
-      );
+    it('roles that are not had can be renounced', async function () {
+      const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
     });
 
     context('with granted role', function () {
@@ -118,12 +113,11 @@ describe('AccessControl', function () {
         );
       });
 
-      it('a role cannot be renounced multiple times', async function () {
+      it('a role can be renounced multiple times', async function () {
         await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
-        await expectRevert(
-          this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized }),
-          'AccessControl: account does not have revoked role'
-        );
+
+        const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
       });
     });
   });

From 2a8978416d5cbfbbf06e1b98fba927c7bece068c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 16:40:33 -0300
Subject: [PATCH 06/24] Fix typo

---
 test/access/AccessControl.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 0b07481867b..b349086480f 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -21,7 +21,7 @@ describe('AccessControl', function () {
       expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE_ID, defaultAdmin)).to.equal(true);
     });
 
-    it('other roles admin\'s is the default admin role', async function () {
+    it('other roles\'s admin is the default admin role', async function () {
       expect(await this.accessControl.getRoleAdmin(OTHER_ROLE_ID)).to.equal(DEFAULT_ADMIN_ROLE_ID);
     });
 

From 6754f6376ca7d807e8cf54d297c54846ce41da89 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 18:06:11 -0300
Subject: [PATCH 07/24] Add documentation

---
 contracts/access/AccessControl.sol | 117 +++++++++++++++++++++++++++--
 test/access/AccessControl.test.js  |  66 ++++++++--------
 2 files changed, 142 insertions(+), 41 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index e96b6963d10..12a15ec09f4 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -2,74 +2,175 @@ pragma solidity ^0.6.0;
 
 import "../utils/EnumerableSet.sol";
 
+/**
+ * @dev Contract module that allows children to implement role-based access
+ * control mechanisms.
+ *
+ * Roles are referred to by their `bytes32` identifier. These should be exposed
+ * in the external API and be unique. The best way to achieve this is by
+ * using `public constant` hash digests:
+ *
+ * ```
+ * bytes32 public constant MY_ROLE = keccak256("MY_ROLE");
+ * ```
+ *
+ * Roles can be used to represent a set of permissions. To restrict access to a
+ * function call, use {hasRole}:
+ *
+ * ```
+ * function foo() public {
+ *     require(hasRole(MY_ROLE, msg.sender));
+ *     ...
+ * }
+ * ```
+ *
+ * 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
+ * {revokeRole} functions. Each role has an associated admin role, and only
+ * accounts that have a role's admin role can call {grantRole} and {revokeRoke}.
+ *
+ * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
+ * that only accounts with this role will be able to grant or revoke other
+ * roles. More complex role relationships can be created by using
+ * {_setRoleAdmin}.
+ */
 abstract contract AccessControl {
     using EnumerableSet for EnumerableSet.AddressSet;
 
     struct Role {
         EnumerableSet.AddressSet members;
-        bytes32 admin;
+        bytes32 admin; // This role's admin role
     }
 
     mapping (bytes32 => Role) private _roles;
 
     bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
 
+    /**
+     * @dev Emitted when `account` is granted `roleId` role.
+     */
     event RoleGranted(bytes32 indexed roleId, address indexed account);
+
+    /**
+     * @dev Emitted when `account` is revoked the `roleId` role.
+     */
     event RoleRevoked(bytes32 indexed roleId, address indexed account);
 
-    // Returns true if an account has a role
+    /**
+     * @dev Returns `true` if `account` has the `roleId` role.
+     */
     function hasRole(bytes32 roleId, address account) public view returns (bool) {
         return _roles[roleId].members.contains(account);
     }
 
-    // Returns the number of accounts with a role
+    /**
+     * @dev Returns the number of accounts that have the `roleId` role. Can be
+     * used together with {getRoleMember} to enumerate all bearers of a role.
+     */
     function getRoleMembersCount(bytes32 roleId) public view returns (uint256) {
         return _roles[roleId].members.length();
     }
 
-    // Returns an account with a role at index
+    /**
+     * @dev Returns one of the accounts that has the `roleId` role. `index` must
+     * be a value between 0 and {getRoleMembersCount}, non-inclusive.
+     *
+     * Role bearers are not sorted in any particular way, and their ordering may
+     * change at any point.
+     *
+     * WARNING: When using {getRoleMember} and {getRoleMembersCount}, make sure
+     * you perform all queries on the same block. See the following
+     * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
+     * for more information.
+     */
     function getRoleMember(bytes32 roleId, uint256 index) public view returns (address) {
         return _roles[roleId].members.get(index);
     }
 
-    // Returns a role's admin role
+    /**
+     * @dev Returns the role identifier for the admin role that controls
+     * `roleId` role. See {grantRole} and {revokeRole}.
+     *
+     * To change a role's admin, use {_setRoleAdmin}.
+     */
     function getRoleAdmin(bytes32 roleId) external view returns (bytes32) {
         return _roles[roleId].admin;
     }
 
-    // Gives a role to an account. Caller must have its admin role
+    /**
+     * @dev Grants the `roleId` role to `account`.
+     *
+     * Calls {_grantRole} internally.
+     *
+     * Requirements:
+     *
+     * - the caller must have `roleId`'s admin role.
+     */
     function grantRole(bytes32 roleId, address account) external virtual {
         require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to grant");
 
         _grantRole(roleId, account);
     }
 
-    // Revokes a role from an account. Caller must have its admin role
+    /**
+     * @dev Revokes the `roleId` role from `account`.
+     *
+     * Calls {_revokeRole} internally.
+     *
+     * Requirements:
+     *
+     * - the caller must have `roleId`'s admin role.
+     */
     function revokeRole(bytes32 roleId, address account) external virtual {
         require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to revoke");
 
         _revokeRole(roleId, account);
     }
 
-    // Renounces a role. Caller must be `account`
+    /**
+     * @dev Revokes a role from calling account.
+     *
+     * Roles are often managed via {grantRole} and {revokeRole}: this function's
+     * purpose is to provide a mechanism for accounts to lose their privileges
+     * if they are compromised (such as when a trusted device is misplaced).
+     *
+     * Requirements:
+     *
+     * - the caller must be `account`.
+     */
     function renounceRole(bytes32 roleId, address account) external virtual {
         require(account == msg.sender, "AccessControl: can only renounce roles for self");
 
         _revokeRole(roleId, account);
     }
 
+    /**
+     * @dev Grants the `roleId` role to `account`.
+     *
+     * Emits a {RoleGranted} event.
+     */
     function _grantRole(bytes32 roleId, address account) internal virtual {
         _roles[roleId].members.add(account);
 
         emit RoleGranted(roleId, account);
     }
 
+    /**
+     * @dev Revokes the `roleId` role from `account`.
+     *
+     * Emits a {RoleRevoked} event.
+     */
     function _revokeRole(bytes32 roleId, address account) internal virtual {
         _roles[roleId].members.remove(account);
 
         emit RoleRevoked(roleId, account);
     }
 
+    /**
+     * @dev Sets `adminRoleId` as `roleId` role's admin role.
+     */
     function _setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) internal virtual {
         _roles[roleId].admin = adminRoleId;
     }
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index b349086480f..b7d1e6da65f 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -9,8 +9,8 @@ const AccessControlMock = contract.fromArtifact('AccessControlMock');
 describe('AccessControl', function () {
   const [ defaultAdmin, authorized, other ] = accounts;
 
-  const DEFAULT_ADMIN_ROLE_ID = '0x0000000000000000000000000000000000000000000000000000000000000000';
-  const OTHER_ROLE_ID = web3.utils.soliditySha3('OTHER_ROLE_ID');
+  const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
+  const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');
 
   beforeEach(async function () {
     this.accessControl = await AccessControlMock.new({ from: defaultAdmin });
@@ -18,106 +18,106 @@ describe('AccessControl', function () {
 
   describe('default admin', function () {
     it('deployer has default admin role', async function () {
-      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE_ID, defaultAdmin)).to.equal(true);
+      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.equal(true);
     });
 
     it('other roles\'s admin is the default admin role', async function () {
-      expect(await this.accessControl.getRoleAdmin(OTHER_ROLE_ID)).to.equal(DEFAULT_ADMIN_ROLE_ID);
+      expect(await this.accessControl.getRoleAdmin(OTHER_ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
     });
 
     it('default admin role\'s admin is itself', async function () {
-      expect(await this.accessControl.getRoleAdmin(DEFAULT_ADMIN_ROLE_ID)).to.equal(DEFAULT_ADMIN_ROLE_ID);
+      expect(await this.accessControl.getRoleAdmin(DEFAULT_ADMIN_ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
     });
   });
 
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
-      const receipt = await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE_ID });
+      const receipt = await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE });
 
-      expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(true);
+      expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(true);
     });
 
     it('non-admin cannot grant role to other accounts', async function () {
       await expectRevert(
-        this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: other }),
+        this.accessControl.grantRole(OTHER_ROLE, authorized, { from: other }),
         'AccessControl: sender must be an admin to grant'
       );
     });
 
     it('accounts can be granted a role multiple times', async function () {
-      await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-      const receipt = await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE_ID });
+      await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+      const receipt = await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE });
     });
   });
 
   describe('revoking', function () {
     it('roles that are not had can be revoked', async function () {
-      expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+      expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
 
-      const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+      const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
     });
 
     context('with granted role', function () {
       beforeEach(async function () {
-        await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
       });
 
       it('admin can revoke role', async function () {
-        const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+        const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
 
-        expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+        expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
       });
 
       it('non-admin cannot revoke role', async function () {
         await expectRevert(
-          this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: other }),
+          this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: other }),
           'AccessControl: sender must be an admin to revoke'
         );
       });
 
       it('a role can be revoked multiple times', async function () {
-        await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
 
-        const receipt = await this.accessControl.revokeRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+        const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
       });
     });
   });
 
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
-      const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+      const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
     });
 
     context('with granted role', function () {
       beforeEach(async function () {
-        await this.accessControl.grantRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin });
+        await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
       });
 
       it('bearer can renounce role', async function () {
-        const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+        const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
 
-        expect(await this.accessControl.hasRole(OTHER_ROLE_ID, authorized)).to.equal(false);
+        expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
       });
 
       it('only the sender can renounce their roles', async function () {
         await expectRevert(
-          this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: defaultAdmin }),
+          this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: defaultAdmin }),
           'AccessControl: can only renounce roles for self'
         );
       });
 
       it('a role can be renounced multiple times', async function () {
-        await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
+        await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
 
-        const receipt = await this.accessControl.renounceRole(OTHER_ROLE_ID, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE_ID });
+        const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
       });
     });
   });

From 4732b68d4514759a6e992d4553a9e1f698500041 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 18:16:20 -0300
Subject: [PATCH 08/24] Cleanup tests

---
 test/access/AccessControl.test.js | 66 +++++++++++++++----------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index b7d1e6da65f..baf3d629a98 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -7,22 +7,22 @@ const { expect } = require('chai');
 const AccessControlMock = contract.fromArtifact('AccessControlMock');
 
 describe('AccessControl', function () {
-  const [ defaultAdmin, authorized, other ] = accounts;
+  const [ admin, authorized, otherAuthorized, other ] = accounts;
 
   const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
-  const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');
+  const ROLE = web3.utils.soliditySha3('ROLE');
 
   beforeEach(async function () {
-    this.accessControl = await AccessControlMock.new({ from: defaultAdmin });
+    this.accessControl = await AccessControlMock.new({ from: admin });
   });
 
   describe('default admin', function () {
     it('deployer has default admin role', async function () {
-      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.equal(true);
+      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.equal(true);
     });
 
     it('other roles\'s admin is the default admin role', async function () {
-      expect(await this.accessControl.getRoleAdmin(OTHER_ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
+      expect(await this.accessControl.getRoleAdmin(ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
     });
 
     it('default admin role\'s admin is itself', async function () {
@@ -32,92 +32,92 @@ describe('AccessControl', function () {
 
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
-      const receipt = await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE });
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
 
-      expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(true);
+      expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
     });
 
     it('non-admin cannot grant role to other accounts', async function () {
       await expectRevert(
-        this.accessControl.grantRole(OTHER_ROLE, authorized, { from: other }),
+        this.accessControl.grantRole(ROLE, authorized, { from: other }),
         'AccessControl: sender must be an admin to grant'
       );
     });
 
     it('accounts can be granted a role multiple times', async function () {
-      await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-      const receipt = await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: OTHER_ROLE });
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
     });
   });
 
   describe('revoking', function () {
     it('roles that are not had can be revoked', async function () {
-      expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
+      expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
 
-      const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+      const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
     });
 
     context('with granted role', function () {
       beforeEach(async function () {
-        await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+        await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       });
 
       it('admin can revoke role', async function () {
-        const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+        const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
 
-        expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
+        expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
 
       it('non-admin cannot revoke role', async function () {
         await expectRevert(
-          this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: other }),
+          this.accessControl.revokeRole(ROLE, authorized, { from: other }),
           'AccessControl: sender must be an admin to revoke'
         );
       });
 
       it('a role can be revoked multiple times', async function () {
-        await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+        await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
 
-        const receipt = await this.accessControl.revokeRole(OTHER_ROLE, authorized, { from: defaultAdmin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+        const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
       });
     });
   });
 
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
-      const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+      const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
     });
 
     context('with granted role', function () {
       beforeEach(async function () {
-        await this.accessControl.grantRole(OTHER_ROLE, authorized, { from: defaultAdmin });
+        await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       });
 
       it('bearer can renounce role', async function () {
-        const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+        const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
 
-        expect(await this.accessControl.hasRole(OTHER_ROLE, authorized)).to.equal(false);
+        expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
 
       it('only the sender can renounce their roles', async function () {
         await expectRevert(
-          this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: defaultAdmin }),
+          this.accessControl.renounceRole(ROLE, authorized, { from: admin }),
           'AccessControl: can only renounce roles for self'
         );
       });
 
       it('a role can be renounced multiple times', async function () {
-        await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
+        await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
 
-        const receipt = await this.accessControl.renounceRole(OTHER_ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: OTHER_ROLE });
+        const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
       });
     });
   });

From c4ddbdd38041d7e2ef6f1538e4a81746e8bf8105 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 18:16:28 -0300
Subject: [PATCH 09/24] Add enumeration tests

---
 test/access/AccessControl.test.js | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index baf3d629a98..753a82fe348 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -121,4 +121,21 @@ describe('AccessControl', function () {
       });
     });
   });
+
+  describe('enumerating', function () {
+    it('role bearers can be enumerated', async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      await this.accessControl.grantRole(ROLE, otherAuthorized, { from: admin });
+
+      const memberCount = await this.accessControl.getRoleMembersCount(ROLE);
+      expect(memberCount).to.bignumber.equal('2');
+
+      let bearers = [];
+      for (let i = 0; i < memberCount; ++i) {
+        bearers.push(await this.accessControl.getRoleMember(ROLE, i));
+      }
+
+      expect(bearers).to.have.members([authorized, otherAuthorized]);
+    });
+  });
 });

From 7fb1b43e2567be1330038fe5a82c06b2db75d08a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 6 Mar 2020 18:19:23 -0300
Subject: [PATCH 10/24] Add _setRoleAdmin tests

---
 test/access/AccessControl.test.js | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 753a82fe348..a1645a6c259 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -11,6 +11,7 @@ describe('AccessControl', function () {
 
   const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
   const ROLE = web3.utils.soliditySha3('ROLE');
+  const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');
 
   beforeEach(async function () {
     this.accessControl = await AccessControlMock.new({ from: admin });
@@ -138,4 +139,21 @@ describe('AccessControl', function () {
       expect(bearers).to.have.members([authorized, otherAuthorized]);
     });
   });
+
+  describe('setting role admin', function () {
+    it('a role\'s admin role can be changed', async function () {
+      await this.accessControl.setRoleAdmin(ROLE, OTHER_ROLE);
+
+      expect(await this.accessControl.getRoleAdmin(ROLE)).to.equal(OTHER_ROLE);
+    });
+
+    it('a role\'s previous admins no longer control it', async function () {
+      await this.accessControl.setRoleAdmin(ROLE, OTHER_ROLE);
+
+      await expectRevert(
+        this.accessControl.grantRole(ROLE, authorized, { from: admin }),
+        'AccessControl: sender must be an admin to grant'
+      );
+    });
+  });
 });

From e5b1cc35174c709a9d3d6c2d331c2138ce32d516 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 9 Mar 2020 15:34:19 -0300
Subject: [PATCH 11/24] Fix lint error

---
 test/access/AccessControl.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index a1645a6c259..81ede4909f9 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -131,7 +131,7 @@ describe('AccessControl', function () {
       const memberCount = await this.accessControl.getRoleMembersCount(ROLE);
       expect(memberCount).to.bignumber.equal('2');
 
-      let bearers = [];
+      const bearers = [];
       for (let i = 0; i < memberCount; ++i) {
         bearers.push(await this.accessControl.getRoleMember(ROLE, i));
       }

From 4bc424954b187fbf17d42fd2d53fec42aed509a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 14:55:02 -0300
Subject: [PATCH 12/24] Fix AccessControl link in docs

---
 contracts/access/README.adoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc
index ec60be8da3e..dbecfdc4561 100644
--- a/contracts/access/README.adoc
+++ b/contracts/access/README.adoc
@@ -4,4 +4,4 @@ NOTE: This page is incomplete. We're working to improve it for the next release.
 
 == Library
 
-{{Roles}}
+{{AccessControl}}

From bc5c929a09939c69c81070cb4085391aa5372254 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 17:42:20 -0300
Subject: [PATCH 13/24] WIP on access control guide

---
 docs/modules/ROOT/pages/access-control.adoc | 119 ++++++++++++++------
 1 file changed, 87 insertions(+), 32 deletions(-)

diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc
index 553dab5156a..79c8f84321e 100644
--- a/docs/modules/ROOT/pages/access-control.adoc
+++ b/docs/modules/ROOT/pages/access-control.adoc
@@ -42,67 +42,122 @@ In this way you can use _composability_ to add additional layers of access contr
 [[role-based-access-control]]
 == Role-Based Access Control
 
-While the simplicity of _ownership_ can be useful for simple systems or quick prototyping, different levels of authorization are often needed. An account may be able to ban users from a system, but not create new tokens. _Role-Based Access Control (RBAC)_ offers flexibility in this regard.
+While the simplicity of _ownership_ can be useful for simple systems or quick prototyping, different levels of authorization are often needed. You may want for an account to have permission to ban users from a system, but not create new tokens. https://en.wikipedia.org/wiki/Role-based_access_control[_Role-Based Access Control (RBAC)_] offers flexibility in this regard.
 
-In essence, we will be defining multiple _roles_, each allowed to perform different sets of actions. Instead of `onlyOwner` everywhere - you will use, for example, `onlyAdminRole` in some places, and `onlyModeratorRole` in others. Separately, you will be able to define rules for how accounts can be assignned a role, transfer it, and more.
+In essence, we will be defining multiple _roles_, each allowed to perform different sets of actions. An account may have, for example, 'moderator', 'minter' or 'admin' roles, which you will then check for instead of simply using `onlyOwner`. Separately, you will be able to define rules for how accounts can be granted a role, have it revoked, and more.
 
 Most of software development uses access control systems that are role-based: some users are regular users, some may be supervisors or managers, and a few will often have administrative privileges.
 
-[[using-roles]]
-=== Using `Roles`
+[[using-access-control]]
+=== Using `AccessControl`
 
-OpenZeppelin provides xref:api:access.adoc#Roles[`Roles`] for implementing role-based access control. Its usage is straightforward: for each role that you want to define, you'll store a variable of type `Role`, which will hold the list of accounts with that role.
+OpenZeppelin Contracts provides xref:api:access.adoc#AccessControl[`AccessControl`] for implementing role-based access control. Its usage is straightforward: for each role that you want to define,
+you will create a new _role identifier_ that is used to grant, revoke, and check if an account has that role.
 
-Here's a simple example of using `Roles` in an xref:tokens.adoc#ERC20[`ERC20` token]: we'll define two roles, `minters` and `burners`, that will be able to mint new tokens, and burn them, respectively.
+Here's a simple example of using `AccessControl` in an xref:tokens.adoc#ERC20[`ERC20` token] to define a 'minter' role, which allows accounts that have it create new tokens:
 
 [source,solidity]
 ----
-pragma solidity ^0.5.0;
+pragma solidity ^0.6.0;
 
-import "@openzeppelin/contracts/access/Roles.sol";
+import "@openzeppelin/contracts/access/AccessControl.sol";
 import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
-import "@openzeppelin/contracts/token/ERC20/ERC20Detailed.sol";
 
-contract MyToken is ERC20, ERC20Detailed {
-    using Roles for Roles.Role;
+contract MyToken is ERC20, AccessControl {
+    // Create a new role identifier for the minter role
+    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
 
-    Roles.Role private _minters;
-    Roles.Role private _burners;
+    constructor(address minter) public {
+        // Grant the minter role to a specified account
+        _grantRole(MINTER_ROLE, minter);
+    }
 
-    constructor(address[] memory minters, address[] memory burners)
-        ERC20Detailed("MyToken", "MTKN", 18)
-        public
-    {
-        for (uint256 i = 0; i < minters.length; ++i) {
-            _minters.add(minters[i]);
-        }
+    function mint(address to, uint256 amount) public {
+        // Check that the calling account has the minter role
+        require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
+        _mint(to, amount);
+    }
+}
+----
+
+NOTE: Make sure you fully understand how xref:api:access.adoc#AccessControl[`AccessControl`] works before using it on your system, or copy-pasting the examples from this guide.
+
+While clear and explicit, this isn't anything we wouldn't have been able to achieve with `Ownable`. Indeed, where `AccessControl` shines is in scenarios where granular permissions are required, which can be implemented by defining _multiple_ roles.
 
-        for (uint256 i = 0; i < burners.length; ++i) {
-            _burners.add(burners[i]);
-        }
+Let's augment our ERC20 token example by also defining a 'burner' role, which lets accounts destroy tokens:
+
+[source,solidity]
+----
+pragma solidity ^0.6.0;
+
+import "@openzeppelin/contracts/access/AccessControl.sol";
+import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+contract MyToken is ERC20, AccessControl {
+    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
+    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
+
+    constructor(address minter, address burner) public {
+        _grantRole(MINTER_ROLE, minter);
+        _grantRole(BURNER_ROLE, burner);
     }
 
     function mint(address to, uint256 amount) public {
-        // Only minters can mint
-        require(_minters.has(msg.sender), "DOES_NOT_HAVE_MINTER_ROLE");
-
+        require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
         _mint(to, amount);
     }
 
     function burn(address from, uint256 amount) public {
-        // Only burners can burn
-        require(_burners.has(msg.sender), "DOES_NOT_HAVE_BURNER_ROLE");
-
+        require(hasRole(BURNER_ROLE, msg.sender), "Caller is not a burner");
        _burn(from, amount);
     }
 }
 ----
 
-So clean! By splitting concerns this way, much more granular levels of permission may be implemented than were possible with the simpler _ownership_ approach to access control. Note that an account may have more than one role, if desired.
+So clean! By splitting concerns this way, more granular levels of permission may be implemented than were possible with the simpler _ownership_ approach to access control. Limiting what each component of a system is able to do is known as the https://en.wikipedia.org/wiki/Principle_of_least_privilege[principle of least privilege], and is a good security practice. Note that each account may still have more than one role, if so desired.
+
+[[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?
+
+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_.
 
-OpenZeppelin uses `Roles` extensively with predefined contracts that encode rules for each specific role. A few examples are: xref:api:token/ERC20.adoc#ERC20Mintable[`ERC20Mintable`] which uses the xref:api:access.adoc#MinterRole[`MinterRole`] to determine who can mint tokens, and xref:api:crowdsale.adoc#WhitelistCrowdsale[`WhitelistCrowdsale`] which uses both xref:api:access.adoc#WhitelistAdminRole[`WhitelistAdminRole`] and xref:api:access.adoc#WhitelistedRole[`WhitelistedRole`] to create a set of accounts that can purchase tokens.
+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.
+
+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.
+
+Let's take a look at the ERC20 token example, this time taking advantage of the default admin role:
+
+[source,solidity]
+----
+pragma solidity ^0.6.0;
+
+import "@openzeppelin/contracts/access/AccessControl.sol";
+import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+contract MyToken is ERC20, AccessControl {
+    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
+    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
+
+    constructor() public {
+        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
+    }
+
+    function mint(address to, uint256 amount) public {
+        require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
+        _mint(to, amount);
+    }
+
+    function burn(address from, uint256 amount) public {
+        require(hasRole(BURNER_ROLE, msg.sender), "Caller is not a burner");
+       _burn(from, amount);
+    }
+}
+----
 
-This flexibility allows for interesting setups: for example, a xref:api:crowdsale.adoc#MintedCrowdsale[`MintedCrowdsale`] expects to be given the `MinterRole` of an `ERC20Mintable` in order to work, but the token contract could also extend xref:api:token/ERC20.adoc#ERC20Pausable[`ERC20Pausable`] and assign the xref:api:access.adoc#PauserRole[`PauserRole`] to a DAO that serves as a contingency mechanism in case a vulnerability is discovered in the contract code. Limiting what each component of a system is able to do is known as the https://en.wikipedia.org/wiki/Principle_of_least_privilege[principle of least privilege], and is a good security practice.
+[[querying-privileged-accounts]]
+=== Querying Privileged Accounts
 
 [[usage-in-openzeppelin]]
 == Usage in OpenZeppelin

From f8ab7d7d859aaef1bba6b43f2aff98a67f9cdbda Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 20:24:07 -0300
Subject: [PATCH 14/24] Rename getRoleMembersCount

---
 contracts/access/AccessControl.sol | 6 +++---
 test/access/AccessControl.test.js  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 12a15ec09f4..3fee1ae0fab 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -69,18 +69,18 @@ abstract contract AccessControl {
      * @dev Returns the number of accounts that have the `roleId` role. Can be
      * used together with {getRoleMember} to enumerate all bearers of a role.
      */
-    function getRoleMembersCount(bytes32 roleId) public view returns (uint256) {
+    function getRoleMemberCount(bytes32 roleId) public view returns (uint256) {
         return _roles[roleId].members.length();
     }
 
     /**
      * @dev Returns one of the accounts that has the `roleId` role. `index` must
-     * be a value between 0 and {getRoleMembersCount}, non-inclusive.
+     * be a value between 0 and {getRoleMemberCount}, non-inclusive.
      *
      * Role bearers are not sorted in any particular way, and their ordering may
      * change at any point.
      *
-     * WARNING: When using {getRoleMember} and {getRoleMembersCount}, make sure
+     * WARNING: When using {getRoleMember} and {getRoleMemberCount}, make sure
      * you perform all queries on the same block. See the following
      * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
      * for more information.
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 81ede4909f9..0b0645f098b 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -128,7 +128,7 @@ describe('AccessControl', function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       await this.accessControl.grantRole(ROLE, otherAuthorized, { from: admin });
 
-      const memberCount = await this.accessControl.getRoleMembersCount(ROLE);
+      const memberCount = await this.accessControl.getRoleMemberCount(ROLE);
       expect(memberCount).to.bignumber.equal('2');
 
       const bearers = [];

From 6e92c966473ec29456cffb1a87a8460df84194e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 20:53:30 -0300
Subject: [PATCH 15/24] Add tests for new role admin

---
 test/access/AccessControl.test.js | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 0b0645f098b..6797f3877ef 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -7,7 +7,7 @@ const { expect } = require('chai');
 const AccessControlMock = contract.fromArtifact('AccessControlMock');
 
 describe('AccessControl', function () {
-  const [ admin, authorized, otherAuthorized, other ] = accounts;
+  const [ admin, authorized, otherAuthorized, other, otherAdmin ] = accounts;
 
   const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
   const ROLE = web3.utils.soliditySha3('ROLE');
@@ -141,19 +141,37 @@ describe('AccessControl', function () {
   });
 
   describe('setting role admin', function () {
-    it('a role\'s admin role can be changed', async function () {
+    beforeEach(async function () {
       await this.accessControl.setRoleAdmin(ROLE, OTHER_ROLE);
+      await this.accessControl.grantRole(OTHER_ROLE, otherAdmin, { from: admin });
+    });
 
+    it('a role\'s admin role can be changed', async function () {
       expect(await this.accessControl.getRoleAdmin(ROLE)).to.equal(OTHER_ROLE);
     });
 
-    it('a role\'s previous admins no longer control it', async function () {
-      await this.accessControl.setRoleAdmin(ROLE, OTHER_ROLE);
+    it('the new admin can grant roles', async function () {
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
+    });
 
+    it('the new admin can revoke roles', async function () {
+      const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+    });
+
+    it('a role\'s previous admins no longer grant roles', async function () {
       await expectRevert(
         this.accessControl.grantRole(ROLE, authorized, { from: admin }),
         'AccessControl: sender must be an admin to grant'
       );
     });
+
+    it('a role\'s previous admins no longer revoke roles', async function () {
+      await expectRevert(
+        this.accessControl.revokeRole(ROLE, authorized, { from: admin }),
+        'AccessControl: sender must be an admin to revoke'
+      );
+    });
   });
 });

From ac965e74ff9da592bfec5e02e50977a9958dc644 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 21:03:09 -0300
Subject: [PATCH 16/24] Make AccessControl GSN compatible

---
 contracts/access/AccessControl.sol | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 3fee1ae0fab..9a28a27a50d 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -1,6 +1,7 @@
 pragma solidity ^0.6.0;
 
 import "../utils/EnumerableSet.sol";
+import "../GSN/Context.sol";
 
 /**
  * @dev Contract module that allows children to implement role-based access
@@ -19,7 +20,7 @@ import "../utils/EnumerableSet.sol";
  *
  * ```
  * function foo() public {
- *     require(hasRole(MY_ROLE, msg.sender));
+ *     require(hasRole(MY_ROLE, _msgSender()));
  *     ...
  * }
  * ```
@@ -36,7 +37,7 @@ import "../utils/EnumerableSet.sol";
  * roles. More complex role relationships can be created by using
  * {_setRoleAdmin}.
  */
-abstract contract AccessControl {
+abstract contract AccessControl is Context {
     using EnumerableSet for EnumerableSet.AddressSet;
 
     struct Role {
@@ -109,7 +110,7 @@ abstract contract AccessControl {
      * - the caller must have `roleId`'s admin role.
      */
     function grantRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to grant");
+        require(hasRole(_roles[roleId].admin, _msgSender()), "AccessControl: sender must be an admin to grant");
 
         _grantRole(roleId, account);
     }
@@ -124,7 +125,7 @@ abstract contract AccessControl {
      * - the caller must have `roleId`'s admin role.
      */
     function revokeRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].admin, msg.sender), "AccessControl: sender must be an admin to revoke");
+        require(hasRole(_roles[roleId].admin, _msgSender()), "AccessControl: sender must be an admin to revoke");
 
         _revokeRole(roleId, account);
     }
@@ -141,7 +142,7 @@ abstract contract AccessControl {
      * - the caller must be `account`.
      */
     function renounceRole(bytes32 roleId, address account) external virtual {
-        require(account == msg.sender, "AccessControl: can only renounce roles for self");
+        require(account == _msgSender(), "AccessControl: can only renounce roles for self");
 
         _revokeRole(roleId, account);
     }

From 0b972679918a72fb5fe55c1ffa38422a294a361d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Tue, 10 Mar 2020 21:50:09 -0300
Subject: [PATCH 17/24] Update access control guide

---
 docs/modules/ROOT/pages/access-control.adoc | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc
index 79c8f84321e..bcd4f6e2971 100644
--- a/docs/modules/ROOT/pages/access-control.adoc
+++ b/docs/modules/ROOT/pages/access-control.adoc
@@ -141,6 +141,8 @@ contract MyToken is ERC20, AccessControl {
     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
 
     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);
     }
 
@@ -156,9 +158,26 @@ contract MyToken is ERC20, AccessControl {
 }
 ----
 
+Note that, unlike the previous examples, no accounts are granted the 'minter' or 'burner' roles. However, because those roles' admin role is the default admin role, and _that_ role was granted to `msg.sender`, that same account can call `grantRole` to give minting or burning permission, and `revokeRole` to remove it.
+
+Dynamic role allocation is a often a desirable property, for example in systems where trust in a participant may vary over time. It can also be used to support use cases such as https://en.wikipedia.org/wiki/Know_your_customer[KYC], where the list of role-bearers may not be known up-front, or may be prohibitively expensive to include in a single transaction.
+
 [[querying-privileged-accounts]]
 === Querying Privileged Accounts
 
+Because accounts might <<granting-and-revoking, grant and revoke roles>> dynamically, it is not always possible to determine which accounts hold a particular role. This is important as it allows to prove certain properties about a system, such as that an administrative account is a multisig or a DAO, or that a certain role has been removed from all users, effectively disabling any associated functionality.
+
+Under the hood, `AccessControl` uses `EnumerableSet`, a more powerful variant of Solidity's `mapping` type, which allows for key enumeration. `getRoleMemberCount` can be used to retrieve the number of accounts that have a particular role, and `getRoleMember` can then be called to get the address of each of these accounts.
+
+```javascript
+const minterCount = await myToken.getRoleMemberCount(MINTER_ROLE);
+
+const members = [];
+for (let i = 0; i < minterCount; ++i) {
+    members.push(await myToken.getRoleMember(MINTER_ROLE, i));
+}
+```
+
 [[usage-in-openzeppelin]]
 == Usage in OpenZeppelin
 

From d14c6282e0a44bd1b8725cd38782195dc5c741d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 11 Mar 2020 16:08:47 -0300
Subject: [PATCH 18/24] Rename admin to adminRole

---
 contracts/access/AccessControl.sol | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 9a28a27a50d..4168da70bdf 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -42,7 +42,7 @@ abstract contract AccessControl is Context {
 
     struct Role {
         EnumerableSet.AddressSet members;
-        bytes32 admin; // This role's admin role
+        bytes32 adminRole;
     }
 
     mapping (bytes32 => Role) private _roles;
@@ -97,7 +97,7 @@ abstract contract AccessControl is Context {
      * To change a role's admin, use {_setRoleAdmin}.
      */
     function getRoleAdmin(bytes32 roleId) external view returns (bytes32) {
-        return _roles[roleId].admin;
+        return _roles[roleId].adminRole;
     }
 
     /**
@@ -110,7 +110,7 @@ abstract contract AccessControl is Context {
      * - the caller must have `roleId`'s admin role.
      */
     function grantRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].admin, _msgSender()), "AccessControl: sender must be an admin to grant");
+        require(hasRole(_roles[roleId].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
 
         _grantRole(roleId, account);
     }
@@ -125,7 +125,7 @@ abstract contract AccessControl is Context {
      * - the caller must have `roleId`'s admin role.
      */
     function revokeRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].admin, _msgSender()), "AccessControl: sender must be an admin to revoke");
+        require(hasRole(_roles[roleId].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");
 
         _revokeRole(roleId, account);
     }
@@ -173,6 +173,6 @@ abstract contract AccessControl is Context {
      * @dev Sets `adminRoleId` as `roleId` role's admin role.
      */
     function _setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) internal virtual {
-        _roles[roleId].admin = adminRoleId;
+        _roles[roleId].adminRole = adminRoleId;
     }
 }

From 8758632bbf96cd525894aaf8cb25b936eb6e4beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 11 Mar 2020 16:13:54 -0300
Subject: [PATCH 19/24] Rename roleIds to roles

---
 contracts/access/AccessControl.sol | 90 +++++++++++++++---------------
 test/access/AccessControl.test.js  | 20 +++----
 2 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 4168da70bdf..61a709a45f1 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -40,43 +40,43 @@ import "../GSN/Context.sol";
 abstract contract AccessControl is Context {
     using EnumerableSet for EnumerableSet.AddressSet;
 
-    struct Role {
+    struct RoleData {
         EnumerableSet.AddressSet members;
         bytes32 adminRole;
     }
 
-    mapping (bytes32 => Role) private _roles;
+    mapping (bytes32 => RoleData) private _roles;
 
     bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
 
     /**
-     * @dev Emitted when `account` is granted `roleId` role.
+     * @dev Emitted when `account` is granted `role`.
      */
-    event RoleGranted(bytes32 indexed roleId, address indexed account);
+    event RoleGranted(bytes32 indexed role, address indexed account);
 
     /**
-     * @dev Emitted when `account` is revoked the `roleId` role.
+     * @dev Emitted when `account` is revoked `role`.
      */
-    event RoleRevoked(bytes32 indexed roleId, address indexed account);
+    event RoleRevoked(bytes32 indexed role, address indexed account);
 
     /**
-     * @dev Returns `true` if `account` has the `roleId` role.
+     * @dev Returns `true` if `account` has been granted `role`.
      */
-    function hasRole(bytes32 roleId, address account) public view returns (bool) {
-        return _roles[roleId].members.contains(account);
+    function hasRole(bytes32 role, address account) public view returns (bool) {
+        return _roles[role].members.contains(account);
     }
 
     /**
-     * @dev Returns the number of accounts that have the `roleId` role. Can be
-     * used together with {getRoleMember} to enumerate all bearers of a role.
+     * @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 roleId) public view returns (uint256) {
-        return _roles[roleId].members.length();
+    function getRoleMemberCount(bytes32 role) public view returns (uint256) {
+        return _roles[role].members.length();
     }
 
     /**
-     * @dev Returns one of the accounts that has the `roleId` role. `index` must
-     * be a value between 0 and {getRoleMemberCount}, non-inclusive.
+     * @dev Returns one of the accounts that have `role`. `index` must be a
+     * value between 0 and {getRoleMemberCount}, non-inclusive.
      *
      * Role bearers are not sorted in any particular way, and their ordering may
      * change at any point.
@@ -86,52 +86,52 @@ abstract contract AccessControl is Context {
      * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
      * for more information.
      */
-    function getRoleMember(bytes32 roleId, uint256 index) public view returns (address) {
-        return _roles[roleId].members.get(index);
+    function getRoleMember(bytes32 role, uint256 index) public view returns (address) {
+        return _roles[role].members.get(index);
     }
 
     /**
-     * @dev Returns the role identifier for the admin role that controls
-     * `roleId` role. See {grantRole} and {revokeRole}.
+     * @dev Returns the admin role that controls `role`. See {grantRole} and
+     * {revokeRole}.
      *
      * To change a role's admin, use {_setRoleAdmin}.
      */
-    function getRoleAdmin(bytes32 roleId) external view returns (bytes32) {
-        return _roles[roleId].adminRole;
+    function getRoleAdmin(bytes32 role) external view returns (bytes32) {
+        return _roles[role].adminRole;
     }
 
     /**
-     * @dev Grants the `roleId` role to `account`.
+     * @dev Grants `role` to `account`.
      *
      * Calls {_grantRole} internally.
      *
      * Requirements:
      *
-     * - the caller must have `roleId`'s admin role.
+     * - the caller must have `role`'s admin role.
      */
-    function grantRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
+    function grantRole(bytes32 role, address account) external virtual {
+        require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
 
-        _grantRole(roleId, account);
+        _grantRole(role, account);
     }
 
     /**
-     * @dev Revokes the `roleId` role from `account`.
+     * @dev Revokes `role` from `account`.
      *
      * Calls {_revokeRole} internally.
      *
      * Requirements:
      *
-     * - the caller must have `roleId`'s admin role.
+     * - the caller must have `role`'s admin role.
      */
-    function revokeRole(bytes32 roleId, address account) external virtual {
-        require(hasRole(_roles[roleId].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");
+    function revokeRole(bytes32 role, address account) external virtual {
+        require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");
 
-        _revokeRole(roleId, account);
+        _revokeRole(role, account);
     }
 
     /**
-     * @dev Revokes a role from calling account.
+     * @dev Revokes `role` from the calling account.
      *
      * Roles are often managed via {grantRole} and {revokeRole}: this function's
      * purpose is to provide a mechanism for accounts to lose their privileges
@@ -141,38 +141,38 @@ abstract contract AccessControl is Context {
      *
      * - the caller must be `account`.
      */
-    function renounceRole(bytes32 roleId, address account) external virtual {
+    function renounceRole(bytes32 role, address account) external virtual {
         require(account == _msgSender(), "AccessControl: can only renounce roles for self");
 
-        _revokeRole(roleId, account);
+        _revokeRole(role, account);
     }
 
     /**
-     * @dev Grants the `roleId` role to `account`.
+     * @dev Grants `role` to `account`.
      *
      * Emits a {RoleGranted} event.
      */
-    function _grantRole(bytes32 roleId, address account) internal virtual {
-        _roles[roleId].members.add(account);
+    function _grantRole(bytes32 role, address account) internal virtual {
+        _roles[role].members.add(account);
 
-        emit RoleGranted(roleId, account);
+        emit RoleGranted(role, account);
     }
 
     /**
-     * @dev Revokes the `roleId` role from `account`.
+     * @dev Revokes `role` from `account`.
      *
      * Emits a {RoleRevoked} event.
      */
-    function _revokeRole(bytes32 roleId, address account) internal virtual {
-        _roles[roleId].members.remove(account);
+    function _revokeRole(bytes32 role, address account) internal virtual {
+        _roles[role].members.remove(account);
 
-        emit RoleRevoked(roleId, account);
+        emit RoleRevoked(role, account);
     }
 
     /**
-     * @dev Sets `adminRoleId` as `roleId` role's admin role.
+     * @dev Sets `adminRole` as `role`'s admin role.
      */
-    function _setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) internal virtual {
-        _roles[roleId].adminRole = adminRoleId;
+    function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual {
+        _roles[role].adminRole = adminRole;
     }
 }
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 6797f3877ef..c9cf57592a1 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -34,7 +34,7 @@ describe('AccessControl', function () {
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
 
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
     });
@@ -49,7 +49,7 @@ describe('AccessControl', function () {
     it('accounts can be granted a role multiple times', async function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
     });
   });
 
@@ -58,7 +58,7 @@ describe('AccessControl', function () {
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
 
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
     });
 
     context('with granted role', function () {
@@ -68,7 +68,7 @@ describe('AccessControl', function () {
 
       it('admin can revoke role', async function () {
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -84,7 +84,7 @@ describe('AccessControl', function () {
         await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
 
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
       });
     });
   });
@@ -92,7 +92,7 @@ describe('AccessControl', function () {
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
       const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
     });
 
     context('with granted role', function () {
@@ -102,7 +102,7 @@ describe('AccessControl', function () {
 
       it('bearer can renounce role', async function () {
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -118,7 +118,7 @@ describe('AccessControl', function () {
         await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
 
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
       });
     });
   });
@@ -152,12 +152,12 @@ describe('AccessControl', function () {
 
     it('the new admin can grant roles', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
     });
 
     it('the new admin can revoke roles', async function () {
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, roleId: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
     });
 
     it('a role\'s previous admins no longer grant roles', async function () {

From eea19030842e2bd332efa45fc1623f8c7de2ca18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 11 Mar 2020 16:31:08 -0300
Subject: [PATCH 20/24] Add 'operator' to RoleGranted and RoleRevoked events.

---
 contracts/access/AccessControl.sol | 17 +++++++++++++----
 test/access/AccessControl.test.js  | 20 ++++++++++----------
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 61a709a45f1..a77ddddf107 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -51,13 +51,22 @@ abstract contract AccessControl is Context {
 
     /**
      * @dev Emitted when `account` is granted `role`.
+     *
+     * `operator` 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
      */
-    event RoleGranted(bytes32 indexed role, address indexed account);
+    event RoleGranted(bytes32 indexed role, address indexed account, address indexed operator);
 
     /**
      * @dev Emitted when `account` is revoked `role`.
+     *
+     * `operator` 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);
+    event RoleRevoked(bytes32 indexed role, address indexed account, address indexed operator);
 
     /**
      * @dev Returns `true` if `account` has been granted `role`.
@@ -155,7 +164,7 @@ abstract contract AccessControl is Context {
     function _grantRole(bytes32 role, address account) internal virtual {
         _roles[role].members.add(account);
 
-        emit RoleGranted(role, account);
+        emit RoleGranted(role, account, msg.sender);
     }
 
     /**
@@ -166,7 +175,7 @@ abstract contract AccessControl is Context {
     function _revokeRole(bytes32 role, address account) internal virtual {
         _roles[role].members.remove(account);
 
-        emit RoleRevoked(role, account);
+        emit RoleRevoked(role, account, msg.sender);
     }
 
     /**
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index c9cf57592a1..48a53d88a04 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -34,7 +34,7 @@ describe('AccessControl', function () {
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: admin });
 
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
     });
@@ -49,7 +49,7 @@ describe('AccessControl', function () {
     it('accounts can be granted a role multiple times', async function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: admin });
     });
   });
 
@@ -58,7 +58,7 @@ describe('AccessControl', function () {
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
 
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
     });
 
     context('with granted role', function () {
@@ -68,7 +68,7 @@ describe('AccessControl', function () {
 
       it('admin can revoke role', async function () {
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -84,7 +84,7 @@ describe('AccessControl', function () {
         await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
 
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
       });
     });
   });
@@ -92,7 +92,7 @@ describe('AccessControl', function () {
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
       const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
     });
 
     context('with granted role', function () {
@@ -102,7 +102,7 @@ describe('AccessControl', function () {
 
       it('bearer can renounce role', async function () {
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -118,7 +118,7 @@ describe('AccessControl', function () {
         await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
 
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
       });
     });
   });
@@ -152,12 +152,12 @@ describe('AccessControl', function () {
 
     it('the new admin can grant roles', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: otherAdmin });
     });
 
     it('the new admin can revoke roles', async function () {
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: otherAdmin });
     });
 
     it('a role\'s previous admins no longer grant roles', async function () {

From 37d4adf29441a784430aacfe07e002b2ee561149 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 11 Mar 2020 17:07:30 -0300
Subject: [PATCH 21/24] Only emit events if the roles were not previously
 granted/revoked

---
 contracts/access/AccessControl.sol | 17 +++++++++--------
 test/access/AccessControl.test.js  | 11 ++++++-----
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index a77ddddf107..3b162c2f549 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -159,23 +159,24 @@ abstract contract AccessControl is Context {
     /**
      * @dev Grants `role` to `account`.
      *
-     * Emits a {RoleGranted} event.
+     * If `account` had not been already granted `role`, emits a {RoleGranted}
+     * event.
      */
     function _grantRole(bytes32 role, address account) internal virtual {
-        _roles[role].members.add(account);
-
-        emit RoleGranted(role, account, msg.sender);
+        if (_roles[role].members.add(account)) {
+            emit RoleGranted(role, account, msg.sender);
+        }
     }
 
     /**
      * @dev Revokes `role` from `account`.
      *
-     * Emits a {RoleRevoked} event.
+     * If `account` had been granted `role`, emits a {RoleRevoked} event.
      */
     function _revokeRole(bytes32 role, address account) internal virtual {
-        _roles[role].members.remove(account);
-
-        emit RoleRevoked(role, account, msg.sender);
+        if (_roles[role].members.remove(account)) {
+            emit RoleRevoked(role, account, msg.sender);
+        }
     }
 
     /**
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 48a53d88a04..27b4667ae35 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -49,7 +49,7 @@ describe('AccessControl', function () {
     it('accounts can be granted a role multiple times', async function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: admin });
+      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleGranted');
     });
   });
 
@@ -58,7 +58,7 @@ describe('AccessControl', function () {
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
 
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
+      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
     });
 
     context('with granted role', function () {
@@ -84,7 +84,7 @@ describe('AccessControl', function () {
         await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
 
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
+        //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
       });
     });
   });
@@ -92,7 +92,7 @@ describe('AccessControl', function () {
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
       const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
+      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
     });
 
     context('with granted role', function () {
@@ -118,7 +118,7 @@ describe('AccessControl', function () {
         await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
 
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
+        //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
       });
     });
   });
@@ -156,6 +156,7 @@ describe('AccessControl', function () {
     });
 
     it('the new admin can revoke roles', async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
       expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: otherAdmin });
     });

From 71f8da2afab4f9ed852b0105a8a454ebd4a2d087 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 12 Mar 2020 16:18:46 -0300
Subject: [PATCH 22/24] Uncomment expectEvent.not tests

---
 package-lock.json                 | 24 ++++++++++++------------
 package.json                      |  2 +-
 test/access/AccessControl.test.js | 10 +++++-----
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 8a7aac90fd2..347709d0e65 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1779,16 +1779,16 @@
       }
     },
     "@openzeppelin/test-helpers": {
-      "version": "0.5.4",
-      "resolved": "https://registry.npmjs.org/@openzeppelin/test-helpers/-/test-helpers-0.5.4.tgz",
-      "integrity": "sha512-sSjft0CcmC6Pwsd/j1uakk2MLamEH4mmphVlF5hzUUVyoxL0KGCbxQgBoLoEpUbw2M9HtFAEMJJPGccEcb9Cog==",
+      "version": "0.5.5",
+      "resolved": "https://registry.npmjs.org/@openzeppelin/test-helpers/-/test-helpers-0.5.5.tgz",
+      "integrity": "sha512-jTSCQojQ0Q7FBMN3Me7o0OIVuRnfHRR9TcE+ZlfbSfdqrHkFLwSfeDHSNWtQGlF1xPQR5r3iRI0ccsCrN+JblA==",
       "dev": true,
       "requires": {
         "@openzeppelin/contract-loader": "^0.4.0",
         "@truffle/contract": "^4.0.35",
         "ansi-colors": "^3.2.3",
         "chai": "^4.2.0",
-        "chai-bn": "^0.2.0",
+        "chai-bn": "^0.2.1",
         "ethjs-abi": "^0.2.1",
         "lodash.flatten": "^4.4.0",
         "semver": "^5.6.0",
@@ -5017,9 +5017,9 @@
       }
     },
     "chai-bn": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/chai-bn/-/chai-bn-0.2.0.tgz",
-      "integrity": "sha512-h+XqIFikre13N3uiZSc50PZ0VztVjuD/Gytle07EUFkbd8z31tWp37DLAsR1dPozmOLA3yu4hi3IFjJDQ5CKBg==",
+      "version": "0.2.1",
+      "resolved": "https://registry.npmjs.org/chai-bn/-/chai-bn-0.2.1.tgz",
+      "integrity": "sha512-01jt2gSXAw7UYFPT5K8d7HYjdXj2vyeIuE+0T/34FWzlNcVbs1JkPxRu7rYMfQnJhrHT8Nr6qjSf5ZwwLU2EYg==",
       "dev": true
     },
     "chalk": {
@@ -32265,8 +32265,8 @@
       }
     },
     "openzeppelin-docs-utils": {
-      "version": "github:OpenZeppelin/docs-utils#459f1710a07ec02cbd683cab4800c32a07aed274",
-      "from": "github:OpenZeppelin/docs-utils#459f1710a07ec02cbd683cab4800c32a07aed274",
+      "version": "github:OpenZeppelin/docs-utils#dbdcce52e45c89f67f07683c536baf4b541ba68a",
+      "from": "github:OpenZeppelin/docs-utils",
       "dev": true,
       "requires": {
         "chalk": "^3.0.0",
@@ -32322,9 +32322,9 @@
           "dev": true
         },
         "minimist": {
-          "version": "1.2.0",
-          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz",
-          "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=",
+          "version": "1.2.4",
+          "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.4.tgz",
+          "integrity": "sha512-wTiNDqe4D2rbTJGZk1qcdZgFtY0/r+iuE6GDT7V0/+Gu5MLpIDm4+CssDECR79OJs/OxLPXMzdxy153b5Qy3hg==",
           "dev": true
         },
         "supports-color": {
diff --git a/package.json b/package.json
index e8c1361f145..f70a1d3595a 100644
--- a/package.json
+++ b/package.json
@@ -48,7 +48,7 @@
     "@openzeppelin/gsn-helpers": "^0.2.3",
     "@openzeppelin/gsn-provider": "^0.1.9",
     "@openzeppelin/test-environment": "^0.1.3",
-    "@openzeppelin/test-helpers": "^0.5.4",
+    "@openzeppelin/test-helpers": "^0.5.5",
     "chai": "^4.2.0",
     "eslint": "^6.5.1",
     "eslint-config-standard": "^14.1.0",
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 27b4667ae35..193892c4015 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -49,7 +49,7 @@ describe('AccessControl', function () {
     it('accounts can be granted a role multiple times', async function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: admin });
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleGranted');
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleGranted');
     });
   });
 
@@ -58,7 +58,7 @@ describe('AccessControl', function () {
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
 
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
     });
 
     context('with granted role', function () {
@@ -84,7 +84,7 @@ describe('AccessControl', function () {
         await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
 
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+        await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
       });
     });
   });
@@ -92,7 +92,7 @@ describe('AccessControl', function () {
   describe('renouncing', function () {
     it('roles that are not had can be renounced', async function () {
       const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-      //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
     });
 
     context('with granted role', function () {
@@ -118,7 +118,7 @@ describe('AccessControl', function () {
         await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
 
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        //await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+        await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
       });
     });
   });

From d6bad63d4fd3cbfd3037f7dff04b1a377eae9ab3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Fri, 13 Mar 2020 16:21:31 -0300
Subject: [PATCH 23/24] Rename operator to sender

---
 contracts/access/AccessControl.sol |  8 ++++----
 test/access/AccessControl.test.js  | 10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 3b162c2f549..6ab3beb408b 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -52,21 +52,21 @@ abstract contract AccessControl is Context {
     /**
      * @dev Emitted when `account` is granted `role`.
      *
-     * `operator` is the account that originated the contract call:
+     * `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
      */
-    event RoleGranted(bytes32 indexed role, address indexed account, address indexed operator);
+    event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
 
     /**
      * @dev Emitted when `account` is revoked `role`.
      *
-     * `operator` is the account that originated the contract call:
+     * `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 operator);
+    event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
 
     /**
      * @dev Returns `true` if `account` has been granted `role`.
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index 193892c4015..c49276f925d 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -34,7 +34,7 @@ describe('AccessControl', function () {
   describe('granting', function () {
     it('admin can grant role to other accounts', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: admin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, sender: admin });
 
       expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
     });
@@ -68,7 +68,7 @@ describe('AccessControl', function () {
 
       it('admin can revoke role', async function () {
         const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: admin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: admin });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -102,7 +102,7 @@ describe('AccessControl', function () {
 
       it('bearer can renounce role', async function () {
         const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
-        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: authorized });
 
         expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
       });
@@ -152,13 +152,13 @@ describe('AccessControl', function () {
 
     it('the new admin can grant roles', async function () {
       const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, operator: otherAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, sender: otherAdmin });
     });
 
     it('the new admin can revoke roles', async function () {
       await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
       const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
-      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, operator: otherAdmin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: otherAdmin });
     });
 
     it('a role\'s previous admins no longer grant roles', async function () {

From 58fd0bea143c6d309c413515c835951d18b73134 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 16 Mar 2020 13:44:27 -0300
Subject: [PATCH 24/24] Add changelog entry

---
 CHANGELOG.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b446128f402..a5c09ee8b3f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # Changelog
 
+## 3.0.0 (unreleased)
+
+### New features
+ * `AccessControl`: new contract for managing permissions in a system, replacement for `Ownable` and `Roles`. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
+
+### Breaking changes
+ * `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
+
 ## 2.5.0 (2020-02-04)
 
 ### New features