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