StandardToken encapsulation (#1197)

* make StandardToken state variables private

* simplify mocks

* document new internal functions

* fix link to ERC20 document

* revert order of Transfer and Mint events

* Revert "simplify mocks"

This reverts commit 371fe3e567e0ec894caa8d5f14ad590b18c5ae86.

* add tests for new internal functions

* add check for null account

* add checks for balances and allowance

* add inline docs to BurnableToken._burn

* remove redundant checks and clarify why

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);
   }
 
 }

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);
   }
 
 }

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)

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);
   }
 
 }

contracts/mocks/StandardTokenMock.sol

@@ -7,8 +7,19 @@ import "../token/ERC20/StandardToken.sol";
 contract StandardTokenMock is StandardToken {
 
   constructor(address _initialAccount, uint256 _initialBalance) public {
-    balances[_initialAccount] = _initialBalance;
-    totalSupply_ = _initialBalance;
+    _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);
   }
 
 }

contracts/token/ERC20/BurnableToken.sol

@@ -25,21 +25,15 @@ 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);
   }
 
+  /**
+   * @dev Overrides StandardToken._burn in order for burn and burnFrom to emit
+   * an additional Burn event.
+   */
   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);
   }
-}
+}

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);
   }

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);
+    _mint(_to, _amount);
     emit Mint(_to, _amount);
-    emit Transfer(address(0), _to, _amount);
     return true;
   }
 

contracts/token/ERC20/StandardToken.sol

@@ -8,17 +8,17 @@ 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 {
   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,48 @@ 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 {
+    require(_account != 0);
+    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 {
+    require(_account != 0);
+    require(balances[_account] > _amount);
+
+    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 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(allowed[_account][msg.sender] > _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);
+    _burn(_account, _amount);
+  }
 }

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 () {

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);
           });
         });
 

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);
         });
       });
     });
@@ -486,4 +489,136 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) {
       });
     });
   });
+
+  describe('_mint', function () {
+    const initialSupply = new BigNumber(100);
+    const amount = new BigNumber(50);
+
+    it('rejects a null account', async function () {
+      await assertRevert(this.token.mint(ZERO_ADDRESS, amount));
+    });
+
+    describe('for a non null account', function () {
+      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);
+
+    it('rejects a null account', async function () {
+      await assertRevert(this.token.burn(ZERO_ADDRESS, amount));
+    });
+
+    describe('for a non null account', function () {
+      it('rejects burning more than balance', async function () {
+        await assertRevert(this.token.burn(owner, initialSupply.plus(1)));
+      });
+
+      describe('for less amount than balance', function () {
+        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 });
+    });
+
+    it('rejects a null account', async function () {
+      await assertRevert(this.token.burnFrom(ZERO_ADDRESS, amount));
+    });
+
+    describe('for a non null account', function () {
+      it('rejects burning more than allowance', async function () {
+        await assertRevert(this.token.burnFrom(owner, allowance.plus(1)));
+      });
+
+      it('rejects burning more than balance', async function () {
+        await assertRevert(this.token.burnFrom(owner, initialSupply.plus(1)));
+      });
+
+      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 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);
+        });
+      });
+    });
+  });
 });