Fix/add erc721 safe mint safe transfer from #1736 (#1816)

* added _safeTransferFrom function

* added safeMint functions

* added package-lock.json for consistency, don't know why it changes

* added initial suggestions/modifications

* change _safeTransferFrom to internal, reverted package-lock.json to original, and changed ERC721Pausable to override _transferFrom instead of transferFrom

* included tests for safeMint functions

* modified safeMint tests to be on ERC721Mock contract

* added safeMint to ERC721Mintable & respective test to ERC721MintBurn.behavior.js

contracts/mocks/ERC721Mock.sol

@@ -4,9 +4,17 @@ import "../token/ERC721/ERC721.sol";
 
 /**
  * @title ERC721Mock
- * This mock just provides a public mint and burn functions for testing purposes
+ * This mock just provides a public safeMint, mint, and burn functions for testing purposes
  */
 contract ERC721Mock is ERC721 {
+    function safeMint(address to, uint256 tokenId) public {
+        _safeMint(to, tokenId);
+    }
+
+    function safeMint(address to, uint256 tokenId, bytes memory _data) public {
+        _safeMint(to, tokenId, _data);
+    }
+
     function mint(address to, uint256 tokenId) public {
         _mint(to, tokenId);
     }

contracts/token/ERC721/ERC721.sol

@@ -174,7 +174,24 @@ contract ERC721 is ERC165, IERC721 {
      * @param _data bytes data to send along with a safe transfer check
      */
     function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) public {
-        transferFrom(from, to, tokenId);
+        require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: transfer caller is not owner nor approved");
+        _safeTransferFrom(from, to, tokenId, _data);
+    }
+
+    /**
+     * @dev Safely transfers the ownership of a given token ID to another address
+     * If the target address is a contract, it must implement `onERC721Received`,
+     * which is called upon a safe transfer, and return the magic value
+     * `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`; otherwise,
+     * the transfer is reverted.
+     * Requires the msg.sender to be the owner, approved, or operator
+     * @param from current owner of the token
+     * @param to address to receive the ownership of the given token ID
+     * @param tokenId uint256 ID of the token to be transferred
+     * @param _data bytes data to send along with a safe transfer check
+     */
+    function _safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) internal {
+        _transferFrom(from, to, tokenId);
         require(_checkOnERC721Received(from, to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");
     }
 
@@ -201,6 +218,36 @@ contract ERC721 is ERC165, IERC721 {
         return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
     }
 
+    /**
+     * @dev Internal function to safely mint a new token.
+     * Reverts if the given token ID already exists.
+     * If the target address is a contract, it must implement `onERC721Received`,
+     * which is called upon a safe transfer, and return the magic value
+     * `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`; otherwise,
+     * the transfer is reverted.
+     * @param to The address that will own the minted token
+     * @param tokenId uint256 ID of the token to be minted
+     */
+    function _safeMint(address to, uint256 tokenId) internal {
+        _safeMint(to, tokenId, "");
+    }
+
+    /**
+     * @dev Internal function to safely mint a new token.
+     * Reverts if the given token ID already exists.
+     * If the target address is a contract, it must implement `onERC721Received`,
+     * which is called upon a safe transfer, and return the magic value
+     * `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`; otherwise,
+     * the transfer is reverted.
+     * @param to The address that will own the minted token
+     * @param tokenId uint256 ID of the token to be minted
+     * @param _data bytes data to send along with a safe transfer check
+     */
+    function _safeMint(address to, uint256 tokenId, bytes memory _data) internal {
+        _mint(to, tokenId);
+        require(_checkOnERC721Received(address(0), to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");
+    }
+
     /**
      * @dev Internal function to mint a new token.
      * Reverts if the given token ID already exists.

contracts/token/ERC721/ERC721Mintable.sol

@@ -10,7 +10,7 @@ import "../../access/roles/MinterRole.sol";
 contract ERC721Mintable is ERC721, MinterRole {
     /**
      * @dev Function to mint tokens.
-     * @param to The address that will receive the minted tokens.
+     * @param to The address that will receive the minted token.
      * @param tokenId The token id to mint.
      * @return A boolean that indicates if the operation was successful.
      */
@@ -18,4 +18,27 @@ contract ERC721Mintable is ERC721, MinterRole {
         _mint(to, tokenId);
         return true;
     }
+
+    /**
+     * @dev Function to safely mint tokens.
+     * @param to The address that will receive the minted token.
+     * @param tokenId The token id to mint.
+     * @return A boolean that indicates if the operation was successful.
+     */
+    function safeMint(address to, uint256 tokenId) public onlyMinter returns (bool) {
+        _safeMint(to, tokenId);
+        return true;
+    }
+
+    /**
+     * @dev Function to safely mint tokens.
+     * @param to The address that will receive the minted token.
+     * @param tokenId The token id to mint.
+     * @param _data bytes data to send along with a safe transfer check.
+     * @return A boolean that indicates if the operation was successful.
+     */
+    function safeMint(address to, uint256 tokenId, bytes memory _data) public onlyMinter returns (bool) {
+        _safeMint(to, tokenId, _data);
+        return true;
+    }
 }

contracts/token/ERC721/ERC721Pausable.sol

@@ -16,7 +16,7 @@ contract ERC721Pausable is ERC721, Pausable {
         super.setApprovalForAll(to, approved);
     }
 
-    function transferFrom(address from, address to, uint256 tokenId) public whenNotPaused {
-        super.transferFrom(from, to, tokenId);
+    function _transferFrom(address from, address to, uint256 tokenId) internal whenNotPaused {
+        super._transferFrom(from, to, tokenId);
     }
 }

test/token/ERC721/ERC721.behavior.js

@@ -4,6 +4,7 @@ const { ZERO_ADDRESS } = constants;
 const { shouldSupportInterfaces } = require('../../introspection/SupportsInterface.behavior');
 
 const ERC721ReceiverMock = artifacts.require('ERC721ReceiverMock.sol');
+const ERC721Mock = artifacts.require('ERC721Mock.sol');
 
 function shouldBehaveLikeERC721 (
   creator,
@@ -324,6 +325,68 @@ function shouldBehaveLikeERC721 (
       });
     });
 
+    describe('safe mint', function () {
+      const fourthTokenId = new BN(4);
+      const tokenId = fourthTokenId;
+      const data = '0x42';
+
+      beforeEach(async function () {
+        this.ERC721Mock = await ERC721Mock.new();
+      });
+
+      describe('via safeMint', function () { // regular minting is tested in ERC721Mintable.test.js and others
+        it('should call onERC721Received — with data', async function () {
+          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false);
+          const receipt = await this.ERC721Mock.safeMint(this.receiver.address, tokenId, data);
+
+          await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', {
+            from: ZERO_ADDRESS,
+            tokenId: tokenId,
+            data: data,
+          });
+        });
+
+        it('should call onERC721Received — without data', async function () {
+          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false);
+          const receipt = await this.ERC721Mock.safeMint(this.receiver.address, tokenId);
+
+          await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', {
+            from: ZERO_ADDRESS,
+            tokenId: tokenId,
+          });
+        });
+
+        context('to a receiver contract returning unexpected value', function () {
+          it('reverts', async function () {
+            const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
+            await expectRevert(
+              this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
+              'ERC721: transfer to non ERC721Receiver implementer'
+            );
+          });
+        });
+
+        context('to a receiver contract that throws', function () {
+          it('reverts', async function () {
+            const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
+            await expectRevert(
+              this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
+              'ERC721ReceiverMock: reverting'
+            );
+          });
+        });
+
+        context('to a contract that does not implement the required function', function () {
+          it('reverts', async function () {
+            const invalidReceiver = this.token;
+            await expectRevert.unspecified(
+              this.ERC721Mock.safeMint(invalidReceiver.address, tokenId)
+            );
+          });
+        });
+      });
+    });
+
     describe('approve', function () {
       const tokenId = firstTokenId;
 

test/token/ERC721/ERC721MintBurn.behavior.js

@@ -13,6 +13,7 @@ function shouldBehaveLikeMintAndBurnERC721 (
   const thirdTokenId = new BN(3);
   const unknownTokenId = new BN(4);
   const MOCK_URI = 'https://example.com';
+  const data = '0x42';
 
   describe('like a mintable and burnable ERC721', function () {
     beforeEach(async function () {
@@ -72,6 +73,18 @@ function shouldBehaveLikeMintAndBurnERC721 (
       });
     });
 
+    describe('safeMint', function () {
+      it('it can safely mint with data', async function () {
+        await this.token.methods['safeMint(address,uint256,bytes)'](...[newOwner, thirdTokenId, data],
+          { from: minter });
+      });
+
+      it('it can safely mint without data', async function () {
+        await this.token.methods['safeMint(address,uint256)'](...[newOwner, thirdTokenId],
+          { from: minter });
+      });
+    });
+
     describe('burn', function () {
       const tokenId = firstTokenId;
       let logs = null;