From baaadde3c52180a0efd7b74f3c3a4732ba656572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 16 Mar 2020 15:06:19 -0300 Subject: [PATCH] Remove ERC20._burnFrom (#2119) * Remove ERC20._burnFrom * Add changelog entry --- CHANGELOG.md | 1 + contracts/mocks/ERC20Mock.sol | 4 -- contracts/token/ERC20/ERC20.sol | 11 ---- contracts/token/ERC20/ERC20Burnable.sol | 15 ++++- test/token/ERC20/ERC20.test.js | 74 ------------------------- 5 files changed, 14 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85e0cf6d5ce..f0e30d84841 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Breaking Changes * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114)) + * `ERC20`: removed `_burnFrom`. ([#2119](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2119)) ## 2.5.0 (2020-02-04) diff --git a/contracts/mocks/ERC20Mock.sol b/contracts/mocks/ERC20Mock.sol index ab0390927cb..5e76e78547b 100644 --- a/contracts/mocks/ERC20Mock.sol +++ b/contracts/mocks/ERC20Mock.sol @@ -16,10 +16,6 @@ contract ERC20Mock is ERC20 { _burn(account, amount); } - function burnFrom(address account, uint256 amount) public { - _burnFrom(account, amount); - } - function transferInternal(address from, address to, uint256 value) public { _transfer(from, to, value); } diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 429488f60b1..6b0d1b14c7e 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -223,17 +223,6 @@ contract ERC20 is Context, IERC20 { emit Approval(owner, spender, amount); } - /** - * @dev Destroys `amount` tokens from `account`.`amount` is then deducted - * from the caller's allowance. - * - * See {_burn} and {_approve}. - */ - function _burnFrom(address account, uint256 amount) internal virtual { - _burn(account, amount); - _approve(account, _msgSender(), _allowances[account][_msgSender()].sub(amount, "ERC20: burn amount exceeds allowance")); - } - /** * @dev Hook that is called before any transfer of tokens. This includes * minting and burning. diff --git a/contracts/token/ERC20/ERC20Burnable.sol b/contracts/token/ERC20/ERC20Burnable.sol index 250e7325bff..749fdc93d1a 100644 --- a/contracts/token/ERC20/ERC20Burnable.sol +++ b/contracts/token/ERC20/ERC20Burnable.sol @@ -19,9 +19,20 @@ contract ERC20Burnable is Context, ERC20 { } /** - * @dev See {ERC20-_burnFrom}. + * @dev Destroys `amount` tokens from `account`, deducting from the caller's + * allowance. + * + * See {ERC20-_burn} and {ERC20-allowance}. + * + * Requirements: + * + * - the caller must have allowance for `accounts`'s tokens of at least + * `amount`. */ function burnFrom(address account, uint256 amount) public virtual { - _burnFrom(account, amount); + uint256 decreasedAllowance = allowance(account, _msgSender()).sub(amount, "ERC20: burn amount exceeds allowance"); + + _approve(account, _msgSender(), decreasedAllowance); + _burn(account, amount); } } diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 6d79f6a1b1f..f2ecb8e0ed3 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -262,80 +262,6 @@ describe('ERC20', function () { }); }); - describe('_burnFrom', function () { - const allowance = new BN(70); - - const spender = anotherAccount; - - beforeEach('approving', async function () { - await this.token.approve(spender, allowance, { from: initialHolder }); - }); - - it('rejects a null account', async function () { - await expectRevert(this.token.burnFrom(ZERO_ADDRESS, new BN(1)), - 'ERC20: burn from the zero address' - ); - }); - - describe('for a non zero account', function () { - it('rejects burning more than allowance', async function () { - await expectRevert(this.token.burnFrom(initialHolder, allowance.addn(1)), - 'ERC20: burn amount exceeds allowance' - ); - }); - - it('rejects burning more than balance', async function () { - await expectRevert(this.token.burnFrom(initialHolder, initialSupply.addn(1)), - 'ERC20: burn amount exceeds balance' - ); - }); - - const describeBurnFrom = function (description, amount) { - describe(description, function () { - beforeEach('burning', async function () { - const { logs } = await this.token.burnFrom(initialHolder, amount, { from: spender }); - this.logs = logs; - }); - - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.sub(amount); - expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); - }); - - it('decrements initialHolder balance', async function () { - const expectedBalance = initialSupply.sub(amount); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(expectedBalance); - }); - - it('decrements spender allowance', async function () { - const expectedAllowance = allowance.sub(amount); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(expectedAllowance); - }); - - it('emits a Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: initialHolder, - to: ZERO_ADDRESS, - }); - - expect(event.args.value).to.be.bignumber.equal(amount); - }); - - it('emits an Approval event', async function () { - expectEvent.inLogs(this.logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: await this.token.allowance(initialHolder, spender), - }); - }); - }); - }; - - describeBurnFrom('for entire allowance', allowance); - describeBurnFrom('for less amount than allowance', allowance.subn(1)); - }); - }); - describe('_transfer', function () { shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) { return this.token.transferInternal(from, to, amount);