From 9b6d89313b8c46587ad0f82215c2a6c8e0095a29 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:20:07 -0300
Subject: [PATCH 01/11] Remove _grantRole and _revokeRole, replace with
 _setupRole

---
 contracts/access/AccessControl.sol          | 50 +++++++++++----------
 contracts/mocks/AccessControlMock.sol       |  2 +-
 docs/modules/ROOT/pages/access-control.adoc | 12 ++---
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 8f82c1bddbc..6fbe62c8871 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -25,10 +25,7 @@ import "../GSN/Context.sol";
  * }
  * ```
  *
- * Roles can be granted and revoked programatically by calling the `internal`
- * {_grantRole} and {_revokeRole} functions.
- *
- * This can also be achieved dynamically via the `external` {grantRole} and
+ * Roles can be granted and revoked dynamically via the {grantRole} and
  * {revokeRole} functions. Each role has an associated admin role, and only
  * accounts that have a role's admin role can call {grantRole} and {revokeRoke}.
  *
@@ -52,9 +49,8 @@ abstract contract AccessControl is Context {
     /**
      * @dev Emitted when `account` is granted `role`.
      *
-     * `sender` is the account that originated the contract call:
-     *   - if using `grantRole`, it is the admin role bearer
-     *   - if using `_grantRole`, its meaning is system-dependent
+     * `sender` is the account that originated the contract call, an admin role
+     * bearer except when using {_setupRole}.
      */
     event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
 
@@ -64,7 +60,6 @@ abstract contract AccessControl is Context {
      * `sender` is the account that originated the contract call:
      *   - if using `revokeRole`, it is the admin role bearer
      *   - if using `renounceRole`, it is the role bearer (i.e. `account`)
-     *   - if using `_renounceRole`, its meaning is system-dependent
      */
     event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
 
@@ -112,7 +107,8 @@ abstract contract AccessControl is Context {
     /**
      * @dev Grants `role` to `account`.
      *
-     * Calls {_grantRole} internally.
+     * If `account` had not been already granted `role`, emits a {RoleGranted}
+     * event.
      *
      * Requirements:
      *
@@ -127,7 +123,7 @@ abstract contract AccessControl is Context {
     /**
      * @dev Revokes `role` from `account`.
      *
-     * Calls {_revokeRole} internally.
+     * If `account` had been granted `role`, emits a {RoleRevoked} event.
      *
      * Requirements:
      *
@@ -146,6 +142,9 @@ abstract contract AccessControl is Context {
      * purpose is to provide a mechanism for accounts to lose their privileges
      * if they are compromised (such as when a trusted device is misplaced).
      *
+     * If the calling account had been granted `role`, emits a {RoleRevoked}
+     * event.
+     *
      * Requirements:
      *
      * - the caller must be `account`.
@@ -161,22 +160,13 @@ abstract contract AccessControl is Context {
      *
      * If `account` had not been already granted `role`, emits a {RoleGranted}
      * event.
-     */
-    function _grantRole(bytes32 role, address account) internal virtual {
-        if (_roles[role].members.add(account)) {
-            emit RoleGranted(role, account, _msgSender());
-        }
-    }
-
-    /**
-     * @dev Revokes `role` from `account`.
      *
-     * If `account` had been granted `role`, emits a {RoleRevoked} event.
+     * NOTE: Unlike {grantRole}, this function doesn't perform any checks on the
+     * calling account. Its usage should be restricted to grating the initial
+     * set of rules during contract construction.
      */
-    function _revokeRole(bytes32 role, address account) internal virtual {
-        if (_roles[role].members.remove(account)) {
-            emit RoleRevoked(role, account, _msgSender());
-        }
+    function _setupRole(bytes32 role, address account) internal virtual {
+        _grantRole(role, account);
     }
 
     /**
@@ -185,4 +175,16 @@ abstract contract AccessControl is Context {
     function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual {
         _roles[role].adminRole = adminRole;
     }
+
+    function _grantRole(bytes32 role, address account) private {
+        if (_roles[role].members.add(account)) {
+            emit RoleGranted(role, account, _msgSender());
+        }
+    }
+
+    function _revokeRole(bytes32 role, address account) private {
+        if (_roles[role].members.remove(account)) {
+            emit RoleRevoked(role, account, _msgSender());
+        }
+    }
 }
diff --git a/contracts/mocks/AccessControlMock.sol b/contracts/mocks/AccessControlMock.sol
index fe89d00f61b..4f96be5d5ff 100644
--- a/contracts/mocks/AccessControlMock.sol
+++ b/contracts/mocks/AccessControlMock.sol
@@ -4,7 +4,7 @@ import "../access/AccessControl.sol";
 
 contract AccessControlMock is AccessControl {
     constructor() public {
-        _grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
+        _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
     }
 
     function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc
index f5137a0b3f0..d2ad728a88b 100644
--- a/docs/modules/ROOT/pages/access-control.adoc
+++ b/docs/modules/ROOT/pages/access-control.adoc
@@ -69,7 +69,7 @@ contract MyToken is ERC20, AccessControl {
 
     constructor(address minter) public {
         // Grant the minter role to a specified account
-        _grantRole(MINTER_ROLE, minter);
+        _setupRole(MINTER_ROLE, minter);
     }
 
     function mint(address to, uint256 amount) public {
@@ -98,8 +98,8 @@ contract MyToken is ERC20, AccessControl {
     bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
 
     constructor(address minter, address burner) public {
-        _grantRole(MINTER_ROLE, minter);
-        _grantRole(BURNER_ROLE, burner);
+        _setupRole(MINTER_ROLE, minter);
+        _setupRole(BURNER_ROLE, burner);
     }
 
     function mint(address to, uint256 amount) public {
@@ -119,11 +119,11 @@ So clean! By splitting concerns this way, more granular levels of permission may
 [[granting-and-revoking]]
 === Granting and Revoking Roles
 
-The ERC20 token example above uses `\_grantRole`, an `internal` function that is useful when programmatically asigning roles (such as during construction). But what if we later want to grant the 'minter' role to additional accounts?
+The ERC20 token example above uses `\_setupRole`, an `internal` function that is useful when programmatically asigning roles (such as during construction). But what if we later want to grant the 'minter' role to additional accounts?
 
 By default, **accounts with a role cannot grant it or revoke it from other accounts**: all having a role does is making the `hasRole` check pass. To grant and revoke roles dynamically, you will need help from the _role's admin_.
 
-Every role has an associated admin role, which grants permission to call the `grantRole` and `revokeRole` `external` functions. A role can be granted or revoked by using these if the calling account has the corresponding admin role. Multiple roles may have the same admin role to make management easier. A role's admin can even be the same role itself, which would cause accounts with that role to be able to also grant and revoke it.
+Every role has an associated admin role, which grants permission to call the `grantRole` and `revokeRole` functions. A role can be granted or revoked by using these if the calling account has the corresponding admin role. Multiple roles may have the same admin role to make management easier. A role's admin can even be the same role itself, which would cause accounts with that role to be able to also grant and revoke it.
 
 This mechanism can be used to create complex permissioning structures resembling organizational charts, but it also provides an easy way to manage simpler applications. `AccessControl` includes a special role, called `DEFAULT_ADMIN_ROLE`, which acts as the **default admin role for all roles**. An account with this role will be able to manage any other role, unless `\_setRoleAdmin` is used to select a new admin role.
 
@@ -143,7 +143,7 @@ contract MyToken is ERC20, AccessControl {
     constructor() public {
         // Grant the contract deployer the default admin role: it will be able
         // to grant and revoke any roles
-        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
+        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
     }
 
     function mint(address to, uint256 amount) public {

From dcdae3e869ab193049f1a2337e6e61b1fe4ec3d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:20:53 -0300
Subject: [PATCH 02/11] Make all external AccessControl functions public

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

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 6fbe62c8871..d1163a5c989 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -100,7 +100,7 @@ abstract contract AccessControl is Context {
      *
      * To change a role's admin, use {_setRoleAdmin}.
      */
-    function getRoleAdmin(bytes32 role) external view returns (bytes32) {
+    function getRoleAdmin(bytes32 role) public view returns (bytes32) {
         return _roles[role].adminRole;
     }
 
@@ -114,7 +114,7 @@ abstract contract AccessControl is Context {
      *
      * - the caller must have `role`'s admin role.
      */
-    function grantRole(bytes32 role, address account) external virtual {
+    function grantRole(bytes32 role, address account) public virtual {
         require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
 
         _grantRole(role, account);
@@ -129,7 +129,7 @@ abstract contract AccessControl is Context {
      *
      * - the caller must have `role`'s admin role.
      */
-    function revokeRole(bytes32 role, address account) external virtual {
+    function revokeRole(bytes32 role, address account) public virtual {
         require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");
 
         _revokeRole(role, account);
@@ -149,7 +149,7 @@ abstract contract AccessControl is Context {
      *
      * - the caller must be `account`.
      */
-    function renounceRole(bytes32 role, address account) external virtual {
+    function renounceRole(bytes32 role, address account) public virtual {
         require(account == _msgSender(), "AccessControl: can only renounce roles for self");
 
         _revokeRole(role, account);

From 399d2b032add0902908735ccd108b328989da813 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:21:50 -0300
Subject: [PATCH 03/11] Remove Ownable._transferOwnership

---
 contracts/access/Ownable.sol | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol
index bea305ba52f..738493f8e73 100644
--- a/contracts/access/Ownable.sol
+++ b/contracts/access/Ownable.sol
@@ -59,13 +59,6 @@ contract Ownable is Context {
      * Can only be called by the current owner.
      */
     function transferOwnership(address newOwner) public virtual onlyOwner {
-        _transferOwnership(newOwner);
-    }
-
-    /**
-     * @dev Transfers ownership of the contract to a new account (`newOwner`).
-     */
-    function _transferOwnership(address newOwner) internal virtual {
         require(newOwner != address(0), "Ownable: new owner is the zero address");
         emit OwnershipTransferred(_owner, newOwner);
         _owner = newOwner;

From 376c36f05ec47f16d2fe64b3fc50833e189bc78e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:23:28 -0300
Subject: [PATCH 04/11] Rename ERC721's _safeTransferFrom and _transferFrom to
 _safeTransfer and _transfer

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

diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
index 0a39c084b98..879bda4abf3 100644
--- a/contracts/token/ERC721/ERC721.sol
+++ b/contracts/token/ERC721/ERC721.sol
@@ -269,7 +269,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
         //solhint-disable-next-line max-line-length
         require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved");
 
-        _transferFrom(from, to, tokenId);
+        _transfer(from, to, tokenId);
     }
 
     /**
@@ -301,7 +301,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      */
     function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) public virtual override {
         require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved");
-        _safeTransferFrom(from, to, tokenId, _data);
+        _safeTransfer(from, to, tokenId, _data);
     }
 
     /**
@@ -316,8 +316,8 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      * @param tokenId uint256 ID of the token to be transferred
      * @param _data bytes data to send along with a safe transfer check
      */
-    function _safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) internal virtual {
-        _transferFrom(from, to, tokenId);
+    function _safeTransfer(address from, address to, uint256 tokenId, bytes memory _data) internal virtual {
+        _transfer(from, to, tokenId);
         require(_checkOnERC721Received(from, to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");
     }
 
@@ -432,7 +432,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      * @param to address to receive the ownership of the given token ID
      * @param tokenId uint256 ID of the token to be transferred
      */
-    function _transferFrom(address from, address to, uint256 tokenId) internal virtual {
+    function _transfer(address from, address to, uint256 tokenId) internal virtual {
         require(ownerOf(tokenId) == from, "ERC721: transfer of token that is not own");
         require(to != address(0), "ERC721: transfer to the zero address");
 

From bf8993ae372c9959e987fa24ac87ccfd8b7bac7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:24:17 -0300
Subject: [PATCH 05/11] Make all ERC721 external functions public

---
 contracts/token/ERC721/ERC721.sol | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
index 879bda4abf3..ad6cdb89add 100644
--- a/contracts/token/ERC721/ERC721.sol
+++ b/contracts/token/ERC721/ERC721.sol
@@ -130,7 +130,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      * @dev Gets the token name.
      * @return string representing the token name
      */
-    function name() external view override returns (string memory) {
+    function name() public view override returns (string memory) {
         return _name;
     }
 
@@ -138,7 +138,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      * @dev Gets the token symbol.
      * @return string representing the token symbol
      */
-    function symbol() external view override returns (string memory) {
+    function symbol() public view override returns (string memory) {
         return _symbol;
     }
 
@@ -150,7 +150,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
      *
      * Reverts if the token ID does not exist.
      */
-    function tokenURI(uint256 tokenId) external view override returns (string memory) {
+    function tokenURI(uint256 tokenId) public view override returns (string memory) {
         require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token");
 
         string memory _tokenURI = _tokenURIs[tokenId];
@@ -169,7 +169,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
     * automatically added as a preffix in {tokenURI} to each token's URI, when
     * they are non-empty.
     */
-    function baseURI() external view returns (string memory) {
+    function baseURI() public view returns (string memory) {
         return _baseURI;
     }
 

From 71a75c694c72a6f0edae9b327b9670b19336e602 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 18:26:26 -0300
Subject: [PATCH 06/11] Make all miscelaneous external functions public instead

---
 contracts/GSN/GSNRecipient.sol                       | 4 ++--
 contracts/GSN/GSNRecipientERC20Fee.sol               | 2 +-
 contracts/GSN/GSNRecipientSignature.sol              | 2 +-
 contracts/introspection/ERC165.sol                   | 2 +-
 contracts/introspection/ERC1820Implementer.sol       | 2 +-
 contracts/mocks/ERC165/ERC165InterfacesSupported.sol | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/contracts/GSN/GSNRecipient.sol b/contracts/GSN/GSNRecipient.sol
index 50e0b111b52..66775005f71 100644
--- a/contracts/GSN/GSNRecipient.sol
+++ b/contracts/GSN/GSNRecipient.sol
@@ -119,7 +119,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      *
      * - the caller must be the `RelayHub` contract.
      */
-    function preRelayedCall(bytes calldata context) external virtual override returns (bytes32) {
+    function preRelayedCall(bytes calldata context) public virtual override returns (bytes32) {
         require(msg.sender == getHubAddr(), "GSNRecipient: caller is not RelayHub");
         return _preRelayedCall(context);
     }
@@ -142,7 +142,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      *
      * - the caller must be the `RelayHub` contract.
      */
-    function postRelayedCall(bytes calldata context, bool success, uint256 actualCharge, bytes32 preRetVal) external virtual override {
+    function postRelayedCall(bytes calldata context, bool success, uint256 actualCharge, bytes32 preRetVal) public virtual override {
         require(msg.sender == getHubAddr(), "GSNRecipient: caller is not RelayHub");
         _postRelayedCall(context, success, actualCharge, preRetVal);
     }
diff --git a/contracts/GSN/GSNRecipientERC20Fee.sol b/contracts/GSN/GSNRecipientERC20Fee.sol
index 893872c5ffa..5f7712a6c4f 100644
--- a/contracts/GSN/GSNRecipientERC20Fee.sol
+++ b/contracts/GSN/GSNRecipientERC20Fee.sol
@@ -61,7 +61,7 @@ contract GSNRecipientERC20Fee is GSNRecipient {
         bytes calldata,
         uint256 maxPossibleCharge
     )
-        external
+        public
         view
         virtual
         override
diff --git a/contracts/GSN/GSNRecipientSignature.sol b/contracts/GSN/GSNRecipientSignature.sol
index 055be817521..7938a0ab70c 100644
--- a/contracts/GSN/GSNRecipientSignature.sol
+++ b/contracts/GSN/GSNRecipientSignature.sol
@@ -40,7 +40,7 @@ contract GSNRecipientSignature is GSNRecipient {
         bytes calldata approvalData,
         uint256
     )
-        external
+        public
         view
         virtual
         override
diff --git a/contracts/introspection/ERC165.sol b/contracts/introspection/ERC165.sol
index 881a636e965..45d78099b92 100644
--- a/contracts/introspection/ERC165.sol
+++ b/contracts/introspection/ERC165.sol
@@ -30,7 +30,7 @@ contract ERC165 is IERC165 {
      *
      * Time complexity O(1), guaranteed to always use less than 30 000 gas.
      */
-    function supportsInterface(bytes4 interfaceId) external view override returns (bool) {
+    function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
         return _supportedInterfaces[interfaceId];
     }
 
diff --git a/contracts/introspection/ERC1820Implementer.sol b/contracts/introspection/ERC1820Implementer.sol
index 3d44c7ec3a2..0fbbd931945 100644
--- a/contracts/introspection/ERC1820Implementer.sol
+++ b/contracts/introspection/ERC1820Implementer.sol
@@ -18,7 +18,7 @@ contract ERC1820Implementer is IERC1820Implementer {
     /**
      * See {IERC1820Implementer-canImplementInterfaceForAddress}.
      */
-    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account) external view override returns (bytes32) {
+    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account) public view override returns (bytes32) {
         return _supportedInterfaces[interfaceHash][account] ? _ERC1820_ACCEPT_MAGIC : bytes32(0x00);
     }
 
diff --git a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol b/contracts/mocks/ERC165/ERC165InterfacesSupported.sol
index 1f184db6437..fb3b82c9f6d 100644
--- a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol
+++ b/contracts/mocks/ERC165/ERC165InterfacesSupported.sol
@@ -34,7 +34,7 @@ contract SupportsInterfaceWithLookupMock is IERC165 {
     /**
      * @dev Implement supportsInterface(bytes4) using a lookup table.
      */
-    function supportsInterface(bytes4 interfaceId) external view override returns (bool) {
+    function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
         return _supportedInterfaces[interfaceId];
     }
 

From 2a1a2b7940ac62b19e2b463c83f2cc863f2a3d7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 19:09:25 -0300
Subject: [PATCH 07/11] Add changelog entry

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

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf46bfa593e..962976dd9ac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@
 ### Breaking changes
  * `ERC721`: `burn(owner, tokenId)` was removed, use `burn(owner)` instead. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
  * `ERC721`: `_checkOnERC721Received` was removed. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
+ * `ERC721`: `_transferFrom` and `_safeTransferFrom` were renamed to `_transfer` and `_safeTransfer`. ([#2162](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2162))
+ * `Ownable`: removed `_transferOwnership`. ([#2162](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2162))
  * `PullPayment`, `Escrow`: `withdrawWithGas` was removed. The old `withdraw` function now forwards all gas. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
  * `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
  * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114))

From c4024f550fb863e1208a16d60ee9955c0e881c90 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Mon, 30 Mar 2020 19:12:33 -0300
Subject: [PATCH 08/11] Move calldata arguments to memory

---
 contracts/GSN/GSNRecipient.sol          | 4 ++--
 contracts/GSN/GSNRecipientERC20Fee.sol  | 4 ++--
 contracts/GSN/GSNRecipientSignature.sol | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/contracts/GSN/GSNRecipient.sol b/contracts/GSN/GSNRecipient.sol
index 66775005f71..f0554e7c0f6 100644
--- a/contracts/GSN/GSNRecipient.sol
+++ b/contracts/GSN/GSNRecipient.sol
@@ -119,7 +119,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      *
      * - the caller must be the `RelayHub` contract.
      */
-    function preRelayedCall(bytes calldata context) public virtual override returns (bytes32) {
+    function preRelayedCall(bytes memory context) public virtual override returns (bytes32) {
         require(msg.sender == getHubAddr(), "GSNRecipient: caller is not RelayHub");
         return _preRelayedCall(context);
     }
@@ -142,7 +142,7 @@ abstract contract GSNRecipient is IRelayRecipient, Context {
      *
      * - the caller must be the `RelayHub` contract.
      */
-    function postRelayedCall(bytes calldata context, bool success, uint256 actualCharge, bytes32 preRetVal) public virtual override {
+    function postRelayedCall(bytes memory context, bool success, uint256 actualCharge, bytes32 preRetVal) public virtual override {
         require(msg.sender == getHubAddr(), "GSNRecipient: caller is not RelayHub");
         _postRelayedCall(context, success, actualCharge, preRetVal);
     }
diff --git a/contracts/GSN/GSNRecipientERC20Fee.sol b/contracts/GSN/GSNRecipientERC20Fee.sol
index 5f7712a6c4f..4bfee568281 100644
--- a/contracts/GSN/GSNRecipientERC20Fee.sol
+++ b/contracts/GSN/GSNRecipientERC20Fee.sol
@@ -53,12 +53,12 @@ contract GSNRecipientERC20Fee is GSNRecipient {
     function acceptRelayedCall(
         address,
         address from,
-        bytes calldata,
+        bytes memory,
         uint256 transactionFee,
         uint256 gasPrice,
         uint256,
         uint256,
-        bytes calldata,
+        bytes memory,
         uint256 maxPossibleCharge
     )
         public
diff --git a/contracts/GSN/GSNRecipientSignature.sol b/contracts/GSN/GSNRecipientSignature.sol
index 7938a0ab70c..a777e37f4a3 100644
--- a/contracts/GSN/GSNRecipientSignature.sol
+++ b/contracts/GSN/GSNRecipientSignature.sol
@@ -32,12 +32,12 @@ contract GSNRecipientSignature is GSNRecipient {
     function acceptRelayedCall(
         address relay,
         address from,
-        bytes calldata encodedFunction,
+        bytes memory encodedFunction,
         uint256 transactionFee,
         uint256 gasPrice,
         uint256 gasLimit,
         uint256 nonce,
-        bytes calldata approvalData,
+        bytes memory approvalData,
         uint256
     )
         public

From 16e55815aeb15ceadda4e293e141d273310a5e5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 1 Apr 2020 11:42:40 -0300
Subject: [PATCH 09/11] Update contracts/access/AccessControl.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>
---
 contracts/access/AccessControl.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index d1163a5c989..9bc1939a05c 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -162,7 +162,7 @@ abstract contract AccessControl is Context {
      * event.
      *
      * NOTE: Unlike {grantRole}, this function doesn't perform any checks on the
-     * calling account. Its usage should be restricted to grating the initial
+     * calling account. Its usage should be restricted to granting the initial
      * set of rules during contract construction.
      */
     function _setupRole(bytes32 role, address account) internal virtual {

From 972a75ee860d5cb79eacaf0e3a0740175ee64fc2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 1 Apr 2020 18:52:40 -0300
Subject: [PATCH 10/11] Restrict setupRole to the constructor

---
 contracts/access/AccessControl.sol    | 24 ++++++++++++++++++++----
 contracts/mocks/AccessControlMock.sol |  4 ++++
 test/access/AccessControl.test.js     |  9 +++++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 9bc1939a05c..50b90e16814 100644
--- a/contracts/access/AccessControl.sol
+++ b/contracts/access/AccessControl.sol
@@ -159,13 +159,15 @@ abstract contract AccessControl is Context {
      * @dev Grants `role` to `account`.
      *
      * If `account` had not been already granted `role`, emits a {RoleGranted}
-     * event.
+     * event. Note that unlike {grantRole}, this function doesn't perform any
+     * checks on the calling account.
+     *
+     * Requirements:
      *
-     * NOTE: Unlike {grantRole}, this function doesn't perform any checks on the
-     * calling account. Its usage should be restricted to granting the initial
-     * set of rules during contract construction.
+     * - this function can only be called from a constructor.
      */
     function _setupRole(bytes32 role, address account) internal virtual {
+        require(_isConstructor(), "AccessControl: roles cannot be setup after construction");
         _grantRole(role, account);
     }
 
@@ -187,4 +189,18 @@ abstract contract AccessControl is Context {
             emit RoleRevoked(role, account, _msgSender());
         }
     }
+
+    // @dev Returns true if and only if the function is running in the constructor
+    function _isConstructor() private view returns (bool) {
+        // extcodesize checks the size of the code stored in an address, and
+        // address returns the current address. Since the code is still not
+        // deployed when running a constructor, any checks on its code size will
+        // yield zero, making it an effective way to detect if a contract is
+        // under construction or not.
+        address self = address(this);
+        uint256 cs;
+        // solhint-disable-next-line no-inline-assembly
+        assembly { cs := extcodesize(self) }
+        return cs == 0;
+    }
 }
diff --git a/contracts/mocks/AccessControlMock.sol b/contracts/mocks/AccessControlMock.sol
index 4f96be5d5ff..876e78a2af8 100644
--- a/contracts/mocks/AccessControlMock.sol
+++ b/contracts/mocks/AccessControlMock.sol
@@ -10,4 +10,8 @@ contract AccessControlMock is AccessControl {
     function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
         _setRoleAdmin(roleId, adminRoleId);
     }
+
+    function setupRole(bytes32 roleId, address account) public {
+        _setupRole(roleId, account);
+    }
 }
diff --git a/test/access/AccessControl.test.js b/test/access/AccessControl.test.js
index c49276f925d..db4cf47c026 100644
--- a/test/access/AccessControl.test.js
+++ b/test/access/AccessControl.test.js
@@ -17,6 +17,15 @@ describe('AccessControl', function () {
     this.accessControl = await AccessControlMock.new({ from: admin });
   });
 
+  describe('_setupRole', function () {
+    it('cannot be called outside the constructor', async function () {
+      await expectRevert(
+        this.accessControl.setupRole(OTHER_ROLE, other),
+        'AccessControl: roles cannot be setup after construction'
+      );
+    });
+  });
+
   describe('default admin', function () {
     it('deployer has default admin role', async function () {
       expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.equal(true);

From f8104fd21baa43311f5785179308601626ce9996 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 2 Apr 2020 15:17:55 -0300
Subject: [PATCH 11/11] Replace isConstructor for !isContract

---
 contracts/access/AccessControl.sol | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol
index 50b90e16814..e03f72b2a4a 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 "../utils/Address.sol";
 import "../GSN/Context.sol";
 
 /**
@@ -36,6 +37,7 @@ import "../GSN/Context.sol";
  */
 abstract contract AccessControl is Context {
     using EnumerableSet for EnumerableSet.AddressSet;
+    using Address for address;
 
     struct RoleData {
         EnumerableSet.AddressSet members;
@@ -167,7 +169,7 @@ abstract contract AccessControl is Context {
      * - this function can only be called from a constructor.
      */
     function _setupRole(bytes32 role, address account) internal virtual {
-        require(_isConstructor(), "AccessControl: roles cannot be setup after construction");
+        require(!address(this).isContract(), "AccessControl: roles cannot be setup after construction");
         _grantRole(role, account);
     }
 
@@ -189,18 +191,4 @@ abstract contract AccessControl is Context {
             emit RoleRevoked(role, account, _msgSender());
         }
     }
-
-    // @dev Returns true if and only if the function is running in the constructor
-    function _isConstructor() private view returns (bool) {
-        // extcodesize checks the size of the code stored in an address, and
-        // address returns the current address. Since the code is still not
-        // deployed when running a constructor, any checks on its code size will
-        // yield zero, making it an effective way to detect if a contract is
-        // under construction or not.
-        address self = address(this);
-        uint256 cs;
-        // solhint-disable-next-line no-inline-assembly
-        assembly { cs := extcodesize(self) }
-        return cs == 0;
-    }
 }