Merge pull request #659 from 0xProject/feature/contracts/split-proxy-internal-logic

Split transfer implementation and AssetProxyMixin

6 files changed, 206 insertions, 129 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index eeddb9707..7ca823d1f 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -22,69 +22,16 @@ pragma experimental ABIEncoderV2;
 import "../../utils/LibBytes/LibBytes.sol";
 import "./MixinAssetProxy.sol";
 import "./MixinAuthorizable.sol";
-import "../../tokens/ERC20Token/IERC20Token.sol";
+import "./MixinERC20Transfer.sol";
 
 contract ERC20Proxy is
-    LibBytes,
     MixinAssetProxy,
-    MixinAuthorizable
+    MixinAuthorizable,
+    MixinERC20Transfer
 {
-
     // Id of this proxy.
     uint8 constant PROXY_ID = 1;
 
-    /// @dev Internal version of `transferFrom`.
-    /// @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 assetData,
-        address from,
-        address to,
-        uint256 amount
-    )
-        internal
-    {
-        // 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(
-            success,
-            TRANSFER_FAILED
-        );
-        
-        // 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,
-            TRANSFER_FAILED
-        );
-    }
-
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
     function getProxyId()
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 861fac2c1..7ff25aea3 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -22,52 +22,16 @@ pragma experimental ABIEncoderV2;
 import "../../utils/LibBytes/LibBytes.sol";
 import "./MixinAssetProxy.sol";
 import "./MixinAuthorizable.sol";
-import "../../tokens/ERC721Token/ERC721Token.sol";
+import "./MixinERC721Transfer.sol";
 
 contract ERC721Proxy is
-    LibBytes,
     MixinAssetProxy,
-    MixinAuthorizable
+    MixinAuthorizable,
+    MixinERC721Transfer
 {
-
     // Id of this proxy.
     uint8 constant PROXY_ID = 2;
 
-    /// @dev Internal version of `transferFrom`.
-    /// @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 assetData,
-        address from,
-        address to,
-        uint256 amount
-    )
-        internal
-    {
-        // There exists only 1 of each token.
-        require(
-            amount == 1,
-            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);
-        }
-    }
-
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
     function getProxyId()
@@ -77,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
new file mode 100644
index 000000000..4af39a00b
--- /dev/null
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -0,0 +1,81 @@
+/*
+
+  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;
+pragma experimental ABIEncoderV2;
+
+import "../../utils/LibBytes/LibBytes.sol";
+import "../../tokens/ERC20Token/IERC20Token.sol";
+import "./libs/LibTransferErrors.sol";
+
+contract MixinERC20Transfer is
+    LibBytes,
+    LibTransferErrors
+{
+    /// @dev Internal version of `transferFrom`.
+    /// @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 assetData,
+        address from,
+        address to,
+        uint256 amount
+    )
+        internal
+    {
+        // 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(
+            success,
+            TRANSFER_FAILED
+        );
+        
+        // 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,
+            TRANSFER_FAILED
+        );
+    }
+}
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
new file mode 100644
index 000000000..d09aba599
--- /dev/null
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
@@ -0,0 +1,94 @@
+/*
+
+  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;
+pragma experimental ABIEncoderV2;
+
+import "../../utils/LibBytes/LibBytes.sol";
+import "../../tokens/ERC721Token/ERC721Token.sol";
+import "./libs/LibTransferErrors.sol";
+
+contract MixinERC721Transfer is
+    LibBytes,
+    LibTransferErrors
+{
+    /// @dev Internal version of `transferFrom`.
+    /// @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 assetData,
+        address from,
+        address to,
+        uint256 amount
+    )
+        internal
+    {
+        // There exists only 1 of each token.
+        require(
+            amount == 1,
+            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);
+        }
+    }
+
+    /// @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.
+}