Fix/improve revert reason #1727

contracts/mocks/ERC721ReceiverNotImplementedMock.sol

@@ -0,0 +1,7 @@
+pragma solidity ^0.5.0;
+
+contract ERC721ReceiverNotImplementedMock{
+
+    event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas);
+
+}

contracts/mocks/ERC721ReceiverRevertsMock.sol

@@ -0,0 +1,23 @@
+pragma solidity ^0.5.0;
+
+import "../token/ERC721/IERC721Receiver.sol";
+
+contract ERC721ReceiverRevertsMock is IERC721Receiver {
+    bytes4 private _retval;
+    bool private _reverts;
+
+    event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas);
+
+    constructor (bytes4 retval, bool reverts) public {
+        _retval = retval;
+        _reverts = reverts;
+    }
+
+    function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data)
+        public returns (bytes4)
+    {
+        require(!_reverts, "ERC721ReceiverRevertsMock: Transaction rejected by receiver");
+        emit Received(operator, from, tokenId, data, gasleft());
+        return _retval;
+    }
+}

contracts/token/ERC721/ERC721.sol

@@ -330,9 +330,28 @@ contract ERC721 is Context, ERC165, IERC721 {
         if (!to.isContract()) {
             return true;
         }
-
-        bytes4 retval = IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data);
-        return (retval == _ERC721_RECEIVED);
+        bytes memory payload = abi.encodeWithSelector(
+            IERC721Receiver(to).onERC721Received.selector,
+            msg.sender,
+            from,
+            tokenId,
+            _data
+        );
+        // solhint-disable-next-line avoid-low-level-calls
+        (bool success, bytes memory returndata) = to.call(payload);
+        if (!success) {
+            if (returndata.length > 0) {
+                // solhint-disable-next-line no-inline-assembly
+                assembly {
+                    let returndata_size := mload(returndata)
+                    revert(returndata, add(0x20, returndata_size))
+                }
+            } else {
+                revert("ERC721: transfer to non ERC721Receiver implementer");
+            }
+        } else {
+            return true;
+        }
     }
 
     /**

test/token/ERC721/ERC721.behavior.js

@@ -6,6 +6,8 @@ const { shouldSupportInterfaces } = require('../../introspection/SupportsInterfa
 
 const ERC721ReceiverMock = contract.fromArtifact('ERC721ReceiverMock');
 const ERC721Mock = contract.fromArtifact('ERC721Mock');
+const ERC721NoReceiverMock = contract.fromArtifact('ERC721ReceiverNotImplementedMock');
+const ERC721ReceiverRevertMock = contract.fromArtifact('ERC721ReceiverRevertsMock');
 
 function shouldBehaveLikeERC721 (
   creator,
@@ -297,25 +299,32 @@ function shouldBehaveLikeERC721 (
 
         describe('to a receiver contract returning unexpected value', function () {
           it('reverts', async function () {
-            const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
+            const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
             await expectRevert(
-              this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }),
+              this.token.safeTransferFrom(owner, noReceiverImplemented.address, tokenId, { from: owner }),
               'ERC721: transfer to non ERC721Receiver implementer'
             );
           });
         });
 
         describe('to a receiver contract that throws', function () {
           it('reverts', async function () {
-            const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
+            const invalidReceiver = await ERC721ReceiverRevertMock.new(RECEIVER_MAGIC_VALUE, true);
             await expectRevert(
               this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }),
-              'ERC721ReceiverMock: reverting'
+              'ERC721ReceiverMock: Transaction rejected by receiver'
             );
           });
         });
 
         describe('to a contract that does not implement the required function', function () {
+          it('reverts', async function () {
+            const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
+            await expectRevert(
+              this.token.safeTransferFrom(owner, noReceiverImplemented.address, tokenId, { from: owner }),
+              'ERC721: transfer to non ERC721Receiver implementer'
+            );
+          });
           it('reverts', async function () {
             const invalidReceiver = this.token;
             await expectRevert.unspecified(
@@ -359,20 +368,20 @@ function shouldBehaveLikeERC721 (
 
         context('to a receiver contract returning unexpected value', function () {
           it('reverts', async function () {
-            const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
+            const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
             await expectRevert(
-              this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
+              this.ERC721Mock.safeMint(noReceiverImplemented.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);
+            const invalidReceiver = await ERC721ReceiverRevertMock.new(RECEIVER_MAGIC_VALUE, true);
             await expectRevert(
               this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
-              'ERC721ReceiverMock: reverting'
+              'ERC721ReceiverMock: Transaction rejected by receiver'
             );
           });
         });