From f7337c1a05ff49eac6dbade08a73ee32b9c28f36 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 21 Jun 2018 18:46:59 +0200
Subject: Remove proxyId argument from dispatchTransferFrom

---
 .../protocol/Exchange/MixinAssetProxyDispatcher.sol    | 18 ++++++++++++++----
 .../current/protocol/Exchange/MixinExchangeCore.sol    |  9 ---------
 .../current/protocol/Exchange/MixinMatchOrders.sol     | 13 -------------
 .../current/protocol/Exchange/libs/LibConstants.sol    |  3 ---
 .../protocol/Exchange/mixins/MAssetProxyDispatcher.sol |  4 +---
 .../TestAssetProxyDispatcher.sol                       |  3 +--
 6 files changed, 16 insertions(+), 34 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
index b8d6c0722..1d07587e9 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
@@ -19,6 +19,7 @@
 pragma solidity ^0.4.24;
 
 import "../../utils/Ownable/Ownable.sol";
+import "../../utils/LibBytes/LibBytes.sol";
 import "./libs/LibExchangeErrors.sol";
 import "./mixins/MAssetProxyDispatcher.sol";
 import "../AssetProxy/interfaces/IAssetProxy.sol";
@@ -28,6 +29,8 @@ contract MixinAssetProxyDispatcher is
     LibExchangeErrors,
     MAssetProxyDispatcher
 {
+    using LibBytes for bytes;
+    
     // Mapping from Asset Proxy Id's to their respective Asset Proxy
     mapping (uint8 => IAssetProxy) public assetProxies;
 
@@ -83,14 +86,12 @@ contract MixinAssetProxyDispatcher is
     }
 
     /// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws.
-    /// @param assetData Byte array encoded for the respective asset proxy.
-    /// @param assetProxyId Id of assetProxy to dispach to.
+    /// @param assetData Byte array encoded for the asset.
     /// @param from Address to transfer token from.
     /// @param to Address to transfer token to.
     /// @param amount Amount of token to transfer.
     function dispatchTransferFrom(
         bytes memory assetData,
-        uint8 assetProxyId,
         address from,
         address to,
         uint256 amount
@@ -100,6 +101,15 @@ contract MixinAssetProxyDispatcher is
         // Do nothing if no amount should be transferred.
         if (amount > 0) {
             // Lookup assetProxy
+            uint8 assetProxyId = uint8(assetData[assetData.length - 1]);
+            
+            bytes memory assetDataP = new bytes(assetData.length - 1);
+            LibBytes.memCopy(
+                assetDataP.contentAddress(),
+                assetData.contentAddress(),
+                assetDataP.length
+            );
+            
             IAssetProxy assetProxy = assetProxies[assetProxyId];
             // Ensure that assetProxy exists
             require(
@@ -108,7 +118,7 @@ contract MixinAssetProxyDispatcher is
             );
             // transferFrom will either succeed or throw.
             assetProxy.transferFrom(
-                assetData,
+                assetDataP,
                 from,
                 to,
                 amount
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index b207b3e57..38a73d885 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -20,7 +20,6 @@ pragma solidity ^0.4.24;
 pragma experimental ABIEncoderV2;
 
 import "./libs/LibConstants.sol";
-import "../../utils/LibBytes/LibBytes.sol";
 import "./libs/LibFillResults.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibMath.sol";
@@ -41,8 +40,6 @@ contract MixinExchangeCore is
     MSignatureValidator,
     MTransactions
 {
-    using LibBytes for bytes;
-    
     // Mapping of orderHash => amount of takerAsset already bought by maker
     mapping (bytes32 => uint256) public filled;
 
@@ -412,33 +409,27 @@ contract MixinExchangeCore is
     )
         private
     {
-        uint8 makerAssetProxyId = uint8(order.makerAssetData.popLastByte());
-        uint8 takerAssetProxyId = uint8(order.takerAssetData.popLastByte());
         bytes memory zrxAssetData = ZRX_ASSET_DATA;
         dispatchTransferFrom(
             order.makerAssetData,
-            makerAssetProxyId,
             order.makerAddress,
             takerAddress,
             fillResults.makerAssetFilledAmount
         );
         dispatchTransferFrom(
             order.takerAssetData,
-            takerAssetProxyId,
             takerAddress,
             order.makerAddress,
             fillResults.takerAssetFilledAmount
         );
         dispatchTransferFrom(
             zrxAssetData,
-            ZRX_PROXY_ID,
             order.makerAddress,
             order.feeRecipientAddress,
             fillResults.makerFeePaid
         );
         dispatchTransferFrom(
             zrxAssetData,
-            ZRX_PROXY_ID,
             takerAddress,
             order.feeRecipientAddress,
             fillResults.takerFeePaid
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
index e36fcc2c5..a3e8d8dde 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
@@ -15,7 +15,6 @@ pragma solidity ^0.4.24;
 pragma experimental ABIEncoderV2;
 
 import "./libs/LibConstants.sol";
-import "../../utils/LibBytes/LibBytes.sol";
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibFillResults.sol";
@@ -34,8 +33,6 @@ contract MixinMatchOrders is
     MMatchOrders,
     MTransactions
 {
-    using LibBytes for bytes;
-
     /// @dev Match two complementary orders that have a profitable spread.
     ///      Each order is filled at their respective price point. However, the calculations are
     ///      carried out as though the orders are both being filled at the right order's price point.
@@ -242,27 +239,22 @@ contract MixinMatchOrders is
     )
         private
     {
-        uint8 leftMakerAssetProxyId = uint8(leftOrder.makerAssetData.popLastByte());
-        uint8 rightMakerAssetProxyId = uint8(rightOrder.makerAssetData.popLastByte());
         bytes memory zrxAssetData = ZRX_ASSET_DATA;
         // Order makers and taker
         dispatchTransferFrom(
             leftOrder.makerAssetData,
-            leftMakerAssetProxyId,
             leftOrder.makerAddress,
             rightOrder.makerAddress,
             matchedFillResults.right.takerAssetFilledAmount
         );
         dispatchTransferFrom(
             rightOrder.makerAssetData,
-            rightMakerAssetProxyId,
             rightOrder.makerAddress,
             leftOrder.makerAddress,
             matchedFillResults.left.takerAssetFilledAmount
         );
         dispatchTransferFrom(
             leftOrder.makerAssetData,
-            leftMakerAssetProxyId,
             leftOrder.makerAddress,
             takerAddress,
             matchedFillResults.leftMakerAssetSpreadAmount
@@ -271,14 +263,12 @@ contract MixinMatchOrders is
         // Maker fees
         dispatchTransferFrom(
             zrxAssetData,
-            ZRX_PROXY_ID,
             leftOrder.makerAddress,
             leftOrder.feeRecipientAddress,
             matchedFillResults.left.makerFeePaid
         );
         dispatchTransferFrom(
             zrxAssetData,
-            ZRX_PROXY_ID,
             rightOrder.makerAddress,
             rightOrder.feeRecipientAddress,
             matchedFillResults.right.makerFeePaid
@@ -288,7 +278,6 @@ contract MixinMatchOrders is
         if (leftOrder.feeRecipientAddress == rightOrder.feeRecipientAddress) {
             dispatchTransferFrom(
                 zrxAssetData,
-                ZRX_PROXY_ID,
                 takerAddress,
                 leftOrder.feeRecipientAddress,
                 safeAdd(
@@ -299,14 +288,12 @@ contract MixinMatchOrders is
         } else {
             dispatchTransferFrom(
                 zrxAssetData,
-                ZRX_PROXY_ID,
                 takerAddress,
                 leftOrder.feeRecipientAddress,
                 matchedFillResults.left.takerFeePaid
             );
             dispatchTransferFrom(
                 zrxAssetData,
-                ZRX_PROXY_ID,
                 takerAddress,
                 rightOrder.feeRecipientAddress,
                 matchedFillResults.right.takerFeePaid
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibConstants.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibConstants.sol
index 4a9452448..488ca956c 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibConstants.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibConstants.sol
@@ -25,9 +25,6 @@ contract LibConstants {
     //        not constant to make testing easier.
     bytes public ZRX_ASSET_DATA;
 
-    // Proxy Id for ZRX token.
-    uint8 constant ZRX_PROXY_ID = 1;
-
     // @TODO: Remove when we deploy.
     constructor (bytes memory zrxAssetData)
         public
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
index 788f42c60..970082a61 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
@@ -33,14 +33,12 @@ contract MAssetProxyDispatcher is
     );
 
     /// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws.
-    /// @param assetData Byte array encoded for the respective asset proxy.
-    /// @param assetProxyId Id of assetProxy to dispach to.
+    /// @param assetData Byte array encoded for the asset.
     /// @param from Address to transfer token from.
     /// @param to Address to transfer token to.
     /// @param amount Amount of token to transfer.
     function dispatchTransferFrom(
         bytes memory assetData,
-        uint8 assetProxyId,
         address from,
         address to,
         uint256 amount
diff --git a/packages/contracts/src/contracts/current/test/TestAssetProxyDispatcher/TestAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/test/TestAssetProxyDispatcher/TestAssetProxyDispatcher.sol
index d469a07f0..2ae69e0ef 100644
--- a/packages/contracts/src/contracts/current/test/TestAssetProxyDispatcher/TestAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/test/TestAssetProxyDispatcher/TestAssetProxyDispatcher.sol
@@ -24,12 +24,11 @@ import "../../protocol/Exchange/MixinAssetProxyDispatcher.sol";
 contract TestAssetProxyDispatcher is MixinAssetProxyDispatcher {
     function publicDispatchTransferFrom(
         bytes memory assetData,
-        uint8 assetProxyId,
         address from,
         address to,
         uint256 amount)
         public
     {
-        dispatchTransferFrom(assetData, assetProxyId, from, to, amount);
+        dispatchTransferFrom(assetData, from, to, amount);
     }
 }
-- 
cgit 


From a98ecc05af5b264f981087005bb3fa041d5c8756 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Thu, 21 Jun 2018 20:00:48 +0200
Subject: ABI encoded asset data

---
 .../current/protocol/AssetProxy/ERC20Proxy.sol     |  4 +--
 .../current/protocol/AssetProxy/ERC721Proxy.sol    |  4 +--
 .../protocol/AssetProxy/MixinERC20Transfer.sol     |  2 +-
 .../protocol/AssetProxy/MixinERC721Transfer.sol    |  8 ++----
 .../protocol/AssetProxy/interfaces/IAssetData.sol  | 33 ++++++++++++++++++++++
 .../protocol/AssetProxy/interfaces/IAssetProxy.sol |  3 +-
 .../Exchange/MixinAssetProxyDispatcher.sol         | 20 ++++---------
 .../Exchange/interfaces/IAssetProxyDispatcher.sol  |  4 +--
 .../Exchange/mixins/MAssetProxyDispatcher.sol      |  2 +-
 9 files changed, 51 insertions(+), 29 deletions(-)
 create mode 100644 packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index 7ca823d1f..69e2dc913 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -30,14 +30,14 @@ contract ERC20Proxy is
     MixinERC20Transfer
 {
     // Id of this proxy.
-    uint8 constant PROXY_ID = 1;
+    bytes4 constant PROXY_ID = 1;
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
     function getProxyId()
         external
         view
-        returns (uint8)
+        returns (bytes4)
     {
         return PROXY_ID;
     }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 7ff25aea3..7a1b13d80 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -30,14 +30,14 @@ contract ERC721Proxy is
     MixinERC721Transfer
 {
     // Id of this proxy.
-    uint8 constant PROXY_ID = 2;
+    bytes4 constant PROXY_ID = 2;
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
     function getProxyId()
         external
         view
-        returns (uint8)
+        returns (bytes4)
     {
         return PROXY_ID;
     }
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index 0e7f3fc89..deac59f4d 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -42,7 +42,7 @@ contract MixinERC20Transfer is
         internal
     {
         // Decode asset data.
-        address token = assetData.readAddress(0);
+        address token = assetData.readAddress(16);
 
         // Transfer tokens.
         // We do a raw call so we can check the success separate
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
index 944068bbb..ebc879644 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
@@ -79,11 +79,9 @@ contract MixinERC721Transfer is
         )
     {
         // Decode asset data.
-        token = assetData.readAddress(0);
-        tokenId = assetData.readUint256(20);
-        if (assetData.length > 52) {
-            receiverData = assetData.readBytesWithLength(52);
-        }
+        token = assetData.readAddress(16);
+        tokenId = assetData.readUint256(36);
+        receiverData = assetData.readBytesWithLength(100);
 
         return (
             token,
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol
new file mode 100644
index 000000000..6f49a8ba9
--- /dev/null
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol
@@ -0,0 +1,33 @@
+/*
+
+  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.23;
+
+interface IAssetData {
+    
+    function ERC20Token(
+        address tokenContract)
+        external pure;
+    
+    function ERC721Token(
+        address tokenContract,
+        uint256 tokenId,
+        bytes receiverData)
+        external pure;
+    
+}
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol
index 22f43b12f..d12d23610 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol
@@ -56,6 +56,5 @@ contract IAssetProxy is
     function getProxyId()
         external
         view
-        returns (uint8);
+        returns (bytes4);
 }
-
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
index 1d07587e9..da0beefde 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
@@ -32,7 +32,7 @@ contract MixinAssetProxyDispatcher is
     using LibBytes for bytes;
     
     // Mapping from Asset Proxy Id's to their respective Asset Proxy
-    mapping (uint8 => IAssetProxy) public assetProxies;
+    mapping (bytes4 => IAssetProxy) public assetProxies;
 
     /// @dev Registers an asset proxy to an asset proxy id.
     ///      An id can only be assigned to a single proxy at a given time.
@@ -40,7 +40,7 @@ contract MixinAssetProxyDispatcher is
     /// @param newAssetProxy Address of new asset proxy to register, or 0x0 to unset assetProxyId.
     /// @param oldAssetProxy Existing asset proxy to overwrite, or 0x0 if assetProxyId is currently unused.
     function registerAssetProxy(
-        uint8 assetProxyId,
+        bytes4 assetProxyId,
         address newAssetProxy,
         address oldAssetProxy
     )
@@ -58,7 +58,7 @@ contract MixinAssetProxyDispatcher is
 
         // Ensure that the id of newAssetProxy matches the passed in assetProxyId, unless it is being reset to 0.
         if (newAssetProxy != address(0)) {
-            uint8 newAssetProxyId = assetProxy.getProxyId();
+            bytes4 newAssetProxyId = assetProxy.getProxyId();
             require(
                 newAssetProxyId == assetProxyId,
                 ASSET_PROXY_ID_MISMATCH
@@ -77,7 +77,7 @@ contract MixinAssetProxyDispatcher is
     /// @dev Gets an asset proxy.
     /// @param assetProxyId Id of the asset proxy.
     /// @return The asset proxy registered to assetProxyId. Returns 0x0 if no proxy is registered.
-    function getAssetProxy(uint8 assetProxyId)
+    function getAssetProxy(bytes4 assetProxyId)
         external
         view
         returns (address)
@@ -101,15 +101,7 @@ contract MixinAssetProxyDispatcher is
         // Do nothing if no amount should be transferred.
         if (amount > 0) {
             // Lookup assetProxy
-            uint8 assetProxyId = uint8(assetData[assetData.length - 1]);
-            
-            bytes memory assetDataP = new bytes(assetData.length - 1);
-            LibBytes.memCopy(
-                assetDataP.contentAddress(),
-                assetData.contentAddress(),
-                assetDataP.length
-            );
-            
+            bytes4 assetProxyId = assetData.readBytes4(0);
             IAssetProxy assetProxy = assetProxies[assetProxyId];
             // Ensure that assetProxy exists
             require(
@@ -118,7 +110,7 @@ contract MixinAssetProxyDispatcher is
             );
             // transferFrom will either succeed or throw.
             assetProxy.transferFrom(
-                assetDataP,
+                assetData,
                 from,
                 to,
                 amount
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IAssetProxyDispatcher.sol
index 2c331dc34..fa55dff00 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IAssetProxyDispatcher.sol
@@ -26,7 +26,7 @@ contract IAssetProxyDispatcher {
     /// @param newAssetProxy Address of new asset proxy to register, or 0x0 to unset assetProxyId.
     /// @param oldAssetProxy Existing asset proxy to overwrite, or 0x0 if assetProxyId is currently unused.
     function registerAssetProxy(
-        uint8 assetProxyId,
+        bytes4 assetProxyId,
         address newAssetProxy,
         address oldAssetProxy
     )
@@ -35,7 +35,7 @@ contract IAssetProxyDispatcher {
     /// @dev Gets an asset proxy.
     /// @param assetProxyId Id of the asset proxy.
     /// @return The asset proxy registered to assetProxyId. Returns 0x0 if no proxy is registered.
-    function getAssetProxy(uint8 assetProxyId)
+    function getAssetProxy(bytes4 assetProxyId)
         external
         view
         returns (address);
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
index 970082a61..c2b506dcf 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol
@@ -27,7 +27,7 @@ contract MAssetProxyDispatcher is
 
     // Logs registration of new asset proxy
     event AssetProxySet(
-        uint8 id,               // Id of new registered AssetProxy.
+        bytes4 id,              // Id of new registered AssetProxy.
         address newAssetProxy,  // Address of new registered AssetProxy.
         address oldAssetProxy   // Address of AssetProxy that was overwritten at given id (or null address).
     );
-- 
cgit 


From 7b0c13c1219fbe2d47b540476eaebb1f6358213d Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 22 Jun 2018 07:03:42 +0200
Subject: WIP

---
 .../contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol  | 2 +-
 .../contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index 69e2dc913..002e8b662 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -30,7 +30,7 @@ contract ERC20Proxy is
     MixinERC20Transfer
 {
     // Id of this proxy.
-    bytes4 constant PROXY_ID = 1;
+    bytes4 constant PROXY_ID = 0xf47261b0;
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 7a1b13d80..f6bdf2a5f 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -30,7 +30,7 @@ contract ERC721Proxy is
     MixinERC721Transfer
 {
     // Id of this proxy.
-    bytes4 constant PROXY_ID = 2;
+    bytes4 constant PROXY_ID = 0xf47261b0;
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
-- 
cgit 


From 32d499421953851002492b6eef68fd7cc7b43639 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 22 Jun 2018 16:32:19 +0200
Subject: Fix ERC721Proxy TokenId

---
 .../contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index f6bdf2a5f..ce2da76c2 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -30,7 +30,7 @@ contract ERC721Proxy is
     MixinERC721Transfer
 {
     // Id of this proxy.
-    bytes4 constant PROXY_ID = 0xf47261b0;
+    bytes4 constant PROXY_ID = 0x08e937fa;
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
-- 
cgit 


From 7ce0f9682f5af7510d6bf8e5d731fd28ddbc4d3a Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Fri, 22 Jun 2018 16:27:41 -0700
Subject: Fix tests

---
 .../current/protocol/Exchange/MixinWrapperFunctions.sol  | 16 ----------------
 1 file changed, 16 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
index 724f95518..9cb2cacd4 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
@@ -366,14 +366,6 @@ contract MixinWrapperFunctions is
                 signatures[i]
             );
 
-            // HACK: the proxyId is "popped" from the byte array before a fill is settled
-            // by subtracting from the length of the array. Since the popped byte is 
-            // still in memory, we can "unpop" it by incrementing the length of the byte array.
-            assembly {
-                let len := mload(takerAssetData)
-                mstore(takerAssetData, add(len, 1))
-            }
-
             // Update amounts filled and fees paid by maker and taker
             addFillResults(totalFillResults, singleFillResults);
 
@@ -467,14 +459,6 @@ contract MixinWrapperFunctions is
                 signatures[i]
             );
 
-            // HACK: the proxyId is "popped" from the byte array before a fill is settled
-            // by subtracting from the length of the array. Since the popped byte is 
-            // still in memory, we can "unpop" it by incrementing the length of the byte array.
-            assembly {
-                let len := mload(makerAssetData)
-                mstore(makerAssetData, add(len, 1))
-            }
-
             // Update amounts filled and fees paid by maker and taker
             addFillResults(totalFillResults, singleFillResults);
 
-- 
cgit 


From 4df66a4802fc75cd9d3e0c01a92e1cc1ae3dfd58 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Sat, 23 Jun 2018 18:03:49 +0200
Subject: Optimize like in PR #767

---
 .../protocol/AssetProxy/MixinERC20Transfer.sol     | 64 ++++++++++--------
 .../protocol/AssetProxy/MixinERC721Transfer.sol    | 77 ++++++++++++++++++++--
 .../Exchange/MixinAssetProxyDispatcher.sol         | 50 +++++++++++++-
 3 files changed, 157 insertions(+), 34 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index deac59f4d..b5f1f9480 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -43,35 +43,47 @@ contract MixinERC20Transfer is
     {
         // Decode asset data.
         address token = assetData.readAddress(16);
-
-        // 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.
+        bytes4 transferFromSelector = IERC20Token(token).transferFrom.selector;
+        bool success;
         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)
+            /////// Setup State ///////
+            // `cdStart` is the start of the calldata for `token.transferFrom` (equal to free memory ptr).
+            let cdStart := mload(64)
+            // `cdEnd` is the end of the calldata for `token.transferFrom`.
+            let cdEnd := add(cdStart, 100)
+
+            /////// Setup Header Area ///////
+            // This area holds the 4-byte `transferFromSelector`.
+            mstore(cdStart, transferFromSelector)
+
+            /////// Setup Params Area ///////
+            // Each parameter is padded to 32-bytes. The entire Params Area is 96 bytes.
+            // A 20-byte mask is applied to addresses to zero-out the unused bytes.
+            mstore(add(cdStart, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
+            mstore(add(cdStart, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
+            mstore(add(cdStart, 68), amount)
+
+            /////// Call `token.transferFrom` using the constructed calldata ///////
+            success := call(
+                gas,
+                token,
+                0,
+                cdStart,
+                sub(cdEnd, cdStart),
+                cdStart,
+                32
+            )
+            if success {
+                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(
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
index ebc879644..af986adfa 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
@@ -27,7 +27,7 @@ contract MixinERC721Transfer is
     LibTransferErrors
 {
     using LibBytes for bytes;
-    
+     bytes4 constant SAFE_TRANSFER_FROM_SELECTOR = bytes4(keccak256("safeTransferFrom(address,address,uint256,bytes)"));
     /// @dev Internal version of `transferFrom`.
     /// @param assetData Encoded byte array.
     /// @param from Address to transfer asset from.
@@ -54,11 +54,76 @@ contract MixinERC721Transfer is
             bytes memory receiverData
         ) = decodeERC721AssetData(assetData);
 
-        ERC721Token(token).safeTransferFrom(
-            from,
-            to,
-            tokenId,
-            receiverData
+        // We construct calldata for the `token.safeTransferFrom` ABI.
+        // The layout of this calldata is in the table below.
+        // 
+        // | Area     | Offset | Length  | Contents                                    |
+        // | -------- |--------|---------|-------------------------------------------- |
+        // | Header   | 0      | 4       | function selector                           |
+        // | Params   |        | 4 * 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                       |
+
+        bytes4 safeTransferFromSelector = SAFE_TRANSFER_FROM_SELECTOR;
+        bool success;
+        assembly {
+            /////// 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 this value then divide by 32 to get the length in words.
+            //      - Multiply this value by 32 to get the padded length in bytes.
+            let dataAreaLength := mul(div(add(mload(receiverData), 63), 32), 32)
+            // `cdEnd` is the end of the calldata for `token.safeTransferFrom`.
+            let cdEnd := add(cdStart, add(132, dataAreaLength))
+
+            /////// Setup Header Area ///////
+            // This area holds the 4-byte `transferFromSelector`.
+            mstore(cdStart, safeTransferFromSelector)
+            
+            /////// 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).
+            mstore(add(cdStart, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
+            mstore(add(cdStart, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
+            mstore(add(cdStart, 68), tokenId)
+            mstore(add(cdStart, 100), 128)
+
+            /////// Setup Data Area ///////
+            // This area holds `receiverData`.
+            let dataArea := add(cdStart, 132)
+            for {} lt(dataArea, cdEnd) {} {
+                mstore(dataArea, mload(receiverData))
+                dataArea := add(dataArea, 32)
+                receiverData := add(receiverData, 32)
+            }
+
+            /////// Call `token.safeTransferFrom` using the constructed calldata ///////
+            success := call(
+                gas,
+                token,
+                0,
+                cdStart,
+                sub(cdEnd, cdStart),
+                cdStart,
+                0
+            )
+        }
+
+        require(
+            success,
+            TRANSFER_FAILED
         );
     }
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
index da0beefde..96763970c 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
@@ -100,21 +100,67 @@ contract MixinAssetProxyDispatcher is
     {
         // Do nothing if no amount should be transferred.
         if (amount > 0) {
+            require(assetData.length >= 4, "ASSET_DATA_LENGTH");
+            
             // Lookup assetProxy
-            bytes4 assetProxyId = assetData.readBytes4(0);
+            bytes4 assetProxyId;
+            assembly {
+                assetProxyId := and(mload(add(assetData, 32)),
+0xFFFFFFFF00000000000000000000000000000000000000000000000000000000
+                )
+            }
             IAssetProxy assetProxy = assetProxies[assetProxyId];
             // Ensure that assetProxy exists
             require(
                 assetProxy != address(0),
                 ASSET_PROXY_DOES_NOT_EXIST
             );
-            // transferFrom will either succeed or throw.
+            
+            /*
             assetProxy.transferFrom(
                 assetData,
                 from,
                 to,
                 amount
             );
+            return;
+            */
+            
+            bytes4 transferFromSelector = IAssetProxy(assetProxy).transferFrom.selector;
+             bool success;
+            assembly {
+                
+                let cdStart := mload(0x40)
+                let dataAreaLength := mul(div(add(mload(assetData), 63), 32), 32)
+                let cdEnd := add(cdStart, add(132, dataAreaLength))
+                
+                mstore(cdStart, transferFromSelector)
+                mstore(add(cdStart, 4), 128)
+                mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
+                mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
+                mstore(add(cdStart, 100), amount)
+                
+                let dataArea := add(cdStart, 132)
+                for {} lt(dataArea, cdEnd) {} {
+                    mstore(dataArea, mload(assetData))
+                    dataArea := add(dataArea, 32)
+                    assetData := add(assetData, 32)
+                }
+                
+                success := call(
+                     gas,
+                     assetProxy,
+                     0,
+                     cdStart,
+                     sub(cdEnd, cdStart),
+                     cdStart,
+                     0
+                )
+            }
+            require(
+                success,
+                "TRANSFER_FAILED"
+            );
         }
     }
 }
-- 
cgit 


From b8051c8fed0f42778692de135df922626da3fb4f Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Sat, 23 Jun 2018 18:03:49 +0200
Subject: Gas golf the transfer success logic

---
 .../protocol/AssetProxy/MixinERC20Transfer.sol      | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index b5f1f9480..54fe7d726 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -74,17 +74,16 @@ contract MixinERC20Transfer is
                 cdStart,
                 32
             )
-            if success {
-                if returndatasize {
-                    success := 0
-                    if eq(returndatasize, 32) {
-                        // First 64 bytes of memory are reserved scratch space
-                        returndatacopy(0, 0, 32)
-                        success := mload(0)
-                    }
-                }
-                
-            }
+
+            // The transfer succeeded if the call succeeded and either
+            // returned nothing, or returned a non-zero 32 byte value. 
+            success := and(success, or(
+                iszero(returndatasize),
+                and(
+                    eq(returndatasize, 32),
+                    gt(mload(cdStart), 0)
+                )
+            ))
         }
         require(
             success,
-- 
cgit 


From 562fec01d89cae43b813e0f22ffe924953d5fadc Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Sat, 23 Jun 2018 18:03:49 +0200
Subject: Add Greg's documentation to MixinErc20Transfer

---
 .../protocol/AssetProxy/MixinERC20Transfer.sol     | 48 ++++++++++++++++------
 1 file changed, 35 insertions(+), 13 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index 54fe7d726..32d5f394e 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -44,38 +44,60 @@ contract MixinERC20Transfer is
         // Decode asset data.
         address token = assetData.readAddress(16);
         
+        // Transfer tokens.
+        // We do a raw call so we can check the success separate
+        // from the return data.
+        // We construct calldata for the `token.transferFrom` ABI.
+        // The layout of this calldata is in the table below.
+        // 
+        // | Area     | Offset | Length  | Contents                            |
+        // |----------|--------|---------|-------------------------------------|
+        // | Header   | 0      | 4       | function selector                   |
+        // | Params   |        | 3 * 32  | function parameters:                |
+        // |          | 4      |         |   1. from                           |
+        // |          | 36     |         |   2. to                             |
+        // |          | 68     |         |   3. amount                         |
+        
         bytes4 transferFromSelector = IERC20Token(token).transferFrom.selector;
         bool success;
         assembly {
             /////// Setup State ///////
             // `cdStart` is the start of the calldata for `token.transferFrom` (equal to free memory ptr).
             let cdStart := mload(64)
-            // `cdEnd` is the end of the calldata for `token.transferFrom`.
-            let cdEnd := add(cdStart, 100)
 
             /////// 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, transferFromSelector)
 
             /////// Setup Params Area ///////
-            // Each parameter is padded to 32-bytes. The entire Params Area is 96 bytes.
-            // A 20-byte mask is applied to addresses to zero-out the unused bytes.
+            // Each parameter is padded to 32-bytes.
+            // The entire Params Area is 96 bytes.
+            // A 20-byte mask is applied to addresses to
+            // zero-out the unused bytes.
             mstore(add(cdStart, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
             mstore(add(cdStart, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
             mstore(add(cdStart, 68), amount)
 
-            /////// Call `token.transferFrom` using the constructed calldata ///////
+            /////// Call `token.transferFrom` using the calldata ///////
             success := call(
-                gas,
-                token,
-                0,
-                cdStart,
-                sub(cdEnd, cdStart),
-                cdStart,
-                32
+                gas,            // forward all gas
+                token,          // call address of token contract
+                0,              // don't send any ETH
+                cdStart,        // pointer to start of input
+                100,            // length of input
+                cdStart,        // write output over input
+                32              // output size should be 32 bytes
             )
 
-            // The transfer succeeded if the call succeeded and either
+            /////// 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
+            // nonzero 32 bytes value.
+            // So the transfer succeeded if the call succeeded and either
             // returned nothing, or returned a non-zero 32 byte value. 
             success := and(success, or(
                 iszero(returndatasize),
-- 
cgit 


From 9175b43542a8797b0eb3c24e8188ebac67242733 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Sat, 23 Jun 2018 18:03:49 +0200
Subject: Add Greg's documentation to MixinErc721Transfer

---
 .../protocol/AssetProxy/MixinERC721Transfer.sol    | 77 ++++++++++++----------
 1 file changed, 44 insertions(+), 33 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
index af986adfa..c170917ea 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol
@@ -27,7 +27,9 @@ contract MixinERC721Transfer is
     LibTransferErrors
 {
     using LibBytes for bytes;
-     bytes4 constant SAFE_TRANSFER_FROM_SELECTOR = bytes4(keccak256("safeTransferFrom(address,address,uint256,bytes)"));
+    
+    bytes4 constant SAFE_TRANSFER_FROM_SELECTOR = bytes4(keccak256("safeTransferFrom(address,address,uint256,bytes)"));
+    
     /// @dev Internal version of `transferFrom`.
     /// @param assetData Encoded byte array.
     /// @param from Address to transfer asset from.
@@ -57,44 +59,54 @@ contract MixinERC721Transfer is
         // We construct calldata for the `token.safeTransferFrom` ABI.
         // The layout of this calldata is in the table below.
         // 
-        // | Area     | Offset | Length  | Contents                                    |
-        // | -------- |--------|---------|-------------------------------------------- |
-        // | Header   | 0      | 4       | function selector                           |
-        // | Params   |        | 4 * 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                       |
+        // | Area     | Offset | Length  | Contents                            |
+        // |----------|--------|---------|-------------------------------------|
+        // | Header   | 0      | 4       | function selector                   |
+        // | Params   |        | 4 * 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               |
 
         bytes4 safeTransferFromSelector = SAFE_TRANSFER_FROM_SELECTOR;
         bool success;
         assembly {
             /////// Setup State ///////
-            // `cdStart` is the start of the calldata for `token.safeTransferFrom` (equal to free memory ptr).
+            // `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:
+            // `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 this value then divide by 32 to get the length in words.
-            //      - Multiply this value by 32 to get the padded length in bytes.
-            let dataAreaLength := mul(div(add(mload(receiverData), 63), 32), 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.
+            let dataAreaLength := and(add(mload(receiverData), 63), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE0)
             // `cdEnd` is the end of the calldata for `token.safeTransferFrom`.
             let cdEnd := add(cdStart, add(132, dataAreaLength))
 
             /////// 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, safeTransferFromSelector)
             
             /////// Setup Params Area ///////
-            // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes.
+            // 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).
+            //   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).
             mstore(add(cdStart, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
             mstore(add(cdStart, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
             mstore(add(cdStart, 68), tokenId)
@@ -108,19 +120,18 @@ contract MixinERC721Transfer is
                 dataArea := add(dataArea, 32)
                 receiverData := add(receiverData, 32)
             }
-
-            /////// Call `token.safeTransferFrom` using the constructed calldata ///////
+            
+            /////// Call `token.safeTransferFrom` using the calldata ///////
             success := call(
-                gas,
-                token,
-                0,
-                cdStart,
-                sub(cdEnd, cdStart),
-                cdStart,
-                0
+                gas,                    // forward all gas
+                token,                  // call address of token contract
+                0,                      // don't send any ETH
+                cdStart,                // pointer to start of input
+                sub(cdEnd, cdStart),    // length of input
+                cdStart,                // write output over input
+                0                       // output size is 0 bytes
             )
         }
-
         require(
             success,
             TRANSFER_FAILED
-- 
cgit 


From 5127cbb22c8be536996137416f826afd231b0c14 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Sat, 23 Jun 2018 18:03:49 +0200
Subject: Return inplace bytes in LibBytes.readBytesWithLength

---
 .../src/contracts/current/utils/LibBytes/LibBytes.sol    | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index e4cbf318b..289d086d6 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -459,6 +459,8 @@ library LibBytes {
     }
 
     /// @dev Reads nested bytes from a specific position.
+    /// @dev NOTE: the returned value overlaps with the input value.
+    ///            Both should be treated as immutable.
     /// @param b Byte array containing nested bytes.
     /// @param index Index of nested bytes.
     /// @return result Nested bytes.
@@ -480,15 +482,11 @@ library LibBytes {
             b.length >= index + nestedBytesLength,
             GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED
         );
-
-        // Allocate memory and copy value to result
-        result = new bytes(nestedBytesLength);
-        memCopy(
-            result.contentAddress(),
-            b.contentAddress() + index,
-            nestedBytesLength
-        );
-
+        
+        // Return a pointer to the byte array as it exists inside `b`
+        assembly {
+            result := add(b, index)
+        }
         return result;
     }
 
-- 
cgit 


From 1076959006be7ad42f933e476aa75cbca17520c8 Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Mon, 25 Jun 2018 18:08:49 +0200
Subject: Compute bytes4 proxyid constants

---
 .../contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol  | 2 +-
 .../contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index 002e8b662..b74d2c231 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -30,7 +30,7 @@ contract ERC20Proxy is
     MixinERC20Transfer
 {
     // Id of this proxy.
-    bytes4 constant PROXY_ID = 0xf47261b0;
+    bytes4 constant PROXY_ID = bytes4(keccak256("ERC20Token(address)"));
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index ce2da76c2..f6293f97d 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -30,7 +30,7 @@ contract ERC721Proxy is
     MixinERC721Transfer
 {
     // Id of this proxy.
-    bytes4 constant PROXY_ID = 0x08e937fa;
+    bytes4 constant PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)"));
 
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
-- 
cgit 


From db3ce08cab7e8c273121996867f491abed1179af Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Mon, 25 Jun 2018 18:08:59 +0200
Subject: Document IAssetData

---
 .../contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol    | 3 +++
 1 file changed, 3 insertions(+)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol
index 6f49a8ba9..8c78ee8c4 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetData.sol
@@ -18,6 +18,9 @@
 
 pragma solidity ^0.4.23;
 
+// @dev Interface of the asset proxy's assetData.
+// The asset proxies take an ABI encoded `bytes assetData` as argument.
+// This argument is ABI encoded as one of the methods of this interface.
 interface IAssetData {
     
     function ERC20Token(
-- 
cgit 


From 0163984ea4f9d7623613d2d7cd8e2e946acecdf5 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Mon, 25 Jun 2018 14:13:31 -0700
Subject: Add comments to dispatchTransferFrom

---
 .../Exchange/MixinAssetProxyDispatcher.sol         | 86 ++++++++++++++--------
 1 file changed, 56 insertions(+), 30 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
index 96763970c..3203322aa 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol
@@ -20,13 +20,11 @@ pragma solidity ^0.4.24;
 
 import "../../utils/Ownable/Ownable.sol";
 import "../../utils/LibBytes/LibBytes.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./mixins/MAssetProxyDispatcher.sol";
 import "../AssetProxy/interfaces/IAssetProxy.sol";
 
 contract MixinAssetProxyDispatcher is
     Ownable,
-    LibExchangeErrors,
     MAssetProxyDispatcher
 {
     using LibBytes for bytes;
@@ -51,7 +49,7 @@ contract MixinAssetProxyDispatcher is
         address currentAssetProxy = assetProxies[assetProxyId];
         require(
             oldAssetProxy == currentAssetProxy,
-            ASSET_PROXY_MISMATCH
+            "ASSET_PROXY_MISMATCH"
         );
 
         IAssetProxy assetProxy = IAssetProxy(newAssetProxy);
@@ -61,7 +59,7 @@ contract MixinAssetProxyDispatcher is
             bytes4 newAssetProxyId = assetProxy.getProxyId();
             require(
                 newAssetProxyId == assetProxyId,
-                ASSET_PROXY_ID_MISMATCH
+                "ASSET_PROXY_ID_MISMATCH"
             );
         }
 
@@ -100,61 +98,89 @@ contract MixinAssetProxyDispatcher is
     {
         // Do nothing if no amount should be transferred.
         if (amount > 0) {
-            require(assetData.length >= 4, "ASSET_DATA_LENGTH");
+            // Ensure assetData length is valid
+            require(
+                assetData.length > 3,
+                "LENGTH_GREATER_THAN_3_REQUIRED"
+            );
             
             // Lookup assetProxy
             bytes4 assetProxyId;
             assembly {
-                assetProxyId := and(mload(add(assetData, 32)),
-0xFFFFFFFF00000000000000000000000000000000000000000000000000000000
+                assetProxyId := and(mload(
+                    add(assetData, 32)),
+                    0xFFFFFFFF00000000000000000000000000000000000000000000000000000000
                 )
             }
             IAssetProxy assetProxy = assetProxies[assetProxyId];
+
             // Ensure that assetProxy exists
             require(
                 assetProxy != address(0),
-                ASSET_PROXY_DOES_NOT_EXIST
+                "ASSET_PROXY_DOES_NOT_EXIST"
             );
             
-            /*
-            assetProxy.transferFrom(
-                assetData,
-                from,
-                to,
-                amount
-            );
-            return;
-            */
-            
+            // We construct calldata for the `assetProxy.transferFrom` ABI.
+            // The layout of this calldata is in the table below.
+            // 
+            // | Area     | Offset | Length  | Contents                                    |
+            // | -------- |--------|---------|-------------------------------------------- |
+            // | Header   | 0      | 4       | function selector                           |
+            // | Params   |        | 4 * 32  | function parameters:                        |
+            // |          | 4      |         |   1. offset to assetData (*)                |
+            // |          | 36     |         |   2. from                                   |
+            // |          | 68     |         |   3. to                                     |
+            // |          | 100    |         |   4. amount                                 |
+            // | Data     |        |         | assetData:                                  |
+            // |          | 132    | 32      | assetData Length                            |
+            // |          | 164    | **      | assetData Contents                          |
+
             bytes4 transferFromSelector = IAssetProxy(assetProxy).transferFrom.selector;
-             bool success;
+            bool success;
             assembly {
-                
-                let cdStart := mload(0x40)
-                let dataAreaLength := mul(div(add(mload(assetData), 63), 32), 32)
+                /////// Setup State ///////
+                // `cdStart` is the start of the calldata for `assetProxy.transferFrom` (equal to free memory ptr).
+                let cdStart := mload(64)
+                // `dataAreaLength` is the total number of words needed to store `assetData`
+                //  As-per the ABI spec, this value is padded up to the nearest multiple of 32,
+                //  and includes 32-bytes for length.
+                let dataAreaLength := and(add(mload(assetData), 63), 0xFFFFFFFFFFFE0)
+                // `cdEnd` is the end of the calldata for `assetProxy.transferFrom`.
                 let cdEnd := add(cdStart, add(132, dataAreaLength))
+
                 
+                /////// Setup Header Area ///////
+                // This area holds the 4-byte `transferFromSelector`.
                 mstore(cdStart, transferFromSelector)
+                
+                /////// Setup Params Area ///////
+                // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes.
+                // Notes:
+                //   1. The offset to `assetData` is the length of the Params Area (128 bytes).
+                //   2. A 20-byte mask is applied to addresses to zero-out the unused bytes.
                 mstore(add(cdStart, 4), 128)
                 mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff))
                 mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff))
                 mstore(add(cdStart, 100), amount)
                 
+                /////// Setup Data Area ///////
+                // This area holds `assetData`.
                 let dataArea := add(cdStart, 132)
                 for {} lt(dataArea, cdEnd) {} {
                     mstore(dataArea, mload(assetData))
                     dataArea := add(dataArea, 32)
                     assetData := add(assetData, 32)
                 }
-                
+
+                /////// Call `assetProxy.transferFrom` using the constructed calldata ///////
                 success := call(
-                     gas,
-                     assetProxy,
-                     0,
-                     cdStart,
-                     sub(cdEnd, cdStart),
-                     cdStart,
-                     0
+                    gas,                    // forward all gas
+                    assetProxy,             // call address of asset proxy
+                    0,                      // don't send any ETH
+                    cdStart,                // pointer to start of input
+                    sub(cdEnd, cdStart),    // length of input  
+                    cdStart,                // write output over input
+                    0                       // output size is 0 bytes
                 )
             }
             require(
-- 
cgit 


From a89908540f8bca266780c59f3cb1521e7fb16b73 Mon Sep 17 00:00:00 2001
From: Amir Bandeali <abandeali1@gmail.com>
Date: Mon, 25 Jun 2018 16:01:34 -0700
Subject: Don't use variables for revert reasons

---
 .../protocol/AssetProxy/MixinAuthorizable.sol      | 14 +++---
 .../protocol/AssetProxy/MixinERC20Transfer.sol     |  8 ++--
 .../AssetProxy/libs/LibAssetProxyErrors.sol        |  3 ++
 .../protocol/AssetProxy/libs/LibTransferErrors.sol |  3 ++
 .../protocol/Exchange/MixinExchangeCore.sol        | 22 +++++-----
 .../current/protocol/Exchange/MixinMatchOrders.sol |  4 +-
 .../protocol/Exchange/MixinSignatureValidator.sol  | 36 +++++++--------
 .../protocol/Exchange/MixinTransactions.sol        | 10 ++---
 .../protocol/Exchange/MixinWrapperFunctions.sol    |  4 +-
 .../protocol/Exchange/libs/LibExchangeErrors.sol   |  5 +++
 .../current/protocol/Exchange/libs/LibMath.sol     |  1 -
 .../contracts/current/utils/LibBytes/LibBytes.sol  | 51 ++++++++++++----------
 .../contracts/current/utils/Ownable/Ownable.sol    |  5 +--
 13 files changed, 82 insertions(+), 84 deletions(-)

(limited to 'packages/contracts/src')

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
index 37c12f861..3b9584a44 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
@@ -19,12 +19,10 @@
 pragma solidity ^0.4.24;
 pragma experimental ABIEncoderV2;
 
-import "./libs/LibAssetProxyErrors.sol";
 import "../../utils/Ownable/Ownable.sol";
 import "./mixins/MAuthorizable.sol";
 
 contract MixinAuthorizable is
-    LibAssetProxyErrors,
     Ownable,
     MAuthorizable
 {
@@ -33,7 +31,7 @@ contract MixinAuthorizable is
     modifier onlyAuthorized {
         require(
             authorized[msg.sender],
-            SENDER_NOT_AUTHORIZED
+            "SENDER_NOT_AUTHORIZED"
         );
         _;
     }
@@ -49,7 +47,7 @@ contract MixinAuthorizable is
     {
         require(
             !authorized[target],
-            TARGET_ALREADY_AUTHORIZED
+            "TARGET_ALREADY_AUTHORIZED"
         );
 
         authorized[target] = true;
@@ -65,7 +63,7 @@ contract MixinAuthorizable is
     {
         require(
             authorized[target],
-            TARGET_NOT_AUTHORIZED
+            "TARGET_NOT_AUTHORIZED"
         );
 
         delete authorized[target];
@@ -91,15 +89,15 @@ contract MixinAuthorizable is
     {
         require(
             authorized[target],
-            TARGET_NOT_AUTHORIZED
+            "TARGET_NOT_AUTHORIZED"
         );
         require(
             index < authorities.length,
-            INDEX_OUT_OF_BOUNDS
+            "INDEX_OUT_OF_BOUNDS"
         );
         require(
             authorities[index] == target,
-            AUTHORIZED_ADDRESS_MISMATCH
+            "AUTHORIZED_ADDRESS_MISMATCH"
         );
 
         delete authorized[target];
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
index 32d5f394e..a09db43bd 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol
@@ -21,11 +21,9 @@ pragma experimental ABIEncoderV2;
 
 import "../../utils/LibBytes/LibBytes.sol";
 import "../../tokens/ERC20Token/IERC20Token.sol";
-import "./libs/LibTransferErrors.sol";
 
-contract MixinERC20Transfer is
-    LibTransferErrors
-{
+contract MixinERC20Transfer {
+
     using LibBytes for bytes;
 
     /// @dev Internal version of `transferFrom`.
@@ -109,7 +107,7 @@ contract MixinERC20Transfer is
         }
         require(
             success,
-            TRANSFER_FAILED
+            "TRANSFER_FAILED"
         );
     }
 }
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 dca4f400f..b0b20c044 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol
@@ -18,7 +18,10 @@
 
 pragma solidity ^0.4.24;
 
+/// @dev This contract documents the revert reasons used in the AssetProxy contracts.
+/// This contract is intended to serve as a reference, but is not actually used for efficiency reasons.
 contract LibAssetProxyErrors {
+
     /// Authorizable errors ///
     string constant SENDER_NOT_AUTHORIZED = "SENDER_NOT_AUTHORIZED";              // Sender not authorized to call this method.
     string constant TARGET_NOT_AUTHORIZED = "TARGET_NOT_AUTHORIZED";              // Target address not authorized to call this method.
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol
index ba784ab22..88187f196 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol
@@ -18,7 +18,10 @@
 
 pragma solidity ^0.4.24;
 
+/// @dev This contract documents the revert reasons used in the `transferFrom` methods of different AssetProxy contracts.
+/// This contract is intended to serve as a reference, but is not actually used for efficiency reasons.
 contract LibTransferErrors {
+
     /// Transfer 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/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index 38a73d885..c0ed023ac 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -23,7 +23,6 @@ import "./libs/LibConstants.sol";
 import "./libs/LibFillResults.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibMath.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./mixins/MExchangeCore.sol";
 import "./mixins/MSignatureValidator.sol";
 import "./mixins/MTransactions.sol";
@@ -34,7 +33,6 @@ contract MixinExchangeCore is
     LibMath,
     LibOrder,
     LibFillResults,
-    LibExchangeErrors,
     MAssetProxyDispatcher,
     MExchangeCore,
     MSignatureValidator,
@@ -70,7 +68,7 @@ contract MixinExchangeCore is
         // Ensure orderEpoch is monotonically increasing
         require(
             newOrderEpoch > oldOrderEpoch, 
-            INVALID_NEW_ORDER_EPOCH
+            "INVALID_NEW_ORDER_EPOCH"
         );
 
         // Update orderEpoch
@@ -282,20 +280,20 @@ contract MixinExchangeCore is
         // An order can only be filled if its status is FILLABLE.
         require(
             orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
-            ORDER_UNFILLABLE
+            "ORDER_UNFILLABLE"
         );
 
         // Revert if fill amount is invalid
         require(
             takerAssetFillAmount != 0,
-            INVALID_TAKER_AMOUNT
+            "INVALID_TAKER_AMOUNT"
         );
 
         // Validate sender is allowed to fill this order
         if (order.senderAddress != address(0)) {
             require(
                 order.senderAddress == msg.sender,
-                INVALID_SENDER
+                "INVALID_SENDER"
             );
         }
 
@@ -303,7 +301,7 @@ contract MixinExchangeCore is
         if (order.takerAddress != address(0)) {
             require(
                 order.takerAddress == takerAddress,
-                INVALID_TAKER
+                "INVALID_TAKER"
             );
         }
 
@@ -315,7 +313,7 @@ contract MixinExchangeCore is
                     order.makerAddress,
                     signature
                 ),
-                INVALID_ORDER_SIGNATURE
+                "INVALID_ORDER_SIGNATURE"
             );
         }
 
@@ -326,7 +324,7 @@ contract MixinExchangeCore is
                 order.takerAssetAmount,
                 order.makerAssetAmount
             ),
-            ROUNDING_ERROR
+            "ROUNDING_ERROR"
         );
     }
 
@@ -344,14 +342,14 @@ contract MixinExchangeCore is
         // An order can only be cancelled if its status is FILLABLE.
         require(
             orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
-            ORDER_UNFILLABLE
+            "ORDER_UNFILLABLE"
         );
 
         // Validate sender is allowed to cancel this order
         if (order.senderAddress != address(0)) {
             require(
                 order.senderAddress == msg.sender,
-                INVALID_SENDER
+                "INVALID_SENDER"
             );
         }
 
@@ -359,7 +357,7 @@ contract MixinExchangeCore is
         address makerAddress = getCurrentContextAddress();
         require(
             order.makerAddress == makerAddress,
-            INVALID_MAKER
+            "INVALID_MAKER"
         );
     }
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
index a3e8d8dde..1a43eec79 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
@@ -18,7 +18,6 @@ import "./libs/LibConstants.sol";
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibFillResults.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./mixins/MExchangeCore.sol";
 import "./mixins/MMatchOrders.sol";
 import "./mixins/MTransactions.sol";
@@ -27,7 +26,6 @@ import "./mixins/MAssetProxyDispatcher.sol";
 contract MixinMatchOrders is
     LibConstants,
     LibMath,
-    LibExchangeErrors,
     MAssetProxyDispatcher,
     MExchangeCore,
     MMatchOrders,
@@ -141,7 +139,7 @@ contract MixinMatchOrders is
         require(
             safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) >=
             safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount),
-            NEGATIVE_SPREAD_REQUIRED
+            "NEGATIVE_SPREAD_REQUIRED"
         );
     }
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
index cbb55bfce..29172057a 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
@@ -19,23 +19,17 @@
 pragma solidity ^0.4.24;
 
 import "../../utils/LibBytes/LibBytes.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./mixins/MSignatureValidator.sol";
 import "./mixins/MTransactions.sol";
 import "./interfaces/IWallet.sol";
 import "./interfaces/IValidator.sol";
 
 contract MixinSignatureValidator is
-    LibExchangeErrors,
     MSignatureValidator,
     MTransactions
 {
     using LibBytes for bytes;
     
-    // Personal message headers
-    string constant ETH_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n32";
-    string constant TREZOR_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n\x20";
-
     // Mapping of hash => signer => signed
     mapping (bytes32 => mapping (address => bool)) public preSigned;
 
@@ -59,7 +53,7 @@ contract MixinSignatureValidator is
                 signerAddress,
                 signature
             ),
-            INVALID_SIGNATURE
+            "INVALID_SIGNATURE"
         );
         preSigned[hash][signerAddress] = true;
     }
@@ -99,14 +93,14 @@ contract MixinSignatureValidator is
         // TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.)
         require(
             signature.length > 0,
-            LENGTH_GREATER_THAN_0_REQUIRED
+            "LENGTH_GREATER_THAN_0_REQUIRED"
         );
 
         // Ensure signature is supported
         uint8 signatureTypeRaw = uint8(signature.popLastByte());
         require(
             signatureTypeRaw < uint8(SignatureType.NSignatureTypes),
-            SIGNATURE_UNSUPPORTED
+            "SIGNATURE_UNSUPPORTED"
         );
 
         // Pop last byte off of signature byte array.
@@ -124,7 +118,7 @@ contract MixinSignatureValidator is
         // it an explicit option. This aids testing and analysis. It is
         // also the initialization value for the enum type.
         if (signatureType == SignatureType.Illegal) {
-            revert(SIGNATURE_ILLEGAL);
+            revert("SIGNATURE_ILLEGAL");
 
         // Always invalid signature.
         // Like Illegal, this is always implicitly available and therefore
@@ -133,7 +127,7 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.Invalid) {
             require(
                 signature.length == 0,
-                LENGTH_0_REQUIRED
+                "LENGTH_0_REQUIRED"
             );
             isValid = false;
             return isValid;
@@ -142,7 +136,7 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.EIP712) {
             require(
                 signature.length == 65,
-                LENGTH_65_REQUIRED
+                "LENGTH_65_REQUIRED"
             );
             v = uint8(signature[0]);
             r = signature.readBytes32(1);
@@ -155,13 +149,16 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.EthSign) {
             require(
                 signature.length == 65,
-                LENGTH_65_REQUIRED
+                "LENGTH_65_REQUIRED"
             );
             v = uint8(signature[0]);
             r = signature.readBytes32(1);
             s = signature.readBytes32(33);
             recovered = ecrecover(
-                keccak256(abi.encodePacked(ETH_PERSONAL_MESSAGE, hash)),
+                keccak256(abi.encodePacked(
+                    "\x19Ethereum Signed Message:\n32",
+                    hash
+                )),
                 v,
                 r,
                 s
@@ -180,7 +177,7 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.Caller) {
             require(
                 signature.length == 0,
-                LENGTH_0_REQUIRED
+                "LENGTH_0_REQUIRED"
             );
             isValid = signerAddress == msg.sender;
             return isValid;
@@ -230,13 +227,16 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.Trezor) {
             require(
                 signature.length == 65,
-                LENGTH_65_REQUIRED
+                "LENGTH_65_REQUIRED"
             );
             v = uint8(signature[0]);
             r = signature.readBytes32(1);
             s = signature.readBytes32(33);
             recovered = ecrecover(
-                keccak256(abi.encodePacked(TREZOR_PERSONAL_MESSAGE, hash)),
+                keccak256(abi.encodePacked(
+                    "\x19Ethereum Signed Message:\n\x20",
+                    hash
+                )),
                 v,
                 r,
                 s
@@ -250,6 +250,6 @@ contract MixinSignatureValidator is
         // that we currently support. In this case returning false
         // may lead the caller to incorrectly believe that the
         // signature was invalid.)
-        revert(SIGNATURE_UNSUPPORTED);
+        revert("SIGNATURE_UNSUPPORTED");
     }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
index 20a4a12df..e0f450d0a 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
@@ -20,12 +20,10 @@ pragma solidity ^0.4.24;
 import "./libs/LibExchangeErrors.sol";
 import "./mixins/MSignatureValidator.sol";
 import "./mixins/MTransactions.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./libs/LibEIP712.sol";
 
 contract MixinTransactions is
     LibEIP712,
-    LibExchangeErrors,
     MSignatureValidator,
     MTransactions
 {
@@ -90,7 +88,7 @@ contract MixinTransactions is
         // Prevent reentrancy
         require(
             currentContextAddress == address(0),
-            REENTRANCY_ILLEGAL
+            "REENTRANCY_ILLEGAL"
         );
 
         bytes32 transactionHash = hashEIP712Message(hashZeroExTransaction(
@@ -102,7 +100,7 @@ contract MixinTransactions is
         // Validate transaction has not been executed
         require(
             !transactions[transactionHash],
-            INVALID_TX_HASH
+            "INVALID_TX_HASH"
         );
 
         // Transaction always valid if signer is sender of transaction
@@ -114,7 +112,7 @@ contract MixinTransactions is
                     signerAddress,
                     signature
                 ),
-                INVALID_TX_SIGNATURE
+                "INVALID_TX_SIGNATURE"
             );
 
             // Set the current transaction signer
@@ -125,7 +123,7 @@ contract MixinTransactions is
         transactions[transactionHash] = true;
         require(
             address(this).delegatecall(data),
-            FAILED_EXECUTION
+            "FAILED_EXECUTION"
         );
 
         // Reset current transaction signer
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
index 9cb2cacd4..00668ca43 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
@@ -22,13 +22,11 @@ pragma experimental ABIEncoderV2;
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibFillResults.sol";
-import "./libs/LibExchangeErrors.sol";
 import "./mixins/MExchangeCore.sol";
 
 contract MixinWrapperFunctions is
     LibMath,
     LibFillResults,
-    LibExchangeErrors,
     MExchangeCore
 {
     /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
@@ -50,7 +48,7 @@ contract MixinWrapperFunctions is
         );
         require(
             fillResults.takerAssetFilledAmount == takerAssetFillAmount,
-            COMPLETE_FILL_FAILED
+            "COMPLETE_FILL_FAILED"
         );
         return fillResults;
     }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
index a43f0f927..e37f41ada 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
@@ -18,7 +18,10 @@
 
 pragma solidity ^0.4.24;
 
+/// @dev This contract documents the revert reasons used in the Exchange contract.
+/// This contract is intended to serve as a reference, but is not actually used for efficiency reasons.
 contract LibExchangeErrors {
+
     /// Order validation errors ///
     string constant ORDER_UNFILLABLE = "ORDER_UNFILLABLE";                              // Order cannot be filled.
     string constant INVALID_MAKER = "INVALID_MAKER";                                    // Invalid makerAddress.
@@ -56,9 +59,11 @@ contract LibExchangeErrors {
 
     /// dispatchTransferFrom errors ///
     string constant ASSET_PROXY_DOES_NOT_EXIST = "ASSET_PROXY_DOES_NOT_EXIST";          // No assetProxy registered at given id.
+    string constant TRANSFER_FAILED = "TRANSFER_FAILED";                                // Asset transfer unsuccesful.
 
     /// Length validation errors ///
     string constant LENGTH_GREATER_THAN_0_REQUIRED = "LENGTH_GREATER_THAN_0_REQUIRED";  // Byte array must have a length greater than 0.
+    string constant LENGTH_GREATER_THAN_3_REQUIRED = "LENGTH_GREATER_THAN_3_REQUIRED";  // Byte array must have a length greater than 3.
     string constant LENGTH_0_REQUIRED = "LENGTH_0_REQUIRED";                            // Byte array must have a length of 0.
     string constant LENGTH_65_REQUIRED = "LENGTH_65_REQUIRED";                          // Byte array must have a length of 65.
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
index 233547b9f..bfe2fd33f 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
@@ -23,7 +23,6 @@ import "../../../utils/SafeMath/SafeMath.sol";
 contract LibMath is
     SafeMath
 {
-    string constant ROUNDING_ERROR_ON_PARTIAL_AMOUNT = "A rounding error occurred when calculating partial transfer amounts.";
 
     /// @dev Calculates partial value given a numerator and denominator.
     /// @param numerator Numerator.
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index 289d086d6..78b1ddf7c 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -19,17 +19,8 @@
 pragma solidity ^0.4.24;
 
 library LibBytes {
-    using LibBytes for bytes;
 
-    // Revert reasons
-    string constant GREATER_THAN_ZERO_LENGTH_REQUIRED = "GREATER_THAN_ZERO_LENGTH_REQUIRED";
-    string constant GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED = "GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED";
-    string constant GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED = "GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED";
-    string constant GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED = "GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED";
-    string constant GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED = "GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED";
-    string constant GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED = "GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED";
-    string constant FROM_LESS_THAN_TO_REQUIRED = "FROM_LESS_THAN_TO_REQUIRED";
-    string constant TO_LESS_THAN_LENGTH_REQUIRED = "TO_LESS_THAN_LENGTH_REQUIRED";
+    using LibBytes for bytes;
 
     /// @dev Gets the memory address for a byte array.
     /// @param input Byte array to lookup.
@@ -176,8 +167,14 @@ library LibBytes {
         pure
         returns (bytes memory result)
     {
-        require(from <= to, FROM_LESS_THAN_TO_REQUIRED);
-        require(to < b.length, TO_LESS_THAN_LENGTH_REQUIRED);
+        require(
+            from <= to,
+            "FROM_LESS_THAN_TO_REQUIRED"
+        );
+        require(
+            to < b.length,
+            "TO_LESS_THAN_LENGTH_REQUIRED"
+        );
         
         // Create a new bytes structure and copy contents
         result = new bytes(to - from);
@@ -199,8 +196,14 @@ library LibBytes {
         pure
         returns (bytes memory result)
     {
-        require(from <= to, FROM_LESS_THAN_TO_REQUIRED);
-        require(to < b.length, TO_LESS_THAN_LENGTH_REQUIRED);
+        require(
+            from <= to,
+            "FROM_LESS_THAN_TO_REQUIRED"
+        );
+        require(
+            to < b.length,
+            "TO_LESS_THAN_LENGTH_REQUIRED"
+        );
         
         // Create a new bytes structure around [from, to) in-place.
         assembly {
@@ -220,7 +223,7 @@ library LibBytes {
     {
         require(
             b.length > 0,
-            GREATER_THAN_ZERO_LENGTH_REQUIRED
+            "GREATER_THAN_ZERO_LENGTH_REQUIRED"
         );
 
         // Store last byte.
@@ -244,7 +247,7 @@ library LibBytes {
     {
         require(
             b.length >= 20,
-            GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED"
         );
 
         // Store last 20 bytes.
@@ -290,7 +293,7 @@ library LibBytes {
     {
         require(
             b.length >= index + 20,  // 20 is length of address
-            GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED"
         );
 
         // Add offset to index:
@@ -322,7 +325,7 @@ library LibBytes {
     {
         require(
             b.length >= index + 20,  // 20 is length of address
-            GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_20_LENGTH_REQUIRED"
         );
 
         // Add offset to index:
@@ -365,7 +368,7 @@ library LibBytes {
     {
         require(
             b.length >= index + 32,
-            GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED"
         );
 
         // Arrays are prefixed by a 256 bit length parameter
@@ -392,7 +395,7 @@ library LibBytes {
     {
         require(
             b.length >= index + 32,
-            GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_32_LENGTH_REQUIRED"
         );
 
         // Arrays are prefixed by a 256 bit length parameter
@@ -447,7 +450,7 @@ library LibBytes {
     {
         require(
             b.length >= index + 4,
-            GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_4_LENGTH_REQUIRED"
         );
         assembly {
             result := mload(add(b, 32))
@@ -480,7 +483,7 @@ library LibBytes {
         // length of nested bytes
         require(
             b.length >= index + nestedBytesLength,
-            GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED"
         );
         
         // Return a pointer to the byte array as it exists inside `b`
@@ -506,7 +509,7 @@ library LibBytes {
         // length of input
         require(
             b.length >= index + 32 /* 32 bytes to store length */ + input.length,
-            GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_NESTED_BYTES_LENGTH_REQUIRED"
         );
 
         // Copy <input> into <b>
@@ -531,7 +534,7 @@ library LibBytes {
         // Dest length must be >= source length, or some bytes would not be copied.
         require(
             dest.length >= sourceLen,
-            GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED
+            "GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED"
         );
         memCopy(
             dest.contentAddress(),
diff --git a/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol b/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
index 99b93d0d2..6f5761cc7 100644
--- a/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
+++ b/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
@@ -13,9 +13,6 @@ import "./IOwnable.sol";
 contract Ownable is IOwnable {
     address public owner;
 
-    // Revert reasons
-    string constant ONLY_CONTRACT_OWNER = "ONLY_CONTRACT_OWNER";
-
     constructor ()
         public
     {
@@ -25,7 +22,7 @@ contract Ownable is IOwnable {
     modifier onlyOwner() {
         require(
             msg.sender == owner,
-            ONLY_CONTRACT_OWNER
+            "ONLY_CONTRACT_OWNER"
         );
         _;
     }
-- 
cgit