From 8176a901a9edfac52391315296fb8b7784454ecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 16 Mar 2020 15:12:29 -0300 Subject: [PATCH] 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 --- CHANGELOG.md | 6 +- contracts/GSN/GSNRecipientERC20Fee.sol | 34 +++++------ contracts/{ownership => access}/Ownable.sol | 12 ++-- contracts/access/README.adoc | 6 +- contracts/drafts/TokenVesting.sol | 2 +- contracts/mocks/OwnableInterfaceId.sol | 15 ----- contracts/mocks/OwnableMock.sol | 2 +- contracts/mocks/SecondaryMock.sol | 7 --- contracts/ownership/README.adoc | 11 ---- contracts/ownership/Secondary.sol | 50 --------------- contracts/payment/escrow/Escrow.sol | 12 ++-- contracts/payment/escrow/RefundEscrow.sol | 8 +-- docs/modules/ROOT/pages/access-control.adoc | 17 ++---- test/access/Ownable.test.js | 54 ++++++++++++++++ test/ownership/Ownable.behavior.js | 53 ---------------- test/ownership/Ownable.test.js | 16 ----- test/ownership/Secondary.test.js | 68 --------------------- test/payment/escrow/Escrow.behavior.js | 34 +++++------ test/payment/escrow/Escrow.test.js | 6 +- test/payment/escrow/RefundEscrow.test.js | 46 +++++++------- 20 files changed, 144 insertions(+), 315 deletions(-) rename contracts/{ownership => access}/Ownable.sol (90%) delete mode 100644 contracts/mocks/OwnableInterfaceId.sol delete mode 100644 contracts/mocks/SecondaryMock.sol delete mode 100644 contracts/ownership/README.adoc delete mode 100644 contracts/ownership/Secondary.sol create mode 100644 test/access/Ownable.test.js delete mode 100644 test/ownership/Ownable.behavior.js delete mode 100644 test/ownership/Ownable.test.js delete mode 100644 test/ownership/Secondary.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index f0e30d848..ce747e9ae 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/contracts/GSN/GSNRecipientERC20Fee.sol b/contracts/GSN/GSNRecipientERC20Fee.sol index b0c624154..da83b0554 100644 --- a/contracts/GSN/GSNRecipientERC20Fee.sol +++ b/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 { diff --git a/contracts/ownership/Ownable.sol b/contracts/access/Ownable.sol similarity index 90% rename from contracts/ownership/Ownable.sol rename to contracts/access/Ownable.sol index c1168054e..bea305ba5 100644 --- a/contracts/ownership/Ownable.sol +++ b/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. diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index ec60be8da..dbc2e50ca 100644 --- a/contracts/access/README.adoc +++ b/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}} diff --git a/contracts/drafts/TokenVesting.sol b/contracts/drafts/TokenVesting.sol index 8981ef9f6..9529a7d34 100644 --- a/contracts/drafts/TokenVesting.sol +++ b/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"; /** diff --git a/contracts/mocks/OwnableInterfaceId.sol b/contracts/mocks/OwnableInterfaceId.sol deleted file mode 100644 index 19dc680bb..000000000 --- a/contracts/mocks/OwnableInterfaceId.sol +++ /dev/null @@ -1,15 +0,0 @@ -pragma solidity ^0.6.0; - -import "../ownership/Ownable.sol"; - -/** - * @title Ownable interface id calculator. - * @dev See the EIP165 specification for more information: - * https://eips.ethereum.org/EIPS/eip-165#specification - */ -contract OwnableInterfaceId { - function getInterfaceId() public pure returns (bytes4) { - Ownable i; - return i.owner.selector ^ i.isOwner.selector ^ i.renounceOwnership.selector ^ i.transferOwnership.selector; - } -} diff --git a/contracts/mocks/OwnableMock.sol b/contracts/mocks/OwnableMock.sol index 85e829198..23f00fe59 100644 --- a/contracts/mocks/OwnableMock.sol +++ b/contracts/mocks/OwnableMock.sol @@ -1,5 +1,5 @@ pragma solidity ^0.6.0; -import "../ownership/Ownable.sol"; +import "../access/Ownable.sol"; contract OwnableMock is Ownable { } diff --git a/contracts/mocks/SecondaryMock.sol b/contracts/mocks/SecondaryMock.sol deleted file mode 100644 index b5ebd22f7..000000000 --- a/contracts/mocks/SecondaryMock.sol +++ /dev/null @@ -1,7 +0,0 @@ -pragma solidity ^0.6.0; - -import "../ownership/Secondary.sol"; - -contract SecondaryMock is Secondary { - function onlyPrimaryMock() public view onlyPrimary { } -} diff --git a/contracts/ownership/README.adoc b/contracts/ownership/README.adoc deleted file mode 100644 index f0b7d0037..000000000 --- a/contracts/ownership/README.adoc +++ /dev/null @@ -1,11 +0,0 @@ -= Ownership - -Contract modules for simple authorization and access control mechanisms. - -TIP: For more complex needs see xref:access.adoc[Access]. - -== Contracts - -{{Ownable}} - -{{Secondary}} diff --git a/contracts/ownership/Secondary.sol b/contracts/ownership/Secondary.sol deleted file mode 100644 index 0eb796f4b..000000000 --- a/contracts/ownership/Secondary.sol +++ /dev/null @@ -1,50 +0,0 @@ -pragma solidity ^0.6.0; - -import "../GSN/Context.sol"; -/** - * @dev A Secondary contract can only be used by its primary account (the one that created it). - */ -contract Secondary is Context { - address private _primary; - - /** - * @dev Emitted when the primary contract changes. - */ - event PrimaryTransferred( - address recipient - ); - - /** - * @dev Sets the primary account to the one that is creating the Secondary contract. - */ - constructor () internal { - address msgSender = _msgSender(); - _primary = msgSender; - emit PrimaryTransferred(msgSender); - } - - /** - * @dev Reverts if called from any account other than the primary. - */ - modifier onlyPrimary() { - require(_msgSender() == _primary, "Secondary: caller is not the primary account"); - _; - } - - /** - * @return the address of the primary. - */ - function primary() public view returns (address) { - return _primary; - } - - /** - * @dev Transfers contract to a new primary. - * @param recipient The address of new primary. - */ - function transferPrimary(address recipient) public virtual onlyPrimary { - require(recipient != address(0), "Secondary: new primary is the zero address"); - _primary = recipient; - emit PrimaryTransferred(recipient); - } -} diff --git a/contracts/payment/escrow/Escrow.sol b/contracts/payment/escrow/Escrow.sol index 3744c2b39..dd1b6cf0d 100644 --- a/contracts/payment/escrow/Escrow.sol +++ b/contracts/payment/escrow/Escrow.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; diff --git a/contracts/payment/escrow/RefundEscrow.sol b/contracts/payment/escrow/RefundEscrow.sol index 96d1c2d8a..b2f717f54 100644 --- a/contracts/payment/escrow/RefundEscrow.sol +++ b/contracts/payment/escrow/RefundEscrow.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(); diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index 553dab515..fc2b64f04 100644 --- a/docs/modules/ROOT/pages/access-control.adoc +++ b/docs/modules/ROOT/pages/access-control.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. diff --git a/test/access/Ownable.test.js b/test/access/Ownable.test.js new file mode 100644 index 000000000..dce0756e7 --- /dev/null +++ b/test/access/Ownable.test.js @@ -0,0 +1,54 @@ +const { accounts, contract } = require('@openzeppelin/test-environment'); +const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = constants; + +const { expect } = require('chai'); + +const Ownable = contract.fromArtifact('OwnableMock'); + +describe('Ownable', function () { + const [ owner, other ] = accounts; + + beforeEach(async function () { + this.ownable = await Ownable.new({ from: owner }); + }); + + it('should have an owner', async function () { + expect(await this.ownable.owner()).to.equal(owner); + }); + + it('changes owner after transfer', async function () { + const receipt = await this.ownable.transferOwnership(other, { from: owner }); + expectEvent(receipt, 'OwnershipTransferred'); + + expect(await this.ownable.owner()).to.equal(other); + }); + + it('should prevent non-owners from transferring', async function () { + await expectRevert( + this.ownable.transferOwnership(other, { from: other }), + 'Ownable: caller is not the owner' + ); + }); + + it('should guard ownership against stuck state', async function () { + await expectRevert( + this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }), + 'Ownable: new owner is the zero address' + ); + }); + + it('loses owner after renouncement', async function () { + const receipt = await this.ownable.renounceOwnership({ from: owner }); + expectEvent(receipt, 'OwnershipTransferred'); + + expect(await this.ownable.owner()).to.equal(ZERO_ADDRESS); + }); + + it('should prevent non-owners from renouncement', async function () { + await expectRevert( + this.ownable.renounceOwnership({ from: other }), + 'Ownable: caller is not the owner' + ); + }); +}); diff --git a/test/ownership/Ownable.behavior.js b/test/ownership/Ownable.behavior.js deleted file mode 100644 index 04fb9c8ec..000000000 --- a/test/ownership/Ownable.behavior.js +++ /dev/null @@ -1,53 +0,0 @@ -const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); -const { ZERO_ADDRESS } = constants; - -const { expect } = require('chai'); - -function shouldBehaveLikeOwnable (owner, [other]) { - describe('as an ownable', function () { - it('should have an owner', async function () { - expect(await this.ownable.owner()).to.equal(owner); - }); - - it('changes owner after transfer', async function () { - expect(await this.ownable.isOwner({ from: other })).to.equal(false); - const receipt = await this.ownable.transferOwnership(other, { from: owner }); - expectEvent(receipt, 'OwnershipTransferred'); - - expect(await this.ownable.owner()).to.equal(other); - expect(await this.ownable.isOwner({ from: other })).to.equal(true); - }); - - it('should prevent non-owners from transferring', async function () { - await expectRevert( - this.ownable.transferOwnership(other, { from: other }), - 'Ownable: caller is not the owner' - ); - }); - - it('should guard ownership against stuck state', async function () { - await expectRevert( - this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }), - 'Ownable: new owner is the zero address' - ); - }); - - it('loses owner after renouncement', async function () { - const receipt = await this.ownable.renounceOwnership({ from: owner }); - expectEvent(receipt, 'OwnershipTransferred'); - - expect(await this.ownable.owner()).to.equal(ZERO_ADDRESS); - }); - - it('should prevent non-owners from renouncement', async function () { - await expectRevert( - this.ownable.renounceOwnership({ from: other }), - 'Ownable: caller is not the owner' - ); - }); - }); -} - -module.exports = { - shouldBehaveLikeOwnable, -}; diff --git a/test/ownership/Ownable.test.js b/test/ownership/Ownable.test.js deleted file mode 100644 index b23f78c82..000000000 --- a/test/ownership/Ownable.test.js +++ /dev/null @@ -1,16 +0,0 @@ -const { accounts, contract } = require('@openzeppelin/test-environment'); - -require('@openzeppelin/test-helpers'); -const { shouldBehaveLikeOwnable } = require('./Ownable.behavior'); - -const Ownable = contract.fromArtifact('OwnableMock'); - -describe('Ownable', function () { - const [ owner, ...otherAccounts ] = accounts; - - beforeEach(async function () { - this.ownable = await Ownable.new({ from: owner }); - }); - - shouldBehaveLikeOwnable(owner, otherAccounts); -}); diff --git a/test/ownership/Secondary.test.js b/test/ownership/Secondary.test.js deleted file mode 100644 index d615d5a5f..000000000 --- a/test/ownership/Secondary.test.js +++ /dev/null @@ -1,68 +0,0 @@ -const { accounts, contract } = require('@openzeppelin/test-environment'); - -const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); -const { ZERO_ADDRESS } = constants; - -const { expect } = require('chai'); - -const SecondaryMock = contract.fromArtifact('SecondaryMock'); - -describe('Secondary', function () { - const [ primary, newPrimary, other ] = accounts; - - beforeEach(async function () { - this.secondary = await SecondaryMock.new({ from: primary }); - }); - - it('stores the primary\'s address', async function () { - expect(await this.secondary.primary()).to.equal(primary); - }); - - describe('onlyPrimary', function () { - it('allows the primary account to call onlyPrimary functions', async function () { - await this.secondary.onlyPrimaryMock({ from: primary }); - }); - - it('reverts when anyone calls onlyPrimary functions', async function () { - await expectRevert(this.secondary.onlyPrimaryMock({ from: other }), - 'Secondary: caller is not the primary account' - ); - }); - }); - - describe('transferPrimary', function () { - it('makes the recipient the new primary', async function () { - const { logs } = await this.secondary.transferPrimary(newPrimary, { from: primary }); - expectEvent.inLogs(logs, 'PrimaryTransferred', { recipient: newPrimary }); - expect(await this.secondary.primary()).to.equal(newPrimary); - }); - - it('reverts when transferring to the null address', async function () { - await expectRevert(this.secondary.transferPrimary(ZERO_ADDRESS, { from: primary }), - 'Secondary: new primary is the zero address' - ); - }); - - it('reverts when called by anyone', async function () { - await expectRevert(this.secondary.transferPrimary(newPrimary, { from: other }), - 'Secondary: caller is not the primary account' - ); - }); - - context('with new primary', function () { - beforeEach(async function () { - await this.secondary.transferPrimary(newPrimary, { from: primary }); - }); - - it('allows the new primary account to call onlyPrimary functions', async function () { - await this.secondary.onlyPrimaryMock({ from: newPrimary }); - }); - - it('reverts when the old primary account calls onlyPrimary functions', async function () { - await expectRevert(this.secondary.onlyPrimaryMock({ from: primary }), - 'Secondary: caller is not the primary account' - ); - }); - }); - }); -}); diff --git a/test/payment/escrow/Escrow.behavior.js b/test/payment/escrow/Escrow.behavior.js index c8d08f1d3..d096b8f8a 100644 --- a/test/payment/escrow/Escrow.behavior.js +++ b/test/payment/escrow/Escrow.behavior.js @@ -2,13 +2,13 @@ const { balance, ether, expectEvent, expectRevert } = require('@openzeppelin/tes const { expect } = require('chai'); -function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { +function shouldBehaveLikeEscrow (owner, [payee1, payee2]) { const amount = ether('42'); describe('as an escrow', function () { describe('deposits', function () { it('can accept a single deposit', async function () { - await this.escrow.deposit(payee1, { from: primary, value: amount }); + await this.escrow.deposit(payee1, { from: owner, value: amount }); expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount); @@ -16,17 +16,17 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { }); it('can accept an empty deposit', async function () { - await this.escrow.deposit(payee1, { from: primary, value: 0 }); + await this.escrow.deposit(payee1, { from: owner, value: 0 }); }); - it('only the primary account can deposit', async function () { + it('only the owner can deposit', async function () { await expectRevert(this.escrow.deposit(payee1, { from: payee2 }), - 'Secondary: caller is not the primary account' + 'Ownable: caller is not the owner' ); }); it('emits a deposited event', async function () { - const { logs } = await this.escrow.deposit(payee1, { from: primary, value: amount }); + const { logs } = await this.escrow.deposit(payee1, { from: owner, value: amount }); expectEvent.inLogs(logs, 'Deposited', { payee: payee1, weiAmount: amount, @@ -34,8 +34,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { }); it('can add multiple deposits on a single account', async function () { - await this.escrow.deposit(payee1, { from: primary, value: amount }); - await this.escrow.deposit(payee1, { from: primary, value: amount.muln(2) }); + await this.escrow.deposit(payee1, { from: owner, value: amount }); + await this.escrow.deposit(payee1, { from: owner, value: amount.muln(2) }); expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount.muln(3)); @@ -43,8 +43,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { }); it('can track deposits to multiple accounts', async function () { - await this.escrow.deposit(payee1, { from: primary, value: amount }); - await this.escrow.deposit(payee2, { from: primary, value: amount.muln(2) }); + await this.escrow.deposit(payee1, { from: owner, value: amount }); + await this.escrow.deposit(payee2, { from: owner, value: amount.muln(2) }); expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount.muln(3)); @@ -58,8 +58,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { it('can withdraw payments', async function () { const balanceTracker = await balance.tracker(payee1); - await this.escrow.deposit(payee1, { from: primary, value: amount }); - await this.escrow.withdraw(payee1, { from: primary }); + await this.escrow.deposit(payee1, { from: owner, value: amount }); + await this.escrow.withdraw(payee1, { from: owner }); expect(await balanceTracker.delta()).to.be.bignumber.equal(amount); @@ -68,18 +68,18 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) { }); it('can do an empty withdrawal', async function () { - await this.escrow.withdraw(payee1, { from: primary }); + await this.escrow.withdraw(payee1, { from: owner }); }); - it('only the primary account can withdraw', async function () { + it('only the owner can withdraw', async function () { await expectRevert(this.escrow.withdraw(payee1, { from: payee1 }), - 'Secondary: caller is not the primary account' + 'Ownable: caller is not the owner' ); }); it('emits a withdrawn event', async function () { - await this.escrow.deposit(payee1, { from: primary, value: amount }); - const { logs } = await this.escrow.withdraw(payee1, { from: primary }); + await this.escrow.deposit(payee1, { from: owner, value: amount }); + const { logs } = await this.escrow.withdraw(payee1, { from: owner }); expectEvent.inLogs(logs, 'Withdrawn', { payee: payee1, weiAmount: amount, diff --git a/test/payment/escrow/Escrow.test.js b/test/payment/escrow/Escrow.test.js index d7fea1a1c..0da833c6f 100644 --- a/test/payment/escrow/Escrow.test.js +++ b/test/payment/escrow/Escrow.test.js @@ -6,11 +6,11 @@ const { shouldBehaveLikeEscrow } = require('./Escrow.behavior'); const Escrow = contract.fromArtifact('Escrow'); describe('Escrow', function () { - const [ primary, ...otherAccounts ] = accounts; + const [ owner, ...otherAccounts ] = accounts; beforeEach(async function () { - this.escrow = await Escrow.new({ from: primary }); + this.escrow = await Escrow.new({ from: owner }); }); - shouldBehaveLikeEscrow(primary, otherAccounts); + shouldBehaveLikeEscrow(owner, otherAccounts); }); diff --git a/test/payment/escrow/RefundEscrow.test.js b/test/payment/escrow/RefundEscrow.test.js index 048cea12a..443228264 100644 --- a/test/payment/escrow/RefundEscrow.test.js +++ b/test/payment/escrow/RefundEscrow.test.js @@ -8,20 +8,20 @@ const { expect } = require('chai'); const RefundEscrow = contract.fromArtifact('RefundEscrow'); describe('RefundEscrow', function () { - const [ primary, beneficiary, refundee1, refundee2 ] = accounts; + const [ owner, beneficiary, refundee1, refundee2 ] = accounts; const amount = ether('54'); const refundees = [refundee1, refundee2]; it('requires a non-null beneficiary', async function () { await expectRevert( - RefundEscrow.new(ZERO_ADDRESS, { from: primary }), 'RefundEscrow: beneficiary is the zero address' + RefundEscrow.new(ZERO_ADDRESS, { from: owner }), 'RefundEscrow: beneficiary is the zero address' ); }); context('once deployed', function () { beforeEach(async function () { - this.escrow = await RefundEscrow.new(beneficiary, { from: primary }); + this.escrow = await RefundEscrow.new(beneficiary, { from: owner }); }); context('active state', function () { @@ -31,44 +31,44 @@ describe('RefundEscrow', function () { }); it('accepts deposits', async function () { - await this.escrow.deposit(refundee1, { from: primary, value: amount }); + await this.escrow.deposit(refundee1, { from: owner, value: amount }); expect(await this.escrow.depositsOf(refundee1)).to.be.bignumber.equal(amount); }); it('does not refund refundees', async function () { - await this.escrow.deposit(refundee1, { from: primary, value: amount }); + await this.escrow.deposit(refundee1, { from: owner, value: amount }); await expectRevert(this.escrow.withdraw(refundee1), 'ConditionalEscrow: payee is not allowed to withdraw' ); }); it('does not allow beneficiary withdrawal', async function () { - await this.escrow.deposit(refundee1, { from: primary, value: amount }); + await this.escrow.deposit(refundee1, { from: owner, value: amount }); await expectRevert(this.escrow.beneficiaryWithdraw(), 'RefundEscrow: beneficiary can only withdraw while closed' ); }); }); - it('only the primary account can enter closed state', async function () { + it('only the owner can enter closed state', async function () { await expectRevert(this.escrow.close({ from: beneficiary }), - 'Secondary: caller is not the primary account' + 'Ownable: caller is not the owner' ); - const { logs } = await this.escrow.close({ from: primary }); + const { logs } = await this.escrow.close({ from: owner }); expectEvent.inLogs(logs, 'RefundsClosed'); }); context('closed state', function () { beforeEach(async function () { - await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount }))); + await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount }))); - await this.escrow.close({ from: primary }); + await this.escrow.close({ from: owner }); }); it('rejects deposits', async function () { - await expectRevert(this.escrow.deposit(refundee1, { from: primary, value: amount }), + await expectRevert(this.escrow.deposit(refundee1, { from: owner, value: amount }), 'RefundEscrow: can only deposit while active' ); }); @@ -86,36 +86,36 @@ describe('RefundEscrow', function () { }); it('prevents entering the refund state', async function () { - await expectRevert(this.escrow.enableRefunds({ from: primary }), + await expectRevert(this.escrow.enableRefunds({ from: owner }), 'RefundEscrow: can only enable refunds while active' ); }); it('prevents re-entering the closed state', async function () { - await expectRevert(this.escrow.close({ from: primary }), + await expectRevert(this.escrow.close({ from: owner }), 'RefundEscrow: can only close while active' ); }); }); - it('only the primary account can enter refund state', async function () { + it('only the owner can enter refund state', async function () { await expectRevert(this.escrow.enableRefunds({ from: beneficiary }), - 'Secondary: caller is not the primary account' + 'Ownable: caller is not the owner' ); - const { logs } = await this.escrow.enableRefunds({ from: primary }); + const { logs } = await this.escrow.enableRefunds({ from: owner }); expectEvent.inLogs(logs, 'RefundsEnabled'); }); context('refund state', function () { beforeEach(async function () { - await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount }))); + await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount }))); - await this.escrow.enableRefunds({ from: primary }); + await this.escrow.enableRefunds({ from: owner }); }); it('rejects deposits', async function () { - await expectRevert(this.escrow.deposit(refundee1, { from: primary, value: amount }), + await expectRevert(this.escrow.deposit(refundee1, { from: owner, value: amount }), 'RefundEscrow: can only deposit while active' ); }); @@ -123,7 +123,7 @@ describe('RefundEscrow', function () { it('refunds refundees', async function () { for (const refundee of [refundee1, refundee2]) { const balanceTracker = await balance.tracker(refundee); - await this.escrow.withdraw(refundee, { from: primary }); + await this.escrow.withdraw(refundee, { from: owner }); expect(await balanceTracker.delta()).to.be.bignumber.equal(amount); } }); @@ -135,13 +135,13 @@ describe('RefundEscrow', function () { }); it('prevents entering the closed state', async function () { - await expectRevert(this.escrow.close({ from: primary }), + await expectRevert(this.escrow.close({ from: owner }), 'RefundEscrow: can only close while active' ); }); it('prevents re-entering the refund state', async function () { - await expectRevert(this.escrow.enableRefunds({ from: primary }), + await expectRevert(this.escrow.enableRefunds({ from: owner }), 'RefundEscrow: can only enable refunds while active' ); });