Remove deprecated functions and contracts (#2125)

* Remove ERC721.burn(owner, tokenId)

* Remove ERC721._checkOnERC721Received from the contract's API

* Fix linter error

* Remove Escrow and PullPayment withdrawWithGas, replace for withdraw

* Add changelog entry

* Add reentrancy notice

CHANGELOG.md

@@ -6,6 +6,9 @@
  * `AccessControl`: new contract for managing permissions in a system, replacement for `Ownable` and `Roles`. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
 
 ### Breaking changes
+ * `ERC721`: `burn(owner, tokenId)` was removed, use `burn(owner)` instead. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
+ * `ERC721`: `_checkOnERC721Received` was removed. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
+ * `PullPayment`, `Escrow`: `withdrawWithGas` was removed. The old `withdraw` function now forwards all gas. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
  * `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
  * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114))
  * `Pausable`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))

contracts/mocks/ERC721Mock.sol

@@ -19,10 +19,6 @@ contract ERC721Mock is ERC721 {
         _mint(to, tokenId);
     }
 
-    function burn(address owner, uint256 tokenId) public {
-        _burn(owner, tokenId);
-    }
-
     function burn(uint256 tokenId) public {
         _burn(tokenId);
     }

contracts/payment/PullPayment.sol

@@ -28,35 +28,21 @@ contract PullPayment {
     }
 
     /**
-     * @dev Withdraw accumulated payments.
+     * @dev Withdraw accumulated payments, forwarding all gas to the recipient.
      *
      * Note that _any_ account can call this function, not just the `payee`.
      * This means that contracts unaware of the `PullPayment` protocol can still
      * receive funds this way, by having a separate account call
      * {withdrawPayments}.
      *
-     * NOTE: This function has been deprecated, use {withdrawPaymentsWithGas}
-     * instead. Calling contracts with fixed gas limits is an anti-pattern and
-     * may break contract interactions in network upgrades (hardforks).
-     * https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.]
-     *
-     * @param payee Whose payments will be withdrawn.
-     */
-    function withdrawPayments(address payable payee) public virtual {
-        _escrow.withdraw(payee);
-    }
-
-    /**
-     * @dev Same as {withdrawPayments}, but forwarding all gas to the recipient.
-     *
      * WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities.
      * Make sure you trust the recipient, or are either following the
      * checks-effects-interactions pattern or using {ReentrancyGuard}.
      *
-     * _Available since v2.4.0._
+     * @param payee Whose payments will be withdrawn.
      */
-    function withdrawPaymentsWithGas(address payable payee) external virtual {
-        _escrow.withdrawWithGas(payee);
+    function withdrawPayments(address payable payee) public virtual {
+        _escrow.withdraw(payee);
     }
 
     /**

contracts/payment/README.adoc

@@ -1,6 +1,8 @@
 = Payment
 
-NOTE: This page is incomplete. We're working to improve it for the next release. Stay tuned!
+Utilities related to sending and receiving payments. Examples are {PullPayment}, which implements the best security practices when sending funds to third parties, and {PaymentSplitter} to receive incoming payments among a number of beneficiaries.
+
+TIP: When transferring funds to and from untrusted third parties, there is always a security risk of reentrancy. If you would like to learn more about this and ways to protect against it, check out our blog post https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul].
 
 == Utilities
 

contracts/payment/escrow/Escrow.sol

@@ -42,36 +42,16 @@ contract Escrow is Ownable {
     }
 
     /**
-     * @dev Withdraw accumulated balance for a payee, forwarding 2300 gas (a
-     * Solidity `transfer`).
-     *
-     * NOTE: This function has been deprecated, use {withdrawWithGas} instead.
-     * Calling contracts with fixed-gas limits is an anti-pattern and may break
-     * contract interactions in network upgrades (hardforks).
-     * https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.]
-     *
-     * @param payee The address whose funds will be withdrawn and transferred to.
-     */
-    function withdraw(address payable payee) public virtual onlyOwner {
-        uint256 payment = _deposits[payee];
-
-        _deposits[payee] = 0;
-
-        payee.transfer(payment);
-
-        emit Withdrawn(payee, payment);
-    }
-
-    /**
-     * @dev Same as {withdraw}, but forwarding all gas to the recipient.
+     * @dev Withdraw accumulated balance for a payee, forwarding all gas to the
+     * recipient.
      *
      * WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities.
      * Make sure you trust the recipient, or are either following the
      * checks-effects-interactions pattern or using {ReentrancyGuard}.
      *
-     * _Available since v2.4.0._
+     * @param payee The address whose funds will be withdrawn and transferred to.
      */
-    function withdrawWithGas(address payable payee) public virtual onlyOwner {
+    function withdraw(address payable payee) public virtual onlyOwner {
         uint256 payment = _deposits[payee];
 
         _deposits[payee] = 0;

contracts/token/ERC721/ERC721.sol

@@ -269,12 +269,10 @@ contract ERC721 is Context, ERC165, IERC721 {
     /**
      * @dev Internal function to burn a specific token.
      * Reverts if the token does not exist.
-     * Deprecated, use {_burn} instead.
-     * @param owner owner of the token to burn
      * @param tokenId uint256 ID of the token being burned
      */
-    function _burn(address owner, uint256 tokenId) internal virtual {
-        require(ownerOf(tokenId) == owner, "ERC721: burn of token that is not own");
+    function _burn(uint256 tokenId) internal virtual {
+        address owner = ownerOf(tokenId);
 
         _beforeTokenTransfer(owner, address(0), tokenId);
 
@@ -287,15 +285,6 @@ contract ERC721 is Context, ERC165, IERC721 {
         emit Transfer(owner, address(0), tokenId);
     }
 
-    /**
-     * @dev Internal function to burn a specific token.
-     * Reverts if the token does not exist.
-     * @param tokenId uint256 ID of the token being burned
-     */
-    function _burn(uint256 tokenId) internal virtual {
-        _burn(ownerOf(tokenId), tokenId);
-    }
-
     /**
      * @dev Internal function to transfer ownership of a given token ID to another address.
      * As opposed to {transferFrom}, this imposes no restrictions on msg.sender.
@@ -324,15 +313,14 @@ contract ERC721 is Context, ERC165, IERC721 {
      * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address.
      * The call is not executed if the target address is not a contract.
      *
-     * This is an internal detail of the `ERC721` contract and its use is deprecated.
      * @param from address representing the previous owner of the given token ID
      * @param to target address that will receive the tokens
      * @param tokenId uint256 ID of the token to be transferred
      * @param _data bytes optional data to send along with the call
      * @return bool whether the call correctly returned the expected magic value
      */
     function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data)
-        internal virtual returns (bool)
+        private returns (bool)
     {
         if (!to.isContract()) {
             return true;

test/payment/PullPayment.test.js

@@ -46,16 +46,4 @@ describe('PullPayment', function () {
     expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
     expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0');
   });
-
-  it('can withdraw payment forwarding all gas', async function () {
-    const balanceTracker = await balance.tracker(payee1);
-
-    await this.contract.callTransfer(payee1, amount, { from: payer });
-    expect(await this.contract.payments(payee1)).to.be.bignumber.equal(amount);
-
-    await this.contract.withdrawPaymentsWithGas(payee1);
-
-    expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
-    expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0');
-  });
 });

test/token/ERC721/ERC721.test.js

@@ -9,7 +9,7 @@ const { shouldBehaveLikeERC721 } = require('./ERC721.behavior');
 const ERC721Mock = contract.fromArtifact('ERC721Mock');
 
 describe('ERC721', function () {
-  const [ owner, other ] = accounts;
+  const [ owner ] = accounts;
 
   beforeEach(async function () {
     this.token = await ERC721Mock.new();
@@ -47,54 +47,10 @@ describe('ERC721', function () {
       });
     });
 
-    describe('_burn(address, uint256)', function () {
+    describe('_burn', function () {
       it('reverts when burning a non-existent token id', async function () {
         await expectRevert(
-          this.token.methods['burn(address,uint256)'](owner, tokenId), 'ERC721: owner query for nonexistent token'
-        );
-      });
-
-      context('with minted token', function () {
-        beforeEach(async function () {
-          await this.token.mint(owner, tokenId);
-        });
-
-        it('reverts when the account is not the owner', async function () {
-          await expectRevert(
-            this.token.methods['burn(address,uint256)'](other, tokenId), 'ERC721: burn of token that is not own'
-          );
-        });
-
-        context('with burnt token', function () {
-          beforeEach(async function () {
-            ({ logs: this.logs } = await this.token.methods['burn(address,uint256)'](owner, tokenId));
-          });
-
-          it('emits a Transfer event', function () {
-            expectEvent.inLogs(this.logs, 'Transfer', { from: owner, to: ZERO_ADDRESS, tokenId });
-          });
-
-          it('deletes the token', async function () {
-            expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('0');
-            await expectRevert(
-              this.token.ownerOf(tokenId), 'ERC721: owner query for nonexistent token'
-            );
-          });
-
-          it('reverts when burning a token id that has been deleted', async function () {
-            await expectRevert(
-              this.token.methods['burn(address,uint256)'](owner, tokenId),
-              'ERC721: owner query for nonexistent token'
-            );
-          });
-        });
-      });
-    });
-
-    describe('_burn(uint256)', function () {
-      it('reverts when burning a non-existent token id', async function () {
-        await expectRevert(
-          this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token'
+          this.token.burn(tokenId), 'ERC721: owner query for nonexistent token'
         );
       });
 
@@ -105,7 +61,7 @@ describe('ERC721', function () {
 
         context('with burnt token', function () {
           beforeEach(async function () {
-            ({ logs: this.logs } = await this.token.methods['burn(uint256)'](tokenId));
+            ({ logs: this.logs } = await this.token.burn(tokenId));
           });
 
           it('emits a Transfer event', function () {
@@ -121,7 +77,7 @@ describe('ERC721', function () {
 
           it('reverts when burning a token id that has been deleted', async function () {
             await expectRevert(
-              this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token'
+              this.token.burn(tokenId), 'ERC721: owner query for nonexistent token'
             );
           });
         });