From cb75f007ea992bcb44f0036cabacb4cc713bef29 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 13:16:28 -0300 Subject: [PATCH 01/11] make StandardToken state variables private --- contracts/examples/SimpleToken.sol | 4 +--- contracts/mocks/BurnableTokenMock.sol | 3 +-- contracts/mocks/ERC223TokenMock.sol | 3 +-- contracts/mocks/PausableTokenMock.sol | 2 +- contracts/mocks/StandardTokenMock.sol | 3 +-- contracts/token/ERC20/BurnableToken.sol | 16 +++------------- contracts/token/ERC20/CappedToken.sol | 2 +- contracts/token/ERC20/MintableToken.sol | 4 +--- contracts/token/ERC20/StandardToken.sol | 24 +++++++++++++++++++++--- 9 files changed, 31 insertions(+), 30 deletions(-) diff --git a/contracts/examples/SimpleToken.sol b/contracts/examples/SimpleToken.sol index 835396f2ef2..73df121dd8f 100644 --- a/contracts/examples/SimpleToken.sol +++ b/contracts/examples/SimpleToken.sol @@ -22,9 +22,7 @@ contract SimpleToken is StandardToken { * @dev Constructor that gives msg.sender all of existing tokens. */ constructor() public { - totalSupply_ = INITIAL_SUPPLY; - balances[msg.sender] = INITIAL_SUPPLY; - emit Transfer(address(0), msg.sender, INITIAL_SUPPLY); + _mint(msg.sender, INITIAL_SUPPLY); } } diff --git a/contracts/mocks/BurnableTokenMock.sol b/contracts/mocks/BurnableTokenMock.sol index f7bdd59d9da..a148f17f47e 100644 --- a/contracts/mocks/BurnableTokenMock.sol +++ b/contracts/mocks/BurnableTokenMock.sol @@ -6,8 +6,7 @@ import "../token/ERC20/BurnableToken.sol"; contract BurnableTokenMock is BurnableToken { constructor(address _initialAccount, uint256 _initialBalance) public { - balances[_initialAccount] = _initialBalance; - totalSupply_ = _initialBalance; + _mint(_initialAccount, _initialBalance); } } diff --git a/contracts/mocks/ERC223TokenMock.sol b/contracts/mocks/ERC223TokenMock.sol index 410348cc044..2f92bc5f1f4 100644 --- a/contracts/mocks/ERC223TokenMock.sol +++ b/contracts/mocks/ERC223TokenMock.sol @@ -11,8 +11,7 @@ contract ERC223ContractInterface { contract ERC223TokenMock is StandardToken { constructor(address _initialAccount, uint256 _initialBalance) public { - balances[_initialAccount] = _initialBalance; - totalSupply_ = _initialBalance; + _mint(_initialAccount, _initialBalance); } // ERC223 compatible transfer function (except the name) diff --git a/contracts/mocks/PausableTokenMock.sol b/contracts/mocks/PausableTokenMock.sol index f1a1d53ddb4..24ef281bade 100644 --- a/contracts/mocks/PausableTokenMock.sol +++ b/contracts/mocks/PausableTokenMock.sol @@ -7,7 +7,7 @@ import "../token/ERC20/PausableToken.sol"; contract PausableTokenMock is PausableToken { constructor(address _initialAccount, uint _initialBalance) public { - balances[_initialAccount] = _initialBalance; + _mint(_initialAccount, _initialBalance); } } diff --git a/contracts/mocks/StandardTokenMock.sol b/contracts/mocks/StandardTokenMock.sol index 6a7dd5aa163..7fe1104e2d9 100644 --- a/contracts/mocks/StandardTokenMock.sol +++ b/contracts/mocks/StandardTokenMock.sol @@ -7,8 +7,7 @@ import "../token/ERC20/StandardToken.sol"; contract StandardTokenMock is StandardToken { constructor(address _initialAccount, uint256 _initialBalance) public { - balances[_initialAccount] = _initialBalance; - totalSupply_ = _initialBalance; + _mint(_initialAccount, _initialBalance); } } diff --git a/contracts/token/ERC20/BurnableToken.sol b/contracts/token/ERC20/BurnableToken.sol index ad49ff11dbc..329f7b66a4e 100644 --- a/contracts/token/ERC20/BurnableToken.sol +++ b/contracts/token/ERC20/BurnableToken.sol @@ -25,21 +25,11 @@ contract BurnableToken is StandardToken { * @param _value uint256 The amount of token to be burned */ function burnFrom(address _from, uint256 _value) public { - require(_value <= allowed[_from][msg.sender]); - // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, - // this function needs to emit an event with the updated approval. - allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); - _burn(_from, _value); + _burnFrom(_from, _value); } function _burn(address _who, uint256 _value) internal { - require(_value <= balances[_who]); - // no need to require value <= totalSupply, since that would imply the - // sender's balance is greater than the totalSupply, which *should* be an assertion failure - - balances[_who] = balances[_who].sub(_value); - totalSupply_ = totalSupply_.sub(_value); + super._burn(_who, _value); emit Burn(_who, _value); - emit Transfer(_who, address(0), _value); } -} \ No newline at end of file +} diff --git a/contracts/token/ERC20/CappedToken.sol b/contracts/token/ERC20/CappedToken.sol index 497b6fdc418..1af8bdcb647 100644 --- a/contracts/token/ERC20/CappedToken.sol +++ b/contracts/token/ERC20/CappedToken.sol @@ -29,7 +29,7 @@ contract CappedToken is MintableToken { public returns (bool) { - require(totalSupply_.add(_amount) <= cap); + require(totalSupply().add(_amount) <= cap); return super.mint(_to, _amount); } diff --git a/contracts/token/ERC20/MintableToken.sol b/contracts/token/ERC20/MintableToken.sol index 1821058ff94..18abb83b260 100644 --- a/contracts/token/ERC20/MintableToken.sol +++ b/contracts/token/ERC20/MintableToken.sol @@ -41,10 +41,8 @@ contract MintableToken is StandardToken, Ownable { canMint returns (bool) { - totalSupply_ = totalSupply_.add(_amount); - balances[_to] = balances[_to].add(_amount); emit Mint(_to, _amount); - emit Transfer(address(0), _to, _amount); + _mint(_to, _amount); return true; } diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 88b215473d4..e54d9a2b878 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -14,11 +14,11 @@ import "../../math/SafeMath.sol"; contract StandardToken is ERC20 { using SafeMath for uint256; - mapping(address => uint256) balances; + mapping (address => uint256) private balances; - mapping (address => mapping (address => uint256)) internal allowed; + mapping (address => mapping (address => uint256)) private allowed; - uint256 totalSupply_; + uint256 private totalSupply_; /** * @dev Total number of tokens in existence @@ -156,4 +156,22 @@ contract StandardToken is ERC20 { return true; } + function _mint(address _account, uint256 _amount) internal { + totalSupply_ = totalSupply_.add(_amount); + balances[_account] = balances[_account].add(_amount); + emit Transfer(address(0), _account, _amount); + } + + function _burn(address _account, uint256 _amount) internal { + totalSupply_ = totalSupply_.sub(_amount); + balances[_account] = balances[_account].sub(_amount); + emit Transfer(_account, address(0), _amount); + } + + function _burnFrom(address _account, uint256 _amount) internal { + // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, + // this function needs to emit an event with the updated approval. + allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount); + _burn(_account, _amount); + } } From 371fe3e567e0ec894caa8d5f14ad590b18c5ae86 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 13:22:32 -0300 Subject: [PATCH 02/11] simplify mocks --- contracts/mocks/BurnableTokenMock.sol | 12 ++++++------ contracts/mocks/ERC223TokenMock.sol | 10 ++++++---- contracts/mocks/PausableTokenMock.sol | 10 ++++++---- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/contracts/mocks/BurnableTokenMock.sol b/contracts/mocks/BurnableTokenMock.sol index a148f17f47e..8f6832f7f3e 100644 --- a/contracts/mocks/BurnableTokenMock.sol +++ b/contracts/mocks/BurnableTokenMock.sol @@ -1,12 +1,12 @@ pragma solidity ^0.4.24; +import "./StandardTokenMock.sol"; import "../token/ERC20/BurnableToken.sol"; -contract BurnableTokenMock is BurnableToken { - - constructor(address _initialAccount, uint256 _initialBalance) public { - _mint(_initialAccount, _initialBalance); - } - +contract BurnableTokenMock is StandardTokenMock, BurnableToken { + constructor(address _initialAccount, uint256 _initialBalance) + StandardTokenMock(_initialAccount, _initialBalance) + public + { } } diff --git a/contracts/mocks/ERC223TokenMock.sol b/contracts/mocks/ERC223TokenMock.sol index 2f92bc5f1f4..47cf164de44 100644 --- a/contracts/mocks/ERC223TokenMock.sol +++ b/contracts/mocks/ERC223TokenMock.sol @@ -1,5 +1,6 @@ pragma solidity ^0.4.24; +import "./StandardTokenMock.sol"; import "../token/ERC20/StandardToken.sol"; @@ -8,11 +9,12 @@ contract ERC223ContractInterface { } -contract ERC223TokenMock is StandardToken { +contract ERC223TokenMock is StandardTokenMock { - constructor(address _initialAccount, uint256 _initialBalance) public { - _mint(_initialAccount, _initialBalance); - } + constructor(address _initialAccount, uint256 _initialBalance) + StandardTokenMock(_initialAccount, _initialBalance) + public + { } // ERC223 compatible transfer function (except the name) function transferERC223(address _to, uint256 _value, bytes _data) public diff --git a/contracts/mocks/PausableTokenMock.sol b/contracts/mocks/PausableTokenMock.sol index 24ef281bade..e8ec8ccff46 100644 --- a/contracts/mocks/PausableTokenMock.sol +++ b/contracts/mocks/PausableTokenMock.sol @@ -1,13 +1,15 @@ pragma solidity ^0.4.24; +import "./StandardTokenMock.sol"; import "../token/ERC20/PausableToken.sol"; // mock class using PausableToken -contract PausableTokenMock is PausableToken { +contract PausableTokenMock is StandardTokenMock, PausableToken { - constructor(address _initialAccount, uint _initialBalance) public { - _mint(_initialAccount, _initialBalance); - } + constructor(address _initialAccount, uint256 _initialBalance) + StandardTokenMock(_initialAccount, _initialBalance) + public + { } } From 47621917a79f1e23dc0a38a832d7302d03f3ece5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 17:21:59 -0300 Subject: [PATCH 03/11] document new internal functions --- contracts/token/ERC20/StandardToken.sol | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index e54d9a2b878..079bf4745d7 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -156,18 +156,37 @@ contract StandardToken is ERC20 { return true; } + /** + * @dev Internal function that mints an amount of the token and assigns it to + * an account. This encapsulates the modification of balances such that the + * proper events are emitted. + * @param _account The account that will receive the created tokens. + * @param _amount The amount that will be created. + */ function _mint(address _account, uint256 _amount) internal { totalSupply_ = totalSupply_.add(_amount); balances[_account] = balances[_account].add(_amount); emit Transfer(address(0), _account, _amount); } + /** + * @dev Internal function that burns an amount of the token of a given + * account. + * @param _account The account whose tokens will be burnt. + * @param _amount The amount that will be burnt. + */ function _burn(address _account, uint256 _amount) internal { totalSupply_ = totalSupply_.sub(_amount); balances[_account] = balances[_account].sub(_amount); emit Transfer(_account, address(0), _amount); } + /** + * @dev Internal function that burns an amount of the token of a given + * account, deducting from the allowance of the transaction sender. + * @param _account The account whose tokens will be burnt. + * @param _amount The amount that will be burnt. + */ function _burnFrom(address _account, uint256 _amount) internal { // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, // this function needs to emit an event with the updated approval. From 5f89cdfcb51fed3598272a16700a64905a57f586 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 17:22:13 -0300 Subject: [PATCH 04/11] fix link to ERC20 document --- contracts/token/ERC20/StandardToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 079bf4745d7..6afbc00146d 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -8,7 +8,7 @@ import "../../math/SafeMath.sol"; * @title Standard ERC20 token * * @dev Implementation of the basic standard token. - * https://github.com/ethereum/EIPs/issues/20 + * https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md * Based on code by FirstBlood: https://github.com/Firstbloodio/token/blob/master/smart_contract/FirstBloodToken.sol */ contract StandardToken is ERC20 { From 8120c46e0cd99fe116a2e2e4bf22f53a99514d2f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 21:12:49 -0300 Subject: [PATCH 05/11] revert order of Transfer and Mint events --- contracts/token/ERC20/MintableToken.sol | 2 +- test/token/ERC20/CappedToken.behavior.js | 5 +++-- test/token/ERC20/MintableToken.behavior.js | 18 +++++++++++++----- test/token/ERC20/StandardToken.test.js | 13 ++++++++----- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/contracts/token/ERC20/MintableToken.sol b/contracts/token/ERC20/MintableToken.sol index 18abb83b260..ba1ca68336f 100644 --- a/contracts/token/ERC20/MintableToken.sol +++ b/contracts/token/ERC20/MintableToken.sol @@ -41,8 +41,8 @@ contract MintableToken is StandardToken, Ownable { canMint returns (bool) { - emit Mint(_to, _amount); _mint(_to, _amount); + emit Mint(_to, _amount); return true; } diff --git a/test/token/ERC20/CappedToken.behavior.js b/test/token/ERC20/CappedToken.behavior.js index 249d15559db..58a64d8b2c0 100644 --- a/test/token/ERC20/CappedToken.behavior.js +++ b/test/token/ERC20/CappedToken.behavior.js @@ -1,4 +1,5 @@ const { expectThrow } = require('../../helpers/expectThrow'); +const expectEvent = require('../../helpers/expectEvent'); const BigNumber = web3.BigNumber; @@ -15,8 +16,8 @@ function shouldBehaveLikeCappedToken (minter, [anyone], cap) { }); it('should mint when amount is less than cap', async function () { - const result = await this.token.mint(anyone, cap.sub(1), { from }); - result.logs[0].event.should.equal('Mint'); + const { logs } = await this.token.mint(anyone, cap.sub(1), { from }); + expectEvent.inLogs(logs, 'Mint'); }); it('should fail to mint if the ammount exceeds the cap', async function () { diff --git a/test/token/ERC20/MintableToken.behavior.js b/test/token/ERC20/MintableToken.behavior.js index 7d34e2719b2..c037d92ff62 100644 --- a/test/token/ERC20/MintableToken.behavior.js +++ b/test/token/ERC20/MintableToken.behavior.js @@ -1,4 +1,5 @@ const { assertRevert } = require('../../helpers/assertRevert'); +const expectEvent = require('../../helpers/expectEvent'); const BigNumber = web3.BigNumber; @@ -7,6 +8,8 @@ require('chai') .should(); function shouldBehaveLikeMintableToken (owner, minter, [anyone]) { + const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; + describe('as a basic mintable token', function () { describe('after token creation', function () { it('sender should be token owner', async function () { @@ -104,11 +107,16 @@ function shouldBehaveLikeMintableToken (owner, minter, [anyone]) { it('emits a mint and a transfer event', async function () { const { logs } = await this.token.mint(owner, amount, { from }); - logs.length.should.eq(2); - logs[0].event.should.eq('Mint'); - logs[0].args.to.should.eq(owner); - logs[0].args.amount.should.be.bignumber.equal(amount); - logs[1].event.should.eq('Transfer'); + const mintEvent = expectEvent.inLogs(logs, 'Mint', { + to: owner, + }); + mintEvent.args.amount.should.be.bignumber.equal(amount); + + const transferEvent = expectEvent.inLogs(logs, 'Transfer', { + from: ZERO_ADDRESS, + to: owner, + }); + transferEvent.args.value.should.be.bignumber.equal(amount); }); }); diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index e1f35750200..ae4619cbafe 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -1,4 +1,6 @@ const { assertRevert } = require('../../helpers/assertRevert'); +const expectEvent = require('../../helpers/expectEvent'); + const StandardToken = artifacts.require('StandardTokenMock'); const BigNumber = web3.BigNumber; @@ -68,11 +70,12 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { it('emits a transfer event', async function () { const { logs } = await this.token.transfer(to, amount, { from: owner }); - logs.length.should.eq(1); - logs[0].event.should.eq('Transfer'); - logs[0].args.from.should.eq(owner); - logs[0].args.to.should.eq(to); - logs[0].args.value.should.be.bignumber.equal(amount); + const event = expectEvent.inLogs(logs, 'Transfer', { + from: owner, + to: to, + }); + + event.args.value.should.be.bignumber.equal(amount); }); }); }); From dc8b3bcb20f7461c4b0853dfe4b29b28ed666cc8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 21:34:56 -0300 Subject: [PATCH 06/11] Revert "simplify mocks" This reverts commit 371fe3e567e0ec894caa8d5f14ad590b18c5ae86. --- contracts/mocks/BurnableTokenMock.sol | 12 ++++++------ contracts/mocks/ERC223TokenMock.sol | 10 ++++------ contracts/mocks/PausableTokenMock.sol | 10 ++++------ 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/contracts/mocks/BurnableTokenMock.sol b/contracts/mocks/BurnableTokenMock.sol index 8f6832f7f3e..a148f17f47e 100644 --- a/contracts/mocks/BurnableTokenMock.sol +++ b/contracts/mocks/BurnableTokenMock.sol @@ -1,12 +1,12 @@ pragma solidity ^0.4.24; -import "./StandardTokenMock.sol"; import "../token/ERC20/BurnableToken.sol"; -contract BurnableTokenMock is StandardTokenMock, BurnableToken { - constructor(address _initialAccount, uint256 _initialBalance) - StandardTokenMock(_initialAccount, _initialBalance) - public - { } +contract BurnableTokenMock is BurnableToken { + + constructor(address _initialAccount, uint256 _initialBalance) public { + _mint(_initialAccount, _initialBalance); + } + } diff --git a/contracts/mocks/ERC223TokenMock.sol b/contracts/mocks/ERC223TokenMock.sol index 47cf164de44..2f92bc5f1f4 100644 --- a/contracts/mocks/ERC223TokenMock.sol +++ b/contracts/mocks/ERC223TokenMock.sol @@ -1,6 +1,5 @@ pragma solidity ^0.4.24; -import "./StandardTokenMock.sol"; import "../token/ERC20/StandardToken.sol"; @@ -9,12 +8,11 @@ contract ERC223ContractInterface { } -contract ERC223TokenMock is StandardTokenMock { +contract ERC223TokenMock is StandardToken { - constructor(address _initialAccount, uint256 _initialBalance) - StandardTokenMock(_initialAccount, _initialBalance) - public - { } + constructor(address _initialAccount, uint256 _initialBalance) public { + _mint(_initialAccount, _initialBalance); + } // ERC223 compatible transfer function (except the name) function transferERC223(address _to, uint256 _value, bytes _data) public diff --git a/contracts/mocks/PausableTokenMock.sol b/contracts/mocks/PausableTokenMock.sol index e8ec8ccff46..24ef281bade 100644 --- a/contracts/mocks/PausableTokenMock.sol +++ b/contracts/mocks/PausableTokenMock.sol @@ -1,15 +1,13 @@ pragma solidity ^0.4.24; -import "./StandardTokenMock.sol"; import "../token/ERC20/PausableToken.sol"; // mock class using PausableToken -contract PausableTokenMock is StandardTokenMock, PausableToken { +contract PausableTokenMock is PausableToken { - constructor(address _initialAccount, uint256 _initialBalance) - StandardTokenMock(_initialAccount, _initialBalance) - public - { } + constructor(address _initialAccount, uint _initialBalance) public { + _mint(_initialAccount, _initialBalance); + } } From 2cd86bf53644ad2eae3a0c30c1e43250b02c5113 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 12 Aug 2018 23:05:46 -0300 Subject: [PATCH 07/11] add tests for new internal functions --- contracts/mocks/StandardTokenMock.sol | 12 ++++ test/token/ERC20/StandardToken.test.js | 98 ++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/contracts/mocks/StandardTokenMock.sol b/contracts/mocks/StandardTokenMock.sol index 7fe1104e2d9..e4420c470bc 100644 --- a/contracts/mocks/StandardTokenMock.sol +++ b/contracts/mocks/StandardTokenMock.sol @@ -10,4 +10,16 @@ contract StandardTokenMock is StandardToken { _mint(_initialAccount, _initialBalance); } + function mint(address _account, uint256 _amount) public { + _mint(_account, _amount); + } + + function burn(address _account, uint256 _amount) public { + _burn(_account, _amount); + } + + function burnFrom(address _account, uint256 _amount) public { + _burnFrom(_account, _amount); + } + } diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index ae4619cbafe..e3f4cc82a00 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -489,4 +489,102 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }); }); }); + + describe('_mint', function () { + const initialSupply = new BigNumber(100); + const amount = new BigNumber(50); + + beforeEach('minting', async function () { + const { logs } = await this.token.mint(recipient, amount); + this.logs = logs; + }); + + it('increments totalSupply', async function () { + const expectedSupply = initialSupply.plus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); + + it('increments recipient balance', async function () { + (await this.token.balanceOf(recipient)).should.be.bignumber.equal(amount); + }); + + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: ZERO_ADDRESS, + to: recipient, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); + }); + + describe('_burn', function () { + const initialSupply = new BigNumber(100); + const amount = new BigNumber(50); + + beforeEach('burning', async function () { + const { logs } = await this.token.burn(owner, amount); + this.logs = logs; + }); + + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); + + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); + }); + + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); + }); + + describe('_burnFrom', function () { + const initialSupply = new BigNumber(100); + const allowance = new BigNumber(70); + const amount = new BigNumber(50); + + const spender = anotherAccount; + + beforeEach('approving', async function () { + await this.token.approve(spender, allowance, { from: owner }); + }); + + beforeEach('burning', async function () { + const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); + this.logs = logs; + }); + + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); + + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); + }); + + it('decrements spender allowance', async function () { + const expectedAllowance = allowance.minus(amount); + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); + }); + + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); + }); }); From 53f7b0e3d5a07b5a0b65d44c6799d1f2f5b7f018 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 14 Aug 2018 18:16:38 -0300 Subject: [PATCH 08/11] add check for null account --- contracts/token/ERC20/StandardToken.sol | 3 + test/token/ERC20/StandardToken.test.js | 120 ++++++++++++++---------- 2 files changed, 72 insertions(+), 51 deletions(-) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 6afbc00146d..39098a68b58 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -164,6 +164,7 @@ contract StandardToken is ERC20 { * @param _amount The amount that will be created. */ function _mint(address _account, uint256 _amount) internal { + require(_account != 0); totalSupply_ = totalSupply_.add(_amount); balances[_account] = balances[_account].add(_amount); emit Transfer(address(0), _account, _amount); @@ -176,6 +177,7 @@ contract StandardToken is ERC20 { * @param _amount The amount that will be burnt. */ function _burn(address _account, uint256 _amount) internal { + require(_account != 0); totalSupply_ = totalSupply_.sub(_amount); balances[_account] = balances[_account].sub(_amount); emit Transfer(_account, address(0), _amount); @@ -188,6 +190,7 @@ contract StandardToken is ERC20 { * @param _amount The amount that will be burnt. */ function _burnFrom(address _account, uint256 _amount) internal { + require(_account != 0); // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, // this function needs to emit an event with the updated approval. allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount); diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index e3f4cc82a00..6c1dee429ab 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -494,27 +494,33 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { const initialSupply = new BigNumber(100); const amount = new BigNumber(50); - beforeEach('minting', async function () { - const { logs } = await this.token.mint(recipient, amount); - this.logs = logs; + it('rejects a null account', async function () { + await assertRevert(this.token.mint(ZERO_ADDRESS, amount)); }); - it('increments totalSupply', async function () { - const expectedSupply = initialSupply.plus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + describe('for a non null account', function () { + beforeEach('minting', async function () { + const { logs } = await this.token.mint(recipient, amount); + this.logs = logs; + }); - it('increments recipient balance', async function () { - (await this.token.balanceOf(recipient)).should.be.bignumber.equal(amount); - }); + it('increments totalSupply', async function () { + const expectedSupply = initialSupply.plus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: ZERO_ADDRESS, - to: recipient, + it('increments recipient balance', async function () { + (await this.token.balanceOf(recipient)).should.be.bignumber.equal(amount); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: ZERO_ADDRESS, + to: recipient, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); }); @@ -522,28 +528,34 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { const initialSupply = new BigNumber(100); const amount = new BigNumber(50); - beforeEach('burning', async function () { - const { logs } = await this.token.burn(owner, amount); - this.logs = logs; + it('rejects a null account', async function () { + await assertRevert(this.token.burn(ZERO_ADDRESS, amount)); }); - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + describe('for a non null account', function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burn(owner, amount); + this.logs = logs; + }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); }); @@ -558,33 +570,39 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { await this.token.approve(spender, allowance, { from: owner }); }); - beforeEach('burning', async function () { - const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); - this.logs = logs; + it('rejects a null account', async function () { + await assertRevert(this.token.burnFrom(ZERO_ADDRESS, amount)); }); - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + describe('for a non null account', function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); + this.logs = logs; + }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('decrements spender allowance', async function () { - const expectedAllowance = allowance.minus(amount); - (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); - }); + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements spender allowance', async function () { + const expectedAllowance = allowance.minus(amount); + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); }); }); From bc88f3c4c740367332a22ff494dafb33cac6d13a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 14 Aug 2018 18:26:27 -0300 Subject: [PATCH 09/11] add checks for balances and allowance --- contracts/token/ERC20/StandardToken.sol | 5 ++ test/token/ERC20/StandardToken.test.js | 86 +++++++++++++++---------- 2 files changed, 56 insertions(+), 35 deletions(-) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 39098a68b58..0dd86a9bc69 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -178,6 +178,8 @@ contract StandardToken is ERC20 { */ function _burn(address _account, uint256 _amount) internal { require(_account != 0); + require(balances[_account] > _amount); + totalSupply_ = totalSupply_.sub(_amount); balances[_account] = balances[_account].sub(_amount); emit Transfer(_account, address(0), _amount); @@ -191,6 +193,9 @@ contract StandardToken is ERC20 { */ function _burnFrom(address _account, uint256 _amount) internal { require(_account != 0); + require(allowed[_account][msg.sender] > _amount); + require(balances[_account] > _amount); + // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, // this function needs to emit an event with the updated approval. allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount); diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index 6c1dee429ab..9487d107a35 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -533,28 +533,34 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }); describe('for a non null account', function () { - beforeEach('burning', async function () { - const { logs } = await this.token.burn(owner, amount); - this.logs = logs; + it('rejects burning more than balance', async function () { + await assertRevert(this.token.burn(owner, initialSupply.plus(1))); }); - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + describe('for less amount than balance', function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burn(owner, amount); + this.logs = logs; + }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); }); }); @@ -575,33 +581,43 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }); describe('for a non null account', function () { - beforeEach('burning', async function () { - const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); - this.logs = logs; + it('rejects burning more than allowance', async function () { + await assertRevert(this.token.burnFrom(owner, allowance.plus(1))); }); - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + it('rejects burning more than balance', async function () { + await assertRevert(this.token.burnFrom(owner, initialSupply.plus(1))); }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + describe('for less amount than allowance', function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); + this.logs = logs; + }); - it('decrements spender allowance', async function () { - const expectedAllowance = allowance.minus(amount); - (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); }); - event.args.value.should.be.bignumber.equal(amount); + it('decrements spender allowance', async function () { + const expectedAllowance = allowance.minus(amount); + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); + }); + + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); }); }); From 78f4b9e9e73d354e9bf299abbdca8c37cb2fa95f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 15 Aug 2018 16:02:40 -0300 Subject: [PATCH 10/11] add inline docs to BurnableToken._burn --- contracts/token/ERC20/BurnableToken.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/token/ERC20/BurnableToken.sol b/contracts/token/ERC20/BurnableToken.sol index 329f7b66a4e..7b1c7c9be95 100644 --- a/contracts/token/ERC20/BurnableToken.sol +++ b/contracts/token/ERC20/BurnableToken.sol @@ -28,6 +28,10 @@ contract BurnableToken is StandardToken { _burnFrom(_from, _value); } + /** + * @dev Overrides StandardToken._burn in order for burn and burnFrom to emit + * an additional Burn event. + */ function _burn(address _who, uint256 _value) internal { super._burn(_who, _value); emit Burn(_who, _value); From 34a967b2edafc0240846a651b04b0704b591a025 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 15 Aug 2018 16:04:52 -0300 Subject: [PATCH 11/11] remove redundant checks and clarify why --- contracts/token/ERC20/StandardToken.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 0dd86a9bc69..52e125a2771 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -187,14 +187,13 @@ contract StandardToken is ERC20 { /** * @dev Internal function that burns an amount of the token of a given - * account, deducting from the allowance of the transaction sender. + * account, deducting from the sender's allowance for said account. Uses the + * internal _burn function. * @param _account The account whose tokens will be burnt. * @param _amount The amount that will be burnt. */ function _burnFrom(address _account, uint256 _amount) internal { - require(_account != 0); require(allowed[_account][msg.sender] > _amount); - require(balances[_account] > _amount); // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, // this function needs to emit an event with the updated approval.