Unpop byte rather than making deep copy

4 files changed, 95 insertions, 90 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
index 88f916179..a7849f4cb 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
@@ -19,7 +19,6 @@
 pragma solidity ^0.4.24;
 pragma experimental ABIEncoderV2;
 
-import "../../utils/LibBytes/LibBytes.sol";
 import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibFillResults.sol";
@@ -27,7 +26,6 @@ import "./libs/LibExchangeErrors.sol";
 import "./mixins/MExchangeCore.sol";
 
 contract MixinWrapperFunctions is
-    LibBytes,
     LibMath,
     LibFillResults,
     LibExchangeErrors,
@@ -335,15 +333,13 @@ contract MixinWrapperFunctions is
         public
         returns (FillResults memory totalFillResults)
     {
+        bytes memory takerAssetData = orders[0].takerAssetData;
+    
         for (uint256 i = 0; i < orders.length; i++) {
 
             // We assume that asset being sold by taker is the same for each order.
-            // Rather than passing this in as calldata, we copy the takerAssetData from the first order onto all later orders.
-            // We cannot reference the same takerAssetData byte array because the array is modified when a trade is settled.
-            uint256 next = i + 1;
-            if (next != orders.length) {
-                deepCopyBytes(orders[next].takerAssetData, orders[i].takerAssetData);
-            }
+            // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders.
+            orders[i].takerAssetData = takerAssetData;
 
             // Calculate the remaining amount of takerAsset to sell
             uint256 remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount);
@@ -355,6 +351,14 @@ 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);
 
@@ -380,15 +384,13 @@ contract MixinWrapperFunctions is
         public
         returns (FillResults memory totalFillResults)
     {
+        bytes memory takerAssetData = orders[0].takerAssetData;
+
         for (uint256 i = 0; i < orders.length; i++) {
 
             // We assume that asset being sold by taker is the same for each order.
-            // Rather than passing this in as calldata, we copy the takerAssetData from the first order onto all later orders.
-            // We cannot reference the same takerAssetData byte array because the array is modified when a trade is settled.
-            uint256 next = i + 1;
-            if (next != orders.length) {
-                deepCopyBytes(orders[next].takerAssetData, orders[i].takerAssetData);
-            }
+            // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders.
+            orders[i].takerAssetData = takerAssetData;
 
             // Calculate the remaining amount of takerAsset to sell
             uint256 remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount);
@@ -424,15 +426,13 @@ contract MixinWrapperFunctions is
         public
         returns (FillResults memory totalFillResults)
     {
+        bytes memory makerAssetData = orders[0].makerAssetData;
+
         for (uint256 i = 0; i < orders.length; i++) {
 
             // We assume that asset being bought by taker is the same for each order.
             // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders.
-            // We cannot reference the same makerAssetData byte array because the array is modified when a trade is settled.
-            uint256 next = i + 1;
-            if (next != orders.length) {
-                deepCopyBytes(orders[next].makerAssetData, orders[i].makerAssetData);
-            }
+            orders[i].makerAssetData = makerAssetData;
 
             // Calculate the remaining amount of makerAsset to buy
             uint256 remainingMakerAssetFillAmount = safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount);
@@ -452,6 +452,14 @@ 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);
 
@@ -477,15 +485,13 @@ contract MixinWrapperFunctions is
         public
         returns (FillResults memory totalFillResults)
     {
+        bytes memory makerAssetData = orders[0].makerAssetData;
+
         for (uint256 i = 0; i < orders.length; i++) {
 
             // We assume that asset being bought by taker is the same for each order.
             // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders.
-            // We cannot reference the same makerAssetData byte array because the array is modified when a trade is settled.
-            uint256 next = i + 1;
-            if (next != orders.length) {
-                deepCopyBytes(orders[next].makerAssetData, orders[i].makerAssetData);
-            }
+            orders[i].makerAssetData = makerAssetData;
 
             // Calculate the remaining amount of makerAsset to buy
             uint256 remainingMakerAssetFillAmount = safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount);
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index aac0ffc31..339270a57 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -80,64 +80,6 @@ contract LibBytes is
         return result;
     }
 
-    /// @dev Tests equality of two byte arrays.
-    /// @param lhs First byte array to compare.
-    /// @param rhs Second byte array to compare.
-    /// @return True if arrays are the same. False otherwise.
-    function areBytesEqual(
-        bytes memory lhs,
-        bytes memory rhs
-    )
-        internal
-        pure
-        returns (bool equal)
-    {
-        assembly {
-            // Get the number of words occupied by <lhs>
-            let lenFullWords := div(add(mload(lhs), 0x1F), 0x20)
-
-            // Add 1 to the number of words, to account for the length field
-            lenFullWords := add(lenFullWords, 0x1)
-
-            // Test equality word-by-word.
-            // Terminates early if there is a mismatch.
-            for {let i := 0} lt(i, lenFullWords) {i := add(i, 1)} {
-                let lhsWord := mload(add(lhs, mul(i, 0x20)))
-                let rhsWord := mload(add(rhs, mul(i, 0x20)))
-                equal := eq(lhsWord, rhsWord)
-                if eq(equal, 0) {
-                    // Break
-                    i := lenFullWords
-                }
-            }
-       }
-
-       return equal;
-    }
-
-    /// @dev Performs a deep copy of a byte array onto another byte array of greater than or equal length.
-    /// @param dest Byte array that will be overwritten with source bytes.
-    /// @param source Byte array to copy onto dest bytes.
-    function deepCopyBytes(
-        bytes memory dest,
-        bytes memory source
-    )
-        internal
-        pure
-    {
-        uint256 sourceLen = source.length;
-        // 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
-        );
-        memCopy(
-            getMemAddress(dest) + 32,    // +32 to skip length of <dest>
-            getMemAddress(source) + 32,  // +32 to skip length of <source>
-            sourceLen
-        );
-    }
-
     /// @dev Reads an address from a position in a byte array.
     /// @param b Byte array containing an address.
     /// @param index Index in byte array of address.
@@ -370,4 +312,62 @@ contract LibBytes is
             input.length + 32               // +32 bytes to store <input> length
         );
     }
+
+    /// @dev Tests equality of two byte arrays.
+    /// @param lhs First byte array to compare.
+    /// @param rhs Second byte array to compare.
+    /// @return True if arrays are the same. False otherwise.
+    function areBytesEqual(
+        bytes memory lhs,
+        bytes memory rhs
+    )
+        internal
+        pure
+        returns (bool equal)
+    {
+        assembly {
+            // Get the number of words occupied by <lhs>
+            let lenFullWords := div(add(mload(lhs), 0x1F), 0x20)
+
+            // Add 1 to the number of words, to account for the length field
+            lenFullWords := add(lenFullWords, 0x1)
+
+            // Test equality word-by-word.
+            // Terminates early if there is a mismatch.
+            for {let i := 0} lt(i, lenFullWords) {i := add(i, 1)} {
+                let lhsWord := mload(add(lhs, mul(i, 0x20)))
+                let rhsWord := mload(add(rhs, mul(i, 0x20)))
+                equal := eq(lhsWord, rhsWord)
+                if eq(equal, 0) {
+                    // Break
+                    i := lenFullWords
+                }
+            }
+       }
+
+       return equal;
+    }
+
+    /// @dev Performs a deep copy of a byte array onto another byte array of greater than or equal length.
+    /// @param dest Byte array that will be overwritten with source bytes.
+    /// @param source Byte array to copy onto dest bytes.
+    function deepCopyBytes(
+        bytes memory dest,
+        bytes memory source
+    )
+        internal
+        pure
+    {
+        uint256 sourceLen = source.length;
+        // 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
+        );
+        memCopy(
+            getMemAddress(dest) + 32,    // +32 to skip length of <dest>
+            getMemAddress(source) + 32,  // +32 to skip length of <source>
+            sourceLen
+        );
+    }
 }
diff --git a/packages/contracts/src/utils/formatters.ts b/packages/contracts/src/utils/formatters.ts
index b25dec27c..32e4787d6 100644
--- a/packages/contracts/src/utils/formatters.ts
+++ b/packages/contracts/src/utils/formatters.ts
@@ -2,6 +2,7 @@ import { SignedOrder } from '@0xproject/types';
 import { BigNumber } from '@0xproject/utils';
 import * as _ from 'lodash';
 
+import { constants } from './constants';
 import { orderUtils } from './order_utils';
 import { BatchCancelOrders, BatchFillOrders, MarketBuyOrders, MarketSellOrders } from './types';
 
@@ -31,10 +32,7 @@ export const formatters = {
         _.forEach(signedOrders, (signedOrder, i) => {
             const orderWithoutExchangeAddress = orderUtils.getOrderWithoutExchangeAddress(signedOrder);
             if (i !== 0) {
-                orderWithoutExchangeAddress.takerAssetData = `0x${_.repeat(
-                    '0',
-                    signedOrders[0].takerAssetData.length - 2,
-                )}`;
+                orderWithoutExchangeAddress.takerAssetData = constants.NULL_BYTES;
             }
             marketSellOrders.orders.push(orderWithoutExchangeAddress);
             marketSellOrders.signatures.push(signedOrder.signature);
@@ -50,10 +48,7 @@ export const formatters = {
         _.forEach(signedOrders, (signedOrder, i) => {
             const orderWithoutExchangeAddress = orderUtils.getOrderWithoutExchangeAddress(signedOrder);
             if (i !== 0) {
-                orderWithoutExchangeAddress.makerAssetData = `0x${_.repeat(
-                    '0',
-                    signedOrders[0].makerAssetData.length - 2,
-                )}`;
+                orderWithoutExchangeAddress.makerAssetData = constants.NULL_BYTES;
             }
             marketBuyOrders.orders.push(orderWithoutExchangeAddress);
             marketBuyOrders.signatures.push(signedOrder.signature);
diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts
index 5371ae1d1..abba1ac4f 100644
--- a/packages/contracts/test/exchange/wrapper.ts
+++ b/packages/contracts/test/exchange/wrapper.ts
@@ -712,6 +712,10 @@ describe('Exchange wrappers', () => {
                 );
                 await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, {
                     takerAssetFillAmount,
+                    // HACK(albrow): We need to hardcode the gas estimate here because
+                    // the Geth gas estimator doesn't work with the way we use
+                    // delegatecall and swallow errors.
+                    gas: 6000000,
                 });
 
                 const newBalances = await erc20Wrapper.getBalancesAsync();