Remove ERC20._burnFrom (#2119)

* Remove ERC20._burnFrom * Add changelog entry
OpenZeppelin · Mar 16, 2020 · baaadde · baaadde
1 parent 68ad1ed
commit baaadde
Show file tree

Hide file tree

Showing 5 changed files with 14 additions and 91 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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);