Merge pull request #877 from 0xProject/feature/contracts/removeERC721Callback

Removed receiverData and `onReceive` callback from ERC721 proxy.

5 files changed, 31 insertions, 188 deletions

diff --git a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
index eadeaf5ba..cb0ed5422 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
+++ b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
@@ -40,7 +40,7 @@ contract MixinForwarderCore is
     MForwarderCore
 {
     bytes4 constant internal ERC20_DATA_ID = bytes4(keccak256("ERC20Token(address)"));
-    bytes4 constant internal ERC721_DATA_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)"));
+    bytes4 constant internal ERC721_DATA_ID = bytes4(keccak256("ERC721Token(address,uint256)"));
     uint256 constant internal MAX_UINT = 2**256 - 1;
 
     constructor ()
diff --git a/packages/contracts/src/2.0.0/forwarder/MixinTransfer.sol b/packages/contracts/src/2.0.0/forwarder/MixinTransfer.sol
index 6c49330f2..bebfc976b 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinTransfer.sol
+++ b/packages/contracts/src/2.0.0/forwarder/MixinTransfer.sol
@@ -109,12 +109,10 @@ contract MixinTransfer is
         // Decode asset data.
         address token = assetData.readAddress(16);
         uint256 tokenId = assetData.readUint256(36);
-        bytes memory receiverData = assetData.readBytesWithLength(100);
-        IERC721Token(token).safeTransferFrom(
+        IERC721Token(token).transferFrom(
             address(this),
             to,
-            tokenId,
-            receiverData
+            tokenId
         );
     }
 }
diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol
index 1f9958b43..53f5a14e0 100644
--- a/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/2.0.0/protocol/AssetProxy/ERC721Proxy.sol
@@ -26,7 +26,7 @@ contract ERC721Proxy is
     MixinAuthorizable
 {
     // Id of this proxy.
-    bytes4 constant internal PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)"));
+    bytes4 constant internal PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256)"));
 
     // solhint-disable-next-line payable-fallback
     function () 
@@ -83,19 +83,15 @@ contract ERC721Proxy is
                 // WARNING: The ABIv2 specification allows additional padding between
                 //          the Params and Data section. This will result in a larger
                 //          offset to assetData.
-                
+
                 // Asset data itself is encoded as follows:
                 //
                 // | Area     | Offset | Length  | Contents                            |
                 // |----------|--------|---------|-------------------------------------|
                 // | Header   | 0      | 4       | function selector                   |
-                // | Params   |        | 3 * 32  | function parameters:                |
+                // | Params   |        | 2 * 32  | function parameters:                |
                 // |          | 4      | 12 + 20 |   1. token address                  |
                 // |          | 36     |         |   2. tokenId                        |
-                // |          | 68     |         |   3. offset to receiverData (*)     |
-                // | Data     |        |         | receiverData:                       |
-                // |          | 100    | 32      | receiverData Length                 |
-                // |          | 132    | **      | receiverData Contents               |
                 
                 // We construct calldata for the `token.safeTransferFrom` ABI.
                 // The layout of this calldata is in the table below.
@@ -103,14 +99,10 @@ contract ERC721Proxy is
                 // | Area     | Offset | Length  | Contents                            |
                 // |----------|--------|---------|-------------------------------------|
                 // | Header   | 0      | 4       | function selector                   |
-                // | Params   |        | 4 * 32  | function parameters:                |
+                // | Params   |        | 3 * 32  | function parameters:                |
                 // |          | 4      |         |   1. from                           |
                 // |          | 36     |         |   2. to                             |
                 // |          | 68     |         |   3. tokenId                        |
-                // |          | 100    |         |   4. offset to receiverData (*)     |
-                // | Data     |        |         | receiverData:                       |
-                // |          | 132    | 32      | receiverData Length                 |
-                // |          | 164    | **      | receiverData Contents               |
 
                 // There exists only 1 of each token.
                 // require(amount == 1, "INVALID_AMOUNT")
@@ -122,76 +114,32 @@ contract ERC721Proxy is
                     mstore(96, 0)
                     revert(0, 100)
                 }
-                
-                // Require assetData to be at least 132 bytes
-                let offset := calldataload(4)
-                if lt(calldataload(add(offset, 4)), 132) {
-                    // Revert with `Error("LENGTH_GREATER_THAN_131_REQUIRED")`
-                    mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
-                    mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
-                    mstore(64, 0x000000204c454e4754485f475245415445525f5448414e5f3133315f52455155)
-                    mstore(96, 0x4952454400000000000000000000000000000000000000000000000000000000)
-                    revert(0, 100)
-                }
-                
-                /////// Setup State ///////
-                // `cdStart` is the start of the calldata for
-                // `token.safeTransferFrom` (equal to free memory ptr).
-                let cdStart := mload(64)
-                // `dataAreaLength` is the total number of words
-                // needed to store `receiverData`
-                // As-per the ABI spec, this value is padded up to
-                // the nearest multiple of 32,
-                // and includes 32-bytes for length.
-                // It's calculated as folows:
-                //      - Unpadded length in bytes = `mload(receiverData) + 32`
-                //      - Add 31 to convert rounding down to rounding up.
-                //        Combined with the previous and this is `63`.
-                //      - Round down to nearest multiple of 32 by clearing
-                //        bits 0x1F. This is done with `and` and a mask.
 
                 /////// Setup Header Area ///////
                 // This area holds the 4-byte `transferFromSelector`.
                 // Any trailing data in transferFromSelector will be
                 // overwritten in the next `mstore` call.
-                mstore(cdStart, 0xb88d4fde00000000000000000000000000000000000000000000000000000000)
+                mstore(0, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
                 
                 /////// Setup Params Area ///////
-                // Each parameter is padded to 32-bytes.
-                // The entire Params Area is 128 bytes.
-                // Notes:
-                //   1. A 20-byte mask is applied to addresses
-                //      to zero-out the unused bytes.
-                //   2. The offset to `receiverData` is the length
-                //      of the Params Area (128 bytes).
-                
-                let length := calldataload(add(offset, 136))
-                let token := calldataload(add(offset, 40))
-                
-                // Round length up to multiple of 32
-                length := and(add(length, 31), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE0)
-                
-                // Copy `from` and `to`
-                calldatacopy(add(cdStart, 4), 36, 64)
-                
-                // TokenId
-                mstore(add(cdStart, 68), calldataload(add(offset, 72)))
-                
-                // Offset to receiverData
-                mstore(add(cdStart, 100), 128)
-                
-                // receiverData (including length)
-                calldatacopy(add(cdStart, 132), add(offset, 136), add(length, 32))
-                
-                /////// Call `token.safeTransferFrom` using the calldata ///////
+                // We copy the fields `from` and `to` in bulk
+                // from our own calldata to the new calldata.
+                calldatacopy(4, 36, 64)
+
+                // Copy `tokenId` field from our own calldata to the new calldata.
+                let assetDataOffset := calldataload(4)
+                calldatacopy(68, add(assetDataOffset, 72), 32)
+
+                /////// Call `token.transferFrom` using the calldata ///////
+                let token := calldataload(add(assetDataOffset, 40))
                 let success := call(
-                    gas,                    // forward all gas
-                    token,                  // call address of token contract
-                    0,                      // don't send any ETH
-                    cdStart,                // pointer to start of input
-                    add(length, 164),       // length of input
-                    0,                      // write output to null
-                    0                       // output size is 0 bytes
+                    gas,            // forward all gas
+                    token,          // call address of token contract
+                    0,              // don't send any ETH
+                    0,              // pointer to start of input
+                    100,            // length of input
+                    0,              // write output to null
+                    0               // output size is 0 bytes
                 )
                 if success {
                     return(0, 0)
diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts
index 39674a030..62215f08d 100644
--- a/packages/contracts/test/asset_proxy/proxies.ts
+++ b/packages/contracts/test/asset_proxy/proxies.ts
@@ -1,17 +1,12 @@
 import { BlockchainLifecycle } from '@0xproject/dev-utils';
-import { assetDataUtils, generatePseudoRandomSalt } from '@0xproject/order-utils';
+import { assetDataUtils } from '@0xproject/order-utils';
 import { RevertReason } from '@0xproject/types';
 import { BigNumber } from '@0xproject/utils';
 import * as chai from 'chai';
-import { LogWithDecodedArgs } from 'ethereum-types';
-import ethUtil = require('ethereumjs-util');
 import * as _ from 'lodash';
 
 import { DummyERC20TokenContract } from '../../generated_contract_wrappers/dummy_erc20_token';
-import {
-    DummyERC721ReceiverContract,
-    DummyERC721ReceiverTokenReceivedEventArgs,
-} from '../../generated_contract_wrappers/dummy_erc721_receiver';
+import { DummyERC721ReceiverContract } from '../../generated_contract_wrappers/dummy_erc721_receiver';
 import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dummy_erc721_token';
 import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
 import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
@@ -23,7 +18,6 @@ import { constants } from '../utils/constants';
 import { ERC20Wrapper } from '../utils/erc20_wrapper';
 import { ERC721Wrapper } from '../utils/erc721_wrapper';
 import { LogDecoder } from '../utils/log_decoder';
-import { typeEncodingUtils } from '../utils/type_encoding_utils';
 import { provider, txDefaults, web3Wrapper } from '../utils/web3_wrapper';
 
 chaiSetup.configure();
@@ -253,7 +247,7 @@ describe('Asset Transfer Proxies', () => {
                 expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress);
             });
 
-            it('should call onERC721Received when transferring to a smart contract without receiver data', async () => {
+            it('should not call onERC721Received when transferring to a smart contract', async () => {
                 // Construct ERC721 asset data
                 const encodedAssetData = assetDataUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Verify pre-condition
@@ -277,85 +271,12 @@ describe('Asset Transfer Proxies', () => {
                     }),
                 );
                 // Verify that no log was emitted by erc721 receiver
-                expect(tx.logs.length).to.be.equal(1);
-                const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<DummyERC721ReceiverTokenReceivedEventArgs>;
-                expect(tokenReceivedLog.args.from).to.be.equal(makerAddress);
-                expect(tokenReceivedLog.args.tokenId).to.be.bignumber.equal(erc721MakerTokenId);
-                expect(tokenReceivedLog.args.data).to.be.equal(constants.NULL_BYTES);
+                expect(tx.logs.length).to.be.equal(0);
                 // Verify transfer was successful
                 const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
                 expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address);
             });
 
-            it('should call onERC721Received when transferring to a smart contract with receiver data', async () => {
-                // Construct ERC721 asset data
-                const receiverData = ethUtil.bufferToHex(typeEncodingUtils.encodeUint256(generatePseudoRandomSalt()));
-                const encodedAssetData = assetDataUtils.encodeERC721AssetData(
-                    erc721Token.address,
-                    erc721MakerTokenId,
-                    receiverData,
-                );
-                // Verify pre-condition
-                const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
-                expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
-                // Perform a transfer from makerAddress to takerAddress
-                const amount = new BigNumber(1);
-                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
-                    encodedAssetData,
-                    makerAddress,
-                    erc721Receiver.address,
-                    amount,
-                );
-                const logDecoder = new LogDecoder(web3Wrapper, erc721Receiver.address);
-                const tx = await logDecoder.getTxWithDecodedLogsAsync(
-                    await web3Wrapper.sendTransactionAsync({
-                        to: erc721Proxy.address,
-                        data,
-                        from: exchangeAddress,
-                        gas: constants.MAX_TRANSFER_FROM_GAS,
-                    }),
-                );
-                // Validate log emitted by erc721 receiver
-                expect(tx.logs.length).to.be.equal(1);
-                const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<DummyERC721ReceiverTokenReceivedEventArgs>;
-                expect(tokenReceivedLog.args.from).to.be.equal(makerAddress);
-                expect(tokenReceivedLog.args.tokenId).to.be.bignumber.equal(erc721MakerTokenId);
-                expect(tokenReceivedLog.args.data).to.be.equal(receiverData);
-                // Verify transfer was successful
-                const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
-                expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address);
-            });
-
-            it('should throw if there is receiver data but contract does not have onERC721Received', async () => {
-                // Construct ERC721 asset data
-                const receiverData = ethUtil.bufferToHex(typeEncodingUtils.encodeUint256(generatePseudoRandomSalt()));
-                const encodedAssetData = assetDataUtils.encodeERC721AssetData(
-                    erc721Token.address,
-                    erc721MakerTokenId,
-                    receiverData,
-                );
-                // Verify pre-condition
-                const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
-                expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
-                // Perform a transfer from makerAddress to takerAddress
-                const amount = new BigNumber(1);
-                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
-                    encodedAssetData,
-                    makerAddress,
-                    erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver
-                    amount,
-                );
-                return expectTransactionFailedAsync(
-                    web3Wrapper.sendTransactionAsync({
-                        to: erc721Proxy.address,
-                        data,
-                        from: exchangeAddress,
-                        gas: constants.MAX_TRANSFER_FROM_GAS,
-                    }),
-                    RevertReason.TransferFailed,
-                );
-            });
-
             it('should throw if transferring 0 amount of a token', async () => {
                 // Construct ERC721 asset data
                 const encodedAssetData = assetDataUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
@@ -454,9 +375,9 @@ describe('Asset Transfer Proxies', () => {
             });
         });
 
-        it('should have an id of 0x08e937fa', async () => {
+        it('should have an id of 0x02571792', async () => {
             const proxyId = await erc721Proxy.getProxyId.callAsync();
-            const expectedProxyId = '0x08e937fa';
+            const expectedProxyId = '0x02571792';
             expect(proxyId).to.equal(expectedProxyId);
         });
     });
diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index 33246a681..b324988cc 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -456,30 +456,6 @@ describe('Exchange core', () => {
                 RevertReason.RoundingError,
             );
         });
-
-        it('should throw if assetData has a length < 132', async () => {
-            // Construct Exchange parameters
-            const makerAssetId = erc721MakerAssetIds[0];
-            const takerAssetId = erc721TakerAssetIds[0];
-            const makerAssetData = assetDataUtils.encodeERC721AssetData(erc721Token.address, makerAssetId).slice(0, -2);
-            signedOrder = await orderFactory.newSignedOrderAsync({
-                makerAssetAmount: new BigNumber(1),
-                takerAssetAmount: new BigNumber(1),
-                makerAssetData,
-                takerAssetData: assetDataUtils.encodeERC721AssetData(erc721Token.address, takerAssetId),
-            });
-            // Verify pre-conditions
-            const initialOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId);
-            expect(initialOwnerMakerAsset).to.be.bignumber.equal(makerAddress);
-            const initialOwnerTakerAsset = await erc721Token.ownerOf.callAsync(takerAssetId);
-            expect(initialOwnerTakerAsset).to.be.bignumber.equal(takerAddress);
-            // Call Exchange
-            const takerAssetFillAmount = signedOrder.takerAssetAmount;
-            return expectTransactionFailedAsync(
-                exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount }),
-                RevertReason.LengthGreaterThan131Required,
-            );
-        });
     });
 
     describe('getOrderInfo', () => {