From d9a8cd2bef9b58a105d6e0838aa1dd61c2aba528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 17 Jan 2019 20:24:53 -0300 Subject: [PATCH 1/7] Only publish the test suite behavior subdirectory --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 669248cab57..7568a33e4df 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "files": [ "build", "contracts", - "test" + "test/behavior" ], "scripts": { "build": "scripts/build.sh", From 6a658f2ac85b4a4977288caf738c0306c903dae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 17 Jan 2019 20:26:56 -0300 Subject: [PATCH 2/7] Move PublicRole.behavior to behavior directory. --- test/access/roles/CapperRole.test.js | 2 +- test/access/roles/MinterRole.test.js | 2 +- test/access/roles/PauserRole.test.js | 2 +- test/access/roles/SignerRole.test.js | 2 +- test/access/roles/WhitelistAdminRole.test.js | 2 +- test/access/roles/WhitelistedRole.test.js | 2 +- test/{ => behavior}/access/roles/PublicRole.behavior.js | 0 test/crowdsale/IndividuallyCappedCrowdsale.test.js | 2 +- test/drafts/SignatureBouncer.test.js | 2 +- test/lifecycle/Pausable.test.js | 2 +- test/token/ERC20/ERC20Mintable.test.js | 2 +- test/token/ERC20/ERC20Pausable.test.js | 2 +- test/token/ERC721/ERC721Pausable.test.js | 2 +- 13 files changed, 12 insertions(+), 12 deletions(-) rename test/{ => behavior}/access/roles/PublicRole.behavior.js (100%) diff --git a/test/access/roles/CapperRole.test.js b/test/access/roles/CapperRole.test.js index a79944a2778..903d75472c1 100644 --- a/test/access/roles/CapperRole.test.js +++ b/test/access/roles/CapperRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const CapperRoleMock = artifacts.require('CapperRoleMock'); contract('CapperRole', function ([_, capper, otherCapper, ...otherAccounts]) { diff --git a/test/access/roles/MinterRole.test.js b/test/access/roles/MinterRole.test.js index b2e70b53a45..3c38bf8d4ca 100644 --- a/test/access/roles/MinterRole.test.js +++ b/test/access/roles/MinterRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const MinterRoleMock = artifacts.require('MinterRoleMock'); contract('MinterRole', function ([_, minter, otherMinter, ...otherAccounts]) { diff --git a/test/access/roles/PauserRole.test.js b/test/access/roles/PauserRole.test.js index 927e46c0ba8..efb23e538cc 100644 --- a/test/access/roles/PauserRole.test.js +++ b/test/access/roles/PauserRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const PauserRoleMock = artifacts.require('PauserRoleMock'); contract('PauserRole', function ([_, pauser, otherPauser, ...otherAccounts]) { diff --git a/test/access/roles/SignerRole.test.js b/test/access/roles/SignerRole.test.js index 317c100d7ed..c7e086b959e 100644 --- a/test/access/roles/SignerRole.test.js +++ b/test/access/roles/SignerRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const SignerRoleMock = artifacts.require('SignerRoleMock'); contract('SignerRole', function ([_, signer, otherSigner, ...otherAccounts]) { diff --git a/test/access/roles/WhitelistAdminRole.test.js b/test/access/roles/WhitelistAdminRole.test.js index e59dcd895e8..e6fd939140b 100644 --- a/test/access/roles/WhitelistAdminRole.test.js +++ b/test/access/roles/WhitelistAdminRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const WhitelistAdminRoleMock = artifacts.require('WhitelistAdminRoleMock'); contract('WhitelistAdminRole', function ([_, whitelistAdmin, otherWhitelistAdmin, ...otherAccounts]) { diff --git a/test/access/roles/WhitelistedRole.test.js b/test/access/roles/WhitelistedRole.test.js index e578f6fa28f..611540663e8 100644 --- a/test/access/roles/WhitelistedRole.test.js +++ b/test/access/roles/WhitelistedRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const WhitelistedRoleMock = artifacts.require('WhitelistedRoleMock'); contract('WhitelistedRole', function ([_, whitelisted, otherWhitelisted, whitelistAdmin, ...otherAccounts]) { diff --git a/test/access/roles/PublicRole.behavior.js b/test/behavior/access/roles/PublicRole.behavior.js similarity index 100% rename from test/access/roles/PublicRole.behavior.js rename to test/behavior/access/roles/PublicRole.behavior.js diff --git a/test/crowdsale/IndividuallyCappedCrowdsale.test.js b/test/crowdsale/IndividuallyCappedCrowdsale.test.js index 40a3bf71cb1..f9920666df9 100644 --- a/test/crowdsale/IndividuallyCappedCrowdsale.test.js +++ b/test/crowdsale/IndividuallyCappedCrowdsale.test.js @@ -2,7 +2,7 @@ const { BN, ether, shouldFail } = require('openzeppelin-test-helpers'); const IndividuallyCappedCrowdsaleImpl = artifacts.require('IndividuallyCappedCrowdsaleImpl'); const SimpleToken = artifacts.require('SimpleToken'); -const { shouldBehaveLikePublicRole } = require('../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); contract('IndividuallyCappedCrowdsale', function ( [_, capper, otherCapper, wallet, alice, bob, charlie, anyone, ...otherAccounts]) { diff --git a/test/drafts/SignatureBouncer.test.js b/test/drafts/SignatureBouncer.test.js index 21aa9d18541..2527064f4f0 100644 --- a/test/drafts/SignatureBouncer.test.js +++ b/test/drafts/SignatureBouncer.test.js @@ -1,6 +1,6 @@ const { shouldFail } = require('openzeppelin-test-helpers'); const { getSignFor } = require('../helpers/sign'); -const { shouldBehaveLikePublicRole } = require('../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); const SignatureBouncerMock = artifacts.require('SignatureBouncerMock'); diff --git a/test/lifecycle/Pausable.test.js b/test/lifecycle/Pausable.test.js index 0b5fe3126e6..45b6012d690 100644 --- a/test/lifecycle/Pausable.test.js +++ b/test/lifecycle/Pausable.test.js @@ -1,5 +1,5 @@ const { expectEvent, shouldFail } = require('openzeppelin-test-helpers'); -const { shouldBehaveLikePublicRole } = require('../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); const PausableMock = artifacts.require('PausableMock'); diff --git a/test/token/ERC20/ERC20Mintable.test.js b/test/token/ERC20/ERC20Mintable.test.js index 083565d874d..f4cc815fd9e 100644 --- a/test/token/ERC20/ERC20Mintable.test.js +++ b/test/token/ERC20/ERC20Mintable.test.js @@ -1,6 +1,6 @@ const { shouldBehaveLikeERC20Mintable } = require('./behaviors/ERC20Mintable.behavior'); const ERC20MintableMock = artifacts.require('ERC20MintableMock'); -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); contract('ERC20Mintable', function ([_, minter, otherMinter, ...otherAccounts]) { beforeEach(async function () { diff --git a/test/token/ERC20/ERC20Pausable.test.js b/test/token/ERC20/ERC20Pausable.test.js index 6240f64d16e..ea1dd037f4b 100644 --- a/test/token/ERC20/ERC20Pausable.test.js +++ b/test/token/ERC20/ERC20Pausable.test.js @@ -1,7 +1,7 @@ const { BN, expectEvent, shouldFail } = require('openzeppelin-test-helpers'); const ERC20PausableMock = artifacts.require('ERC20PausableMock'); -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherAccount, ...otherAccounts]) { const initialSupply = new BN(100); diff --git a/test/token/ERC721/ERC721Pausable.test.js b/test/token/ERC721/ERC721Pausable.test.js index d43ea0071f6..55d315b94c7 100644 --- a/test/token/ERC721/ERC721Pausable.test.js +++ b/test/token/ERC721/ERC721Pausable.test.js @@ -1,7 +1,7 @@ require('openzeppelin-test-helpers'); const { shouldBehaveLikeERC721PausedToken } = require('./ERC721PausedToken.behavior'); const { shouldBehaveLikeERC721 } = require('./ERC721.behavior'); -const { shouldBehaveLikePublicRole } = require('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); const ERC721PausableMock = artifacts.require('ERC721PausableMock.sol'); From fd808b3ff8c0787608a2a776cc02f07a0ed10ffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 17 Jan 2019 20:27:51 -0300 Subject: [PATCH 3/7] Add some barebones PublicRole.behavior documentation. --- test/behavior/access/roles/PublicRole.behavior.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/behavior/access/roles/PublicRole.behavior.js b/test/behavior/access/roles/PublicRole.behavior.js index 4c4f3470647..3a673e6a089 100644 --- a/test/behavior/access/roles/PublicRole.behavior.js +++ b/test/behavior/access/roles/PublicRole.behavior.js @@ -5,6 +5,21 @@ function capitalize (str) { return str.replace(/\b\w/g, l => l.toUpperCase()); } +// Tests that a role complies with the standard role interface, that is: +// * an onlyRole modifier +// * an isRole function +// * an addRole function, accessible to role havers +// * a renounceRole function +// * roleAdded and roleRemoved events +// Both the modifier and an additional internal remove function are tested through a mock contract that exposes them. +// This mock contract should be stored in this.contract. +// +// @param authorized an account that has the role +// @param otherAuthorized another account that also has the role +// @param anyone an account that doesn't have the role, passed inside an array for ergonomics +// @param rolename a string with the name of the role +// @param manager undefined for regular roles, or a manager account for managed roles. In these, only the manager +// account can create and remove new role bearers. function shouldBehaveLikePublicRole (authorized, otherAuthorized, [anyone], rolename, manager) { rolename = capitalize(rolename); From 1a9cb0786d4d7a4169fe9407f432c159f5b2ad61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 17 Jan 2019 20:36:55 -0300 Subject: [PATCH 4/7] Add changelog entry for PublicRole behavior. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5dfeee4825..5662935480e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 2.2.0 (unreleased) +## 2.1.2 (2019-17-01) + * Removed most of the test suite from the npm package, except `PublicRole.behavior.js`, which may be useful to users testing their own `Roles`. + ## 2.1.1 (2019-04-01) * Version bump to avoid conflict in the npm registry. From 96432bf28e5c58b9407a11464c0baf42a7ee0ad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 18 Jan 2019 15:44:33 -0300 Subject: [PATCH 5/7] Renamed test/behavior to test/behaviors. --- package.json | 2 +- test/access/roles/CapperRole.test.js | 2 +- test/access/roles/MinterRole.test.js | 2 +- test/access/roles/PauserRole.test.js | 2 +- test/access/roles/SignerRole.test.js | 2 +- test/access/roles/WhitelistAdminRole.test.js | 2 +- test/access/roles/WhitelistedRole.test.js | 2 +- .../{behavior => behaviors}/access/roles/PublicRole.behavior.js | 0 test/crowdsale/IndividuallyCappedCrowdsale.test.js | 2 +- test/drafts/SignatureBouncer.test.js | 2 +- test/lifecycle/Pausable.test.js | 2 +- test/token/ERC20/ERC20Mintable.test.js | 2 +- test/token/ERC20/ERC20Pausable.test.js | 2 +- test/token/ERC721/ERC721Pausable.test.js | 2 +- 14 files changed, 13 insertions(+), 13 deletions(-) rename test/{behavior => behaviors}/access/roles/PublicRole.behavior.js (100%) diff --git a/package.json b/package.json index 7568a33e4df..acf3546ce67 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "files": [ "build", "contracts", - "test/behavior" + "test/behaviors" ], "scripts": { "build": "scripts/build.sh", diff --git a/test/access/roles/CapperRole.test.js b/test/access/roles/CapperRole.test.js index 903d75472c1..66722fe506b 100644 --- a/test/access/roles/CapperRole.test.js +++ b/test/access/roles/CapperRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const CapperRoleMock = artifacts.require('CapperRoleMock'); contract('CapperRole', function ([_, capper, otherCapper, ...otherAccounts]) { diff --git a/test/access/roles/MinterRole.test.js b/test/access/roles/MinterRole.test.js index 3c38bf8d4ca..54d1c0fe5b7 100644 --- a/test/access/roles/MinterRole.test.js +++ b/test/access/roles/MinterRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const MinterRoleMock = artifacts.require('MinterRoleMock'); contract('MinterRole', function ([_, minter, otherMinter, ...otherAccounts]) { diff --git a/test/access/roles/PauserRole.test.js b/test/access/roles/PauserRole.test.js index efb23e538cc..275bf9fce51 100644 --- a/test/access/roles/PauserRole.test.js +++ b/test/access/roles/PauserRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const PauserRoleMock = artifacts.require('PauserRoleMock'); contract('PauserRole', function ([_, pauser, otherPauser, ...otherAccounts]) { diff --git a/test/access/roles/SignerRole.test.js b/test/access/roles/SignerRole.test.js index c7e086b959e..184929453cd 100644 --- a/test/access/roles/SignerRole.test.js +++ b/test/access/roles/SignerRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const SignerRoleMock = artifacts.require('SignerRoleMock'); contract('SignerRole', function ([_, signer, otherSigner, ...otherAccounts]) { diff --git a/test/access/roles/WhitelistAdminRole.test.js b/test/access/roles/WhitelistAdminRole.test.js index e6fd939140b..d757262f294 100644 --- a/test/access/roles/WhitelistAdminRole.test.js +++ b/test/access/roles/WhitelistAdminRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const WhitelistAdminRoleMock = artifacts.require('WhitelistAdminRoleMock'); contract('WhitelistAdminRole', function ([_, whitelistAdmin, otherWhitelistAdmin, ...otherAccounts]) { diff --git a/test/access/roles/WhitelistedRole.test.js b/test/access/roles/WhitelistedRole.test.js index 611540663e8..ee566ff442b 100644 --- a/test/access/roles/WhitelistedRole.test.js +++ b/test/access/roles/WhitelistedRole.test.js @@ -1,4 +1,4 @@ -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const WhitelistedRoleMock = artifacts.require('WhitelistedRoleMock'); contract('WhitelistedRole', function ([_, whitelisted, otherWhitelisted, whitelistAdmin, ...otherAccounts]) { diff --git a/test/behavior/access/roles/PublicRole.behavior.js b/test/behaviors/access/roles/PublicRole.behavior.js similarity index 100% rename from test/behavior/access/roles/PublicRole.behavior.js rename to test/behaviors/access/roles/PublicRole.behavior.js diff --git a/test/crowdsale/IndividuallyCappedCrowdsale.test.js b/test/crowdsale/IndividuallyCappedCrowdsale.test.js index f9920666df9..086f1b59180 100644 --- a/test/crowdsale/IndividuallyCappedCrowdsale.test.js +++ b/test/crowdsale/IndividuallyCappedCrowdsale.test.js @@ -2,7 +2,7 @@ const { BN, ether, shouldFail } = require('openzeppelin-test-helpers'); const IndividuallyCappedCrowdsaleImpl = artifacts.require('IndividuallyCappedCrowdsaleImpl'); const SimpleToken = artifacts.require('SimpleToken'); -const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior'); contract('IndividuallyCappedCrowdsale', function ( [_, capper, otherCapper, wallet, alice, bob, charlie, anyone, ...otherAccounts]) { diff --git a/test/drafts/SignatureBouncer.test.js b/test/drafts/SignatureBouncer.test.js index 2527064f4f0..32866378e4b 100644 --- a/test/drafts/SignatureBouncer.test.js +++ b/test/drafts/SignatureBouncer.test.js @@ -1,6 +1,6 @@ const { shouldFail } = require('openzeppelin-test-helpers'); const { getSignFor } = require('../helpers/sign'); -const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior'); const SignatureBouncerMock = artifacts.require('SignatureBouncerMock'); diff --git a/test/lifecycle/Pausable.test.js b/test/lifecycle/Pausable.test.js index 45b6012d690..337adcfc7e3 100644 --- a/test/lifecycle/Pausable.test.js +++ b/test/lifecycle/Pausable.test.js @@ -1,5 +1,5 @@ const { expectEvent, shouldFail } = require('openzeppelin-test-helpers'); -const { shouldBehaveLikePublicRole } = require('../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior'); const PausableMock = artifacts.require('PausableMock'); diff --git a/test/token/ERC20/ERC20Mintable.test.js b/test/token/ERC20/ERC20Mintable.test.js index f4cc815fd9e..659276ddd23 100644 --- a/test/token/ERC20/ERC20Mintable.test.js +++ b/test/token/ERC20/ERC20Mintable.test.js @@ -1,6 +1,6 @@ const { shouldBehaveLikeERC20Mintable } = require('./behaviors/ERC20Mintable.behavior'); const ERC20MintableMock = artifacts.require('ERC20MintableMock'); -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); contract('ERC20Mintable', function ([_, minter, otherMinter, ...otherAccounts]) { beforeEach(async function () { diff --git a/test/token/ERC20/ERC20Pausable.test.js b/test/token/ERC20/ERC20Pausable.test.js index ea1dd037f4b..cee627ab301 100644 --- a/test/token/ERC20/ERC20Pausable.test.js +++ b/test/token/ERC20/ERC20Pausable.test.js @@ -1,7 +1,7 @@ const { BN, expectEvent, shouldFail } = require('openzeppelin-test-helpers'); const ERC20PausableMock = artifacts.require('ERC20PausableMock'); -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherAccount, ...otherAccounts]) { const initialSupply = new BN(100); diff --git a/test/token/ERC721/ERC721Pausable.test.js b/test/token/ERC721/ERC721Pausable.test.js index 55d315b94c7..840e1a90553 100644 --- a/test/token/ERC721/ERC721Pausable.test.js +++ b/test/token/ERC721/ERC721Pausable.test.js @@ -1,7 +1,7 @@ require('openzeppelin-test-helpers'); const { shouldBehaveLikeERC721PausedToken } = require('./ERC721PausedToken.behavior'); const { shouldBehaveLikeERC721 } = require('./ERC721.behavior'); -const { shouldBehaveLikePublicRole } = require('../../behavior/access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const ERC721PausableMock = artifacts.require('ERC721PausableMock.sol'); From 8617c4b4c83fdf6b229c60e9f1c9be0dc5b961f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 18 Jan 2019 15:45:25 -0300 Subject: [PATCH 6/7] Release v2.1.2 --- ethpm.json | 2 +- package-lock.json | 2 +- package.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ethpm.json b/ethpm.json index 03149be3d19..d1234541f40 100644 --- a/ethpm.json +++ b/ethpm.json @@ -1,6 +1,6 @@ { "package_name": "zeppelin", - "version": "2.1.1", + "version": "2.1.2", "description": "Secure Smart Contract library for Solidity", "authors": [ "OpenZeppelin Community " diff --git a/package-lock.json b/package-lock.json index bc241ff27b8..ed84dd244c8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "openzeppelin-solidity", - "version": "2.1.1", + "version": "2.1.2", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index acf3546ce67..fe21e352ab1 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openzeppelin-solidity", - "version": "2.1.1", + "version": "2.1.2", "description": "Secure Smart Contract library for Solidity", "files": [ "build", From 3a5da758767cfbcf256bdd147c56dbb0a2541569 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 21 Jan 2019 17:23:38 -0300 Subject: [PATCH 7/7] ERC20._approve (#1609) * Add ERC20._approve. * Add ERC20._approve tests. * Fix linter error. * Require owner in _approve to be non-zero. --- CHANGELOG.md | 3 + contracts/mocks/ERC20Mock.sol | 4 + contracts/token/ERC20/ERC20.sol | 35 ++++--- test/token/ERC20/ERC20.test.js | 179 +++++++++++++++++--------------- 4 files changed, 122 insertions(+), 99 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6aed3cc898d..1fa60ab87b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 2.2.0 (unreleased) +### New features: + * `ERC20`: added internal `_approve(address owner, address spender, uint256 value)`, allowing derived contracts to set the allowance of arbitrary accounts. + ### Improvements: * Upgraded the minimum compiler version to v0.5.2: this removes many Solidity warnings that were false positives. * Fixed variable shadowing issues. diff --git a/contracts/mocks/ERC20Mock.sol b/contracts/mocks/ERC20Mock.sol index b7e48a563db..23589edb276 100644 --- a/contracts/mocks/ERC20Mock.sol +++ b/contracts/mocks/ERC20Mock.sol @@ -19,4 +19,8 @@ contract ERC20Mock is ERC20 { function burnFrom(address account, uint256 amount) public { _burnFrom(account, amount); } + + function approveInternal(address owner, address spender, uint256 value) public { + _approve(owner, spender, value); + } } diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 9d5d7b7fa81..519271ff5a8 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -70,10 +70,7 @@ contract ERC20 is IERC20 { * @param value The amount of tokens to be spent. */ function approve(address spender, uint256 value) public returns (bool) { - require(spender != address(0)); - - _allowed[msg.sender][spender] = value; - emit Approval(msg.sender, spender, value); + _approve(msg.sender, spender, value); return true; } @@ -86,9 +83,8 @@ contract ERC20 is IERC20 { * @param value uint256 the amount of tokens to be transferred */ function transferFrom(address from, address to, uint256 value) public returns (bool) { - _allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value); _transfer(from, to, value); - emit Approval(from, msg.sender, _allowed[from][msg.sender]); + _approve(from, msg.sender, _allowed[from][msg.sender].sub(value)); return true; } @@ -103,10 +99,7 @@ contract ERC20 is IERC20 { * @param addedValue The amount of tokens to increase the allowance by. */ function increaseAllowance(address spender, uint256 addedValue) public returns (bool) { - require(spender != address(0)); - - _allowed[msg.sender][spender] = _allowed[msg.sender][spender].add(addedValue); - emit Approval(msg.sender, spender, _allowed[msg.sender][spender]); + _approve(msg.sender, spender, _allowed[msg.sender][spender].add(addedValue)); return true; } @@ -121,10 +114,7 @@ contract ERC20 is IERC20 { * @param subtractedValue The amount of tokens to decrease the allowance by. */ function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) { - require(spender != address(0)); - - _allowed[msg.sender][spender] = _allowed[msg.sender][spender].sub(subtractedValue); - emit Approval(msg.sender, spender, _allowed[msg.sender][spender]); + _approve(msg.sender, spender, _allowed[msg.sender][spender].sub(subtractedValue)); return true; } @@ -171,6 +161,20 @@ contract ERC20 is IERC20 { emit Transfer(account, address(0), value); } + /** + * @dev Approve an address to spend another addresses' tokens. + * @param owner The address that owns the tokens. + * @param spender The address that will spend the tokens. + * @param value The number of tokens that can be spent. + */ + function _approve(address owner, address spender, uint256 value) internal { + require(spender != address(0)); + require(owner != address(0)); + + _allowed[owner][spender] = value; + emit Approval(owner, spender, value); + } + /** * @dev Internal function that burns an amount of the token of a given * account, deducting from the sender's allowance for said account. Uses the @@ -180,8 +184,7 @@ contract ERC20 is IERC20 { * @param value The amount that will be burnt. */ function _burnFrom(address account, uint256 value) internal { - _allowed[account][msg.sender] = _allowed[account][msg.sender].sub(value); _burn(account, value); - emit Approval(account, msg.sender, _allowed[account][msg.sender]); + _approve(account, msg.sender, _allowed[account][msg.sender].sub(value)); } } diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 1c71cd21f95..ac82c8dccd6 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -73,89 +73,6 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { }); }); - describe('approve', function () { - describe('when the spender is not the zero address', function () { - const spender = recipient; - - describe('when the sender has enough balance', function () { - const amount = initialSupply; - - it('emits an approval event', async function () { - const { logs } = await this.token.approve(spender, amount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); - }); - - describe('when there was no approved amount before', function () { - it('approves the requested amount', async function () { - await this.token.approve(spender, amount, { from: initialHolder }); - - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal(amount); - }); - }); - - describe('when the spender had an approved amount', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); - }); - - it('approves the requested amount and replaces the previous one', async function () { - await this.token.approve(spender, amount, { from: initialHolder }); - - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal(amount); - }); - }); - }); - - describe('when the sender does not have enough balance', function () { - const amount = initialSupply.addn(1); - - it('emits an approval event', async function () { - const { logs } = await this.token.approve(spender, amount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); - }); - - describe('when there was no approved amount before', function () { - it('approves the requested amount', async function () { - await this.token.approve(spender, amount, { from: initialHolder }); - - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal(amount); - }); - }); - - describe('when the spender had an approved amount', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); - }); - - it('approves the requested amount and replaces the previous one', async function () { - await this.token.approve(spender, amount, { from: initialHolder }); - - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal(amount); - }); - }); - }); - }); - - describe('when the spender is the zero address', function () { - const amount = initialSupply; - const spender = ZERO_ADDRESS; - - it('reverts', async function () { - await shouldFail.reverting(this.token.approve(spender, amount, { from: initialHolder })); - }); - }); - }); - describe('transfer from', function () { const spender = recipient; @@ -546,4 +463,100 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { describeBurnFrom('for less amount than allowance', allowance.subn(1)); }); }); + + describe('approve', function () { + testApprove(initialHolder, recipient, initialSupply, function (owner, spender, amount) { + return this.token.approve(spender, amount, { from: owner }); + }); + }); + + describe('_approve', function () { + testApprove(initialHolder, recipient, initialSupply, function (owner, spender, amount) { + return this.token.approveInternal(owner, spender, amount); + }); + + describe('when the owner is the zero address', function () { + it('reverts', async function () { + await shouldFail.reverting(this.token.approveInternal(ZERO_ADDRESS, recipient, initialSupply)); + }); + }); + }); + + function testApprove (owner, spender, supply, approve) { + describe('when the spender is not the zero address', function () { + describe('when the sender has enough balance', function () { + const amount = supply; + + it('emits an approval event', async function () { + const { logs } = await approve.call(this, owner, spender, amount); + + expectEvent.inLogs(logs, 'Approval', { + owner: owner, + spender: spender, + value: amount, + }); + }); + + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await approve.call(this, owner, spender, amount); + + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount); + }); + }); + + describe('when the spender had an approved amount', function () { + beforeEach(async function () { + await approve.call(this, owner, spender, new BN(1)); + }); + + it('approves the requested amount and replaces the previous one', async function () { + await approve.call(this, owner, spender, amount); + + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount); + }); + }); + }); + + describe('when the sender does not have enough balance', function () { + const amount = supply.addn(1); + + it('emits an approval event', async function () { + const { logs } = await approve.call(this, owner, spender, amount); + + expectEvent.inLogs(logs, 'Approval', { + owner: owner, + spender: spender, + value: amount, + }); + }); + + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await approve.call(this, owner, spender, amount); + + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount); + }); + }); + + describe('when the spender had an approved amount', function () { + beforeEach(async function () { + await approve.call(this, owner, spender, new BN(1)); + }); + + it('approves the requested amount and replaces the previous one', async function () { + await approve.call(this, owner, spender, amount); + + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount); + }); + }); + }); + }); + + describe('when the spender is the zero address', function () { + it('reverts', async function () { + await shouldFail.reverting(approve.call(this, owner, ZERO_ADDRESS, supply)); + }); + }); + } });