ERC20._approve (#1609)

* Add ERC20._approve.

* Add ERC20._approve tests.

* Fix linter error.

* Require owner in _approve to be non-zero.

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.

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

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;
     }
 
@@ -172,6 +162,20 @@ contract ERC20 is IERC20 {
     }
 
     /**
+     * @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
      * internal burn function.
@@ -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));
     }
 }

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