Rebase with latest removing PROXY_ID from transfer

5 files changed, 111 insertions, 95 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 2b280add4..7ff25aea3 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -41,34 +41,4 @@ contract ERC721Proxy is
     {
         return PROXY_ID;
     }
-
-    /// @dev Decodes ERC721 Asset data.
-    /// @param assetData Encoded byte array.
-    /// @return proxyId Intended ERC721 proxy id.
-    /// @return token ERC721 token address.
-    /// @return tokenId ERC721 token id.
-    /// @return receiverData Additional data with no specific format, which
-    ///                      is passed to the receiving contract's onERC721Received.
-    function decodeERC721AssetData(bytes memory assetData)
-        internal
-        pure
-        returns (
-            address token,
-            uint256 tokenId,
-            bytes memory receiverData
-        )
-    {
-        // Decode asset data.
-        token = readAddress(assetData, 0);
-        tokenId = readUint256(assetData, 20);
-        if (assetData.length > 52) {
-            receiverData = readBytes(assetData, 52);
-        }
-
-        return (
-            token,
-            tokenId,
-            receiverData
-        );
-    }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index b0c32db59..4af39a00b 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -21,50 +21,60 @@ pragma experimental ABIEncoderV2;
 
 import "../../utils/LibBytes/LibBytes.sol";
 import "../../tokens/ERC20Token/IERC20Token.sol";
+import "./libs/LibTransferErrors.sol";
 
 contract MixinERC20Transfer is
-    LibBytes
+    LibBytes,
+    LibTransferErrors
 {
-    // Id of this proxy.
-    uint8 constant PROXY_ID = 1;
-    // Revert reasons
-    string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 21.";
-    string constant TRANSFER_FAILED = "Transfer failed.";
-    string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id.";
-
     /// @dev Internal version of `transferFrom`.
-    /// @param assetMetadata Encoded byte array.
+    /// @param assetData Encoded byte array.
     /// @param from Address to transfer asset from.
     /// @param to Address to transfer asset to.
     /// @param amount Amount of asset to transfer.
     function transferFromInternal(
-        bytes memory assetMetadata,
+        bytes memory assetData,
         address from,
         address to,
         uint256 amount
     )
         internal
     {
-        // Data must be intended for this proxy.
-        uint256 length = assetMetadata.length;
-
-        require(
-            length == 21,
-            INVALID_METADATA_LENGTH
-        );
+        // Decode asset data.
+        address token = readAddress(assetData, 0);
 
+        // Transfer tokens.
+        // We do a raw call so we can check the success separate
+        // from the return data.
+        bool success = token.call(abi.encodeWithSelector(
+            IERC20Token(token).transferFrom.selector,
+            from,
+            to,
+            amount
+        ));
         require(
-            uint8(assetMetadata[length - 1]) == PROXY_ID,
-            PROXY_ID_MISMATCH
+            success,
+            TRANSFER_FAILED
         );
-
-        // Decode metadata.
-        address token = readAddress(assetMetadata, 0);
-
-        // Transfer tokens.
-        bool success = IERC20Token(token).transferFrom(from, to, amount);
+        
+        // Check return data.
+        // If there is no return data, we assume the token incorrectly
+        // does not return a bool. In this case we expect it to revert
+        // on failure, which was handled above.
+        // If the token does return data, we require that it is a single
+        // value that evaluates to true.
+        assembly {
+            if returndatasize {
+                success := 0
+                if eq(returndatasize, 32) {
+                    // First 64 bytes of memory are reserved scratch space
+                    returndatacopy(0, 0, 32)
+                    success := mload(0)
+                }
+            }
+        }
         require(
-            success == true,
+            success,
             TRANSFER_FAILED
         );
     }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
index bbb807956..d09aba599 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
@@ -21,59 +21,74 @@ pragma experimental ABIEncoderV2;
 
 import "../../utils/LibBytes/LibBytes.sol";
 import "../../tokens/ERC721Token/ERC721Token.sol";
+import "./libs/LibTransferErrors.sol";
 
 contract MixinERC721Transfer is
-    LibBytes
+    LibBytes,
+    LibTransferErrors
 {
-
-    // Id of this proxy.
-    uint8 constant PROXY_ID = 2;
-
-    // Revert reasons
-    string constant INVALID_TRANSFER_AMOUNT = "Transfer amount must equal 1.";
-    string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 53.";
-    string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id.";
-
     /// @dev Internal version of `transferFrom`.
-    /// @param assetMetadata Encoded byte array.
+    /// @param assetData Encoded byte array.
     /// @param from Address to transfer asset from.
     /// @param to Address to transfer asset to.
     /// @param amount Amount of asset to transfer.
     function transferFromInternal(
-        bytes memory assetMetadata,
+        bytes memory assetData,
         address from,
         address to,
         uint256 amount
     )
         internal
     {
-        // Data must be intended for this proxy.
-        uint256 length = assetMetadata.length;
-
-        require(
-            length == 53,
-            INVALID_METADATA_LENGTH
-        );
-
-        require(
-            uint8(assetMetadata[length - 1]) == PROXY_ID,
-            PROXY_ID_MISMATCH
-        );
-
         // There exists only 1 of each token.
         require(
             amount == 1,
-            INVALID_TRANSFER_AMOUNT
+            INVALID_AMOUNT
         );
+    
+        // Decode asset data.
+        (
+            address token,
+            uint256 tokenId,
+            bytes memory receiverData
+        ) = decodeERC721AssetData(assetData);
+
+        // Transfer token. Saves gas by calling safeTransferFrom only
+        // when there is receiverData present. Either succeeds or throws.
+        if (receiverData.length > 0) {
+            ERC721Token(token).safeTransferFrom(from, to, tokenId, receiverData);
+        } else {
+            ERC721Token(token).transferFrom(from, to, tokenId);
+        }
+    }
 
-        // Decode metadata
-        address token = readAddress(assetMetadata, 0);
-        uint256 tokenId = readUint256(assetMetadata, 20);
-
-        // Transfer token.
-        // Either succeeds or throws.
-        // @TODO: Call safeTransferFrom if there is additional
-        //        data stored in `assetMetadata`.
-        ERC721Token(token).transferFrom(from, to, tokenId);
+    /// @dev Decodes ERC721 Asset data.
+    /// @param assetData Encoded byte array.
+    /// @return proxyId Intended ERC721 proxy id.
+    /// @return token ERC721 token address.
+    /// @return tokenId ERC721 token id.
+    /// @return receiverData Additional data with no specific format, which
+    ///                      is passed to the receiving contract's onERC721Received.
+    function decodeERC721AssetData(bytes memory assetData)
+        internal
+        pure
+        returns (
+            address token,
+            uint256 tokenId,
+            bytes memory receiverData
+        )
+    {
+        // Decode asset data.
+        token = readAddress(assetData, 0);
+        tokenId = readUint256(assetData, 20);
+        if (assetData.length > 52) {
+            receiverData = readBytes(assetData, 52);
+        }
+
+        return (
+            token,
+            tokenId,
+            receiverData
+        );
     }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol
index 65bdacdb7..dca4f400f 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol
@@ -25,8 +25,4 @@ contract LibAssetProxyErrors {
     string constant TARGET_ALREADY_AUTHORIZED = "TARGET_ALREADY_AUTHORIZED";      // Target address must not already be authorized.
     string constant INDEX_OUT_OF_BOUNDS = "INDEX_OUT_OF_BOUNDS";                  // Specified array index is out of bounds.
     string constant AUTHORIZED_ADDRESS_MISMATCH = "AUTHORIZED_ADDRESS_MISMATCH";  // Address at index does not match given target address.
-
-    /// AssetProxy errors ///
-    string constant INVALID_AMOUNT = "INVALID_AMOUNT";                            // Transfer amount must equal 1.
-    string constant TRANSFER_FAILED = "TRANSFER_FAILED";                          // Transfer failed.
 }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol
new file mode 100644
index 000000000..ba784ab22
--- /dev/null
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol
@@ -0,0 +1,25 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity ^0.4.24;
+
+contract LibTransferErrors {
+    /// Transfer errors ///
+    string constant INVALID_AMOUNT = "INVALID_AMOUNT";                            // Transfer amount must equal 1.
+    string constant TRANSFER_FAILED = "TRANSFER_FAILED";                          // Transfer failed.
+}