Cleanup of Ownership directory (#2120)

* Remove Ownable.isOwner.

* Remove ownable behavior from tests

* Make Escrow use Ownable instead of Secondary

* Migrate GSNRecipientERC20Fee to Ownable

* Remove Secondary

* Move Ownable to access directory

* Remove mentions of Secondary

* Add changelog entry

* Move Ownable tests to access

* Remove unused mock

Author: nventuro

CHANGELOG.md

@@ -2,8 +2,12 @@
 
 ## 3.0.0 (unreleased)
 
-### Breaking Changes
+### Breaking changes
  * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114))
+ * `Ownable`: moved to the `access` directory. ([#2120](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2120))
+ * `Ownable`: removed `isOwner`. ([#2120](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2120))
+ * `Secondary`: removed from the library, use `Ownable` instead. ([#2120](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2120))
+ * `Escrow`, `ConditionalEscrow`, `RefundEscrow`: these now use `Ownable` instead of `Secondary`, their external API changed accordingly. ([#2120](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2120))
  * `ERC20`: removed `_burnFrom`. ([#2119](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2119))
 
 ## 2.5.0 (2020-02-04)

contracts/GSN/GSNRecipientERC20Fee.sol

@@ -2,7 +2,7 @@ pragma solidity ^0.6.0;
 
 import "./GSNRecipient.sol";
 import "../math/SafeMath.sol";
-import "../ownership/Secondary.sol";
+import "../access/Ownable.sol";
 import "../token/ERC20/SafeERC20.sol";
 import "../token/ERC20/ERC20.sol";
 import "../token/ERC20/ERC20Detailed.sol";
@@ -17,20 +17,20 @@ import "../token/ERC20/ERC20Detailed.sol";
  * internal {_mint} function.
  */
 contract GSNRecipientERC20Fee is GSNRecipient {
-    using SafeERC20 for __unstable__ERC20PrimaryAdmin;
+    using SafeERC20 for __unstable__ERC20Owned;
     using SafeMath for uint256;
 
     enum GSNRecipientERC20FeeErrorCodes {
         INSUFFICIENT_BALANCE
     }
 
-    __unstable__ERC20PrimaryAdmin private _token;
+    __unstable__ERC20Owned private _token;
 
     /**
      * @dev The arguments to the constructor are the details that the gas payment token will have: `name` and `symbol`. `decimals` is hard-coded to 18.
      */
     constructor(string memory name, string memory symbol) public {
-        _token = new __unstable__ERC20PrimaryAdmin(name, symbol, 18);
+        _token = new __unstable__ERC20Owned(name, symbol, 18);
     }
 
     /**
@@ -106,42 +106,42 @@ contract GSNRecipientERC20Fee is GSNRecipient {
 }
 
 /**
- * @title __unstable__ERC20PrimaryAdmin
+ * @title __unstable__ERC20Owned
  * @dev An ERC20 token owned by another contract, which has minting permissions and can use transferFrom to receive
  * anyone's tokens. This contract is an internal helper for GSNRecipientERC20Fee, and should not be used
  * outside of this context.
  */
 // solhint-disable-next-line contract-name-camelcase
-contract __unstable__ERC20PrimaryAdmin is ERC20, ERC20Detailed, Secondary {
+contract __unstable__ERC20Owned is ERC20, ERC20Detailed, Ownable {
     uint256 private constant UINT256_MAX = 2**256 - 1;
 
     constructor(string memory name, string memory symbol, uint8 decimals) public ERC20Detailed(name, symbol, decimals) { }
 
-    // The primary account (GSNRecipientERC20Fee) can mint tokens
-    function mint(address account, uint256 amount) public onlyPrimary {
+    // The owner (GSNRecipientERC20Fee) can mint tokens
+    function mint(address account, uint256 amount) public onlyOwner {
         _mint(account, amount);
     }
 
-    // The primary account has 'infinite' allowance for all token holders
-    function allowance(address owner, address spender) public view override(ERC20, IERC20) returns (uint256) {
-        if (spender == primary()) {
+    // The owner has 'infinite' allowance for all token holders
+    function allowance(address tokenOwner, address spender) public view override(ERC20, IERC20) returns (uint256) {
+        if (spender == owner()) {
             return UINT256_MAX;
         } else {
-            return super.allowance(owner, spender);
+            return super.allowance(tokenOwner, spender);
         }
     }
 
-    // Allowance for the primary account cannot be changed (it is always 'infinite')
-    function _approve(address owner, address spender, uint256 value) internal override {
-        if (spender == primary()) {
+    // Allowance for the owner cannot be changed (it is always 'infinite')
+    function _approve(address tokenOwner, address spender, uint256 value) internal override {
+        if (spender == owner()) {
             return;
         } else {
-            super._approve(owner, spender, value);
+            super._approve(tokenOwner, spender, value);
         }
     }
 
     function transferFrom(address sender, address recipient, uint256 amount) public override(ERC20, IERC20) returns (bool) {
-        if (recipient == primary()) {
+        if (recipient == owner()) {
             _transfer(sender, recipient, amount);
             return true;
         } else {

contracts/ownership/Ownable.sol contracts/access/Ownable.sol

@@ -6,6 +6,9 @@ import "../GSN/Context.sol";
  * there is an account (an owner) that can be granted exclusive access to
  * specific functions.
  *
+ * By default, the owner account will be the one that deploys the contract. This
+ * can later be changed with {transferOwnership}.
+ *
  * This module is used through inheritance. It will make available the modifier
  * `onlyOwner`, which can be applied to your functions to restrict their use to
  * the owner.
@@ -35,17 +38,10 @@ contract Ownable is Context {
      * @dev Throws if called by any account other than the owner.
      */
     modifier onlyOwner() {
-        require(isOwner(), "Ownable: caller is not the owner");
+        require(_owner == _msgSender(), "Ownable: caller is not the owner");
         _;
     }
 
-    /**
-     * @dev Returns true if the caller is the current owner.
-     */
-    function isOwner() public view returns (bool) {
-        return _msgSender() == _owner;
-    }
-
     /**
      * @dev Leaves the contract without owner. It will not be possible to call
      * `onlyOwner` functions anymore. Can only be called by the current owner.

contracts/access/README.adoc

@@ -1,7 +1,9 @@
 = Access
 
-NOTE: This page is incomplete. We're working to improve it for the next release. Stay tuned!
+Contract modules for authorization and access control mechanisms.
 
-== Library
+== Contracts
+
+{{Ownable}}
 
 {{Roles}}

contracts/drafts/TokenVesting.sol

@@ -1,7 +1,7 @@
 pragma solidity ^0.6.0;
 
 import "../token/ERC20/SafeERC20.sol";
-import "../ownership/Ownable.sol";
+import "../access/Ownable.sol";
 import "../math/SafeMath.sol";
 
 /**

contracts/mocks/OwnableInterfaceId.sol

@@ -1,5 +1,5 @@
 pragma solidity ^0.6.0;
 
-import "../ownership/Ownable.sol";
+import "../access/Ownable.sol";
 
 contract OwnableMock is Ownable { }

contracts/mocks/OwnableMock.sol

@@ -1,7 +1,7 @@
 pragma solidity ^0.6.0;
 
 import "../../math/SafeMath.sol";
-import "../../ownership/Secondary.sol";
+import "../../access/Ownable.sol";
 import "../../utils/Address.sol";
 
  /**
@@ -14,10 +14,10 @@ import "../../utils/Address.sol";
   * it. That way, it is guaranteed that all Ether will be handled according to
   * the `Escrow` rules, and there is no need to check for payable functions or
   * transfers in the inheritance tree. The contract that uses the escrow as its
-  * payment method should be its primary, and provide public methods redirecting
+  * payment method should be its owner, and provide public methods redirecting
   * to the escrow's deposit and withdraw.
   */
-contract Escrow is Secondary {
+contract Escrow is Ownable {
     using SafeMath for uint256;
     using Address for address payable;
 
@@ -34,7 +34,7 @@ contract Escrow is Secondary {
      * @dev Stores the sent amount as credit to be withdrawn.
      * @param payee The destination address of the funds.
      */
-    function deposit(address payee) public virtual payable onlyPrimary {
+    function deposit(address payee) public virtual payable onlyOwner {
         uint256 amount = msg.value;
         _deposits[payee] = _deposits[payee].add(amount);
 
@@ -52,7 +52,7 @@ contract Escrow is Secondary {
      *
      * @param payee The address whose funds will be withdrawn and transferred to.
      */
-    function withdraw(address payable payee) public virtual onlyPrimary {
+    function withdraw(address payable payee) public virtual onlyOwner {
         uint256 payment = _deposits[payee];
 
         _deposits[payee] = 0;
@@ -71,7 +71,7 @@ contract Escrow is Secondary {
      *
      * _Available since v2.4.0._
      */
-    function withdrawWithGas(address payable payee) public virtual onlyPrimary {
+    function withdrawWithGas(address payable payee) public virtual onlyOwner {
         uint256 payment = _deposits[payee];
 
         _deposits[payee] = 0;

contracts/mocks/SecondaryMock.sol

@@ -7,10 +7,10 @@ import "./ConditionalEscrow.sol";
  * @dev Escrow that holds funds for a beneficiary, deposited from multiple
  * parties.
  * @dev Intended usage: See {Escrow}. Same usage guidelines apply here.
- * @dev The primary account (that is, the contract that instantiates this
+ * @dev The owner account (that is, the contract that instantiates this
  * contract) may deposit, close the deposit period, and allow for either
  * withdrawal by the beneficiary, or refunds to the depositors. All interactions
- * with `RefundEscrow` will be made through the primary contract.
+ * with `RefundEscrow` will be made through the owner contract.
  */
 contract RefundEscrow is ConditionalEscrow {
     enum State { Active, Refunding, Closed }
@@ -58,7 +58,7 @@ contract RefundEscrow is ConditionalEscrow {
      * @dev Allows for the beneficiary to withdraw their funds, rejecting
      * further deposits.
      */
-    function close() public onlyPrimary virtual {
+    function close() public onlyOwner virtual {
         require(_state == State.Active, "RefundEscrow: can only close while active");
         _state = State.Closed;
         emit RefundsClosed();
@@ -67,7 +67,7 @@ contract RefundEscrow is ConditionalEscrow {
     /**
      * @dev Allows for refunds to take place, rejecting further deposits.
      */
-    function enableRefunds() public onlyPrimary virtual {
+    function enableRefunds() public onlyOwner virtual {
         require(_state == State.Active, "RefundEscrow: can only enable refunds while active");
         _state = State.Refunding;
         emit RefundsEnabled();

contracts/ownership/README.adoc

@@ -7,13 +7,13 @@ Access control—that is, "who is allowed to do this thing"—is incredibly impo
 
 The most common and basic form of access control is the concept of _ownership_: there's an account that is the `owner` of a contract and can do administrative tasks on it. This approach is perfectly reasonable for contracts that have a single administrative user.
 
-OpenZeppelin provides xref:api:ownership.adoc#Ownable[`Ownable`] for implementing ownership in your contracts.
+OpenZeppelin provides xref:api:access.adoc#Ownable[`Ownable`] for implementing ownership in your contracts.
 
 [source,solidity]
 ----
 pragma solidity ^0.5.0;
 
-import "@openzeppelin/contracts/ownership/Ownable.sol";
+import "@openzeppelin/contracts/access/Ownable.sol";
 
 contract MyContract is Ownable {
     function normalThing() public {
@@ -26,12 +26,12 @@ contract MyContract is Ownable {
 }
 ----
 
-By default, the xref:api:ownership.adoc#Ownable-owner--[`owner`] of an `Ownable` contract is the account that deployed it, which is usually exactly what you want.
+By default, the xref:api:access.adoc#Ownable-owner--[`owner`] of an `Ownable` contract is the account that deployed it, which is usually exactly what you want.
 
 Ownable also lets you:
 
-* xref:api:ownership.adoc#Ownable-transferOwnership-address-[`transferOwnership`] from the owner account to a new one, and
-* xref:api:ownership.adoc#Ownable-renounceOwnership--[`renounceOwnership`] for the owner to relinquish this administrative privilege, a common pattern after an initial stage with centralized administration is over.
+* xref:api:access.adoc#Ownable-transferOwnership-address-[`transferOwnership`] from the owner account to a new one, and
+* xref:api:access.adoc#Ownable-renounceOwnership--[`renounceOwnership`] for the owner to relinquish this administrative privilege, a common pattern after an initial stage with centralized administration is over.
 
 WARNING: Removing the owner altogether will mean that administrative tasks that are protected by `onlyOwner` will no longer be callable!
 
@@ -103,10 +103,3 @@ So clean! By splitting concerns this way, much more granular levels of permissio
 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.
 
 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.
-
-[[usage-in-openzeppelin]]
-== Usage in OpenZeppelin
-
-You'll notice that none of the OpenZeppelin contracts use `Ownable`. `Roles` is a prefferred solution, because it provides the user of the library with enough flexibility to adapt the provided contracts to their needs.
-
-There are some cases, however, where there's a direct relationship between contracts. For example, xref:api:crowdsale.adoc#RefundableCrowdsale[`RefundableCrowdsale`] deploys a xref:api:payment.adoc#RefundEscrow[`RefundEscrow`] on construction, to hold its funds. For those cases, we'll use xref:api:ownership.adoc#Secondary[`Secondary`] to create a _secondary_ contract that allows a _primary_ contract to manage it. You could also think of these as _auxiliary_ contracts.