diff --git a/CHANGELOG.md b/CHANGELOG.md index f2676c54dbf..393bb199bf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,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:: +### Improvements: * Upgraded the minimum compiler version to v0.5.2: this removes many Solidity warnings that were false positives. * `Counter`'s API has been improved, and is now used by `ERC721` (though it is still in `drafts`). * `ERC721`'s transfers are now more gas efficient due to removal of unnecessary `SafeMath` calls. @@ -14,6 +15,9 @@ ### Breaking changes: +## 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. 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/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 669248cab57..fe21e352ab1 100644 --- a/package.json +++ b/package.json @@ -1,11 +1,11 @@ { "name": "openzeppelin-solidity", - "version": "2.1.1", + "version": "2.1.2", "description": "Secure Smart Contract library for Solidity", "files": [ "build", "contracts", - "test" + "test/behaviors" ], "scripts": { "build": "scripts/build.sh", diff --git a/test/access/roles/CapperRole.test.js b/test/access/roles/CapperRole.test.js index a79944a2778..66722fe506b 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('../../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 b2e70b53a45..54d1c0fe5b7 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('../../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 927e46c0ba8..275bf9fce51 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('../../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 317c100d7ed..184929453cd 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('../../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 e59dcd895e8..d757262f294 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('../../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 e578f6fa28f..ee566ff442b 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('../../behaviors/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/behaviors/access/roles/PublicRole.behavior.js similarity index 84% rename from test/access/roles/PublicRole.behavior.js rename to test/behaviors/access/roles/PublicRole.behavior.js index 4c4f3470647..3a673e6a089 100644 --- a/test/access/roles/PublicRole.behavior.js +++ b/test/behaviors/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); diff --git a/test/crowdsale/IndividuallyCappedCrowdsale.test.js b/test/crowdsale/IndividuallyCappedCrowdsale.test.js index 40a3bf71cb1..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('../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 21aa9d18541..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('../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 0b5fe3126e6..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('../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior'); const PausableMock = artifacts.require('PausableMock'); 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)); + }); + }); + } }); diff --git a/test/token/ERC20/ERC20Mintable.test.js b/test/token/ERC20/ERC20Mintable.test.js index 083565d874d..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('../../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 6240f64d16e..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('../../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 d43ea0071f6..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('../../access/roles/PublicRole.behavior'); +const { shouldBehaveLikePublicRole } = require('../../behaviors/access/roles/PublicRole.behavior'); const ERC721PausableMock = artifacts.require('ERC721PausableMock.sol');