Exchange wrapper optimizations, minimize unnecessary transfers/approvals

contracts/core/exchange-wrappers/KyberNetworkWrapper.sol

@@ -33,8 +33,8 @@ import { LibBytes } from "../../external/0x/LibBytes.sol";
 contract KyberNetworkWrapper is
     Authorizable
 {
-    using SafeMath for uint256;
     using LibBytes for bytes;
+    using SafeMath for uint256;
 
     /* ============ State Variables ============ */
 
@@ -79,105 +79,107 @@ contract KyberNetworkWrapper is
      * We currently pass change back to the issuance order maker, exploring how it can safely be passed to the taker.
      *
      *
-     * @param  _maker         Address of issuance order signer to conform to IExchangeWrapper
-     * @param  _              Unused address of fillOrder caller to conform to IExchangeWrapper
-     * @param  _tradeCount    Amount of trades in exchange request
-     * @param  _tradesData    Byte string containing (multiple) Kyber trades
-     * @return address[]      Array of token addresses traded for
-     * @return uint256[]      Array of token amounts traded for
+     * @param  _maker               Address of issuance order signer to conform to IExchangeWrapper
+     * -- Unused address of fillOrder caller to conform to IExchangeWrapper --
+     * @param  _makerToken          Address of maker token used in exchange orders
+     * @param  _makerAssetAmount    Amount of issuance order maker token to use on this exchange
+     * @param  _tradeCount          Amount of trades in exchange request
+     * @param  _tradesData          Byte string containing (multiple) Kyber trades
+     * @return address[]            Array of token addresses traded for
+     * @return uint256[]            Array of token amounts traded for
      */
     function exchange(
         address _maker,
-        address _,
+        address,
+        address _makerToken,
+        uint256 _makerAssetAmount,
         uint256 _tradeCount,
         bytes _tradesData
     )
         external
         onlyAuthorized
         returns (address[], uint256[])
     {
-        address[] memory takerTokens = new address[](_tradeCount);
-        uint256[] memory takerAmounts = new uint256[](_tradeCount);
+        // Ensure the issuance order maker token is allowed to be transferred by KyberNetworkProxy as the source token
+        ERC20.ensureAllowance(
+            _makerToken,
+            address(this),
+            kyberNetworkProxy,
+            _makerAssetAmount
+        );
+
+        address[] memory componentTokensReceived = new address[](_tradeCount);
+        uint256[] memory componentTokensAmounts = new uint256[](_tradeCount);
 
-        uint256 scannedBytes = 0;
+        // Parse and execute the trade at the current offset via the KyberNetworkProxy, each kyber trade is 160 bytes
         for (uint256 i = 0; i < _tradeCount; i++) {
-            // Parse Kyber trade of current offset
-            KyberTrade memory trade = parseKyberTrade(
+            (componentTokensReceived[i], componentTokensAmounts[i]) = tradeOnKyberReserve(
                 _tradesData,
-                scannedBytes
+                i.mul(160)
             );
+        }
 
-            // Execute the trade via the KyberNetworkProxy
-            takerTokens[i] = trade.destinationToken;
-            takerAmounts[i] = tradeOnKyberReserve(
-                trade,
-                _maker
+        // Transfer any unused or remainder maker token back to the issuance order user
+        uint remainderSourceToken = ERC20.balanceOf(_makerToken, this);
+        if (remainderSourceToken > 0) {
+            ERC20.transfer(
+                _makerToken,
+                _maker,
+                remainderSourceToken
             );
-
-            // Update current bytes
-            scannedBytes = scannedBytes.add(160);
         }
 
         return (
-            takerTokens,
-            takerAmounts
+            componentTokensReceived,
+            componentTokensAmounts
         );
     }
 
     /* ============ Private ============ */
 
     /**
-     * Executes Kyber trade
+     * Parses and executes Kyber trade
      *
-     * @param  _trade           Kyber trade parameter struct
-     * @param  _maker           Address of issuance order maker to pass change to
-     * @return address          Address of set component to trade for
-     * @return uint256          Amount of set component received in trade
+     * @param  _tradesData     Kyber trade parameter struct
+     * @param  _offset         Start of current Kyber trade to execute
+     * @return address         Address of set component to trade for
+     * @return uint256         Amount of set component received in trade
      */
     function tradeOnKyberReserve(
-        KyberTrade memory _trade,
-        address _maker
+        bytes _tradesData,
+        uint256 _offset
     )
         private
-        returns (uint256)
+        returns (address, uint256)
     {
-        // Ensure the source token is allowed to be transferred by KyberNetworkProxy
-        ERC20.ensureAllowance(
-            _trade.sourceToken,
-            address(this),
-            kyberNetworkProxy,
-            _trade.sourceTokenQuantity
+        // Parse Kyber trade at the current offset
+        KyberTrade memory trade = parseKyberTrade(
+            _tradesData,
+            _offset
         );
 
         uint256 destinationTokenQuantity = KyberNetworkProxyInterface(kyberNetworkProxy).trade(
-            _trade.sourceToken,
-            _trade.sourceTokenQuantity,
-            _trade.destinationToken,
+            trade.sourceToken,
+            trade.sourceTokenQuantity,
+            trade.destinationToken,
             address(this),
-            _trade.maxDestinationQuantity,
-            _trade.minimumConversionRate,
+            trade.maxDestinationQuantity,
+            trade.minimumConversionRate,
             0
         );
 
-        // Transfer any unused issuance order maker token back to user
-        uint remainderSourceToken = ERC20.balanceOf(_trade.sourceToken, this);
-        if (remainderSourceToken > 0) {
-            ERC20.transfer(
-                _trade.sourceToken,
-                _maker,
-                remainderSourceToken
-            );
-        }
-
-        // Ensure the maker token is allowed to be transferred by Set TransferProxy
+        // Ensure the destination token is allowed to be transferred by Set TransferProxy
         ERC20.ensureAllowance(
-            _trade.destinationToken,
+            trade.destinationToken,
             address(this),
             setTransferProxy,
             destinationTokenQuantity
         );
 
-        return destinationTokenQuantity;
+        return (
+            trade.destinationToken,
+            destinationTokenQuantity
+        );
     }
 
     /*

contracts/core/exchange-wrappers/TakerWalletWrapper.sol

@@ -33,6 +33,7 @@ import { LibBytes } from "../../external/0x/LibBytes.sol";
 contract TakerWalletWrapper is
     Authorizable
 {
+    using LibBytes for bytes;
     using SafeMath for uint256;
 
     /* ============ State Variables ============ */
@@ -62,18 +63,22 @@ contract TakerWalletWrapper is
      * IExchange interface delegate method.
      * Parses taker wallet orders and transfers tokens from taker's wallet.
      *
-     * @param  _              Unused address of issuance order signer to conform to IExchangeWrapper
-     * @param  _taker         Taker wallet address
-     * @param  _orderCount    Amount of orders in exchange request
-     * @param  _ordersData    Encoded taker wallet order data
-     * @return address[]      Array of token addresses executed in orders
-     * @return uint256[]      Array of token amounts executed in orders
+     * -- Unused address of issuance order signer to conform to IExchangeWrapper --
+     * @param  _taker            Taker wallet to transfer components from
+     * -- Unused address of maker token used in exchange orders --
+     * -- Unused amount of issuance order maker token to use on this exchange --
+     * @param  _orderCount       Amount of orders in exchange request
+     * @param  _transfersData    Encoded taker wallet order data
+     * @return address[]         Array of token addresses executed in orders
+     * @return uint256[]         Array of token amounts executed in orders
      */
     function exchange(
-        address _,
+        address,
         address _taker,
+        address,
+        uint256,
         uint256 _orderCount,
-        bytes _ordersData
+        bytes _transfersData
     )
         public
         onlyAuthorized
@@ -82,42 +87,70 @@ contract TakerWalletWrapper is
         address[] memory takerTokens = new address[](_orderCount);
         uint256[] memory takerTokenAmounts = new uint256[](_orderCount);
 
-        uint256 scannedBytes = 32;
-        while (scannedBytes < _ordersData.length) {
-
-            // Read the next transfer order
-            address takerToken;
-            uint256 takerTokenAmount;
-            assembly {
-                takerToken := mload(add(_ordersData, scannedBytes))
-                takerTokenAmount := mload(add(_ordersData, add(scannedBytes, 32)))
-            }
-
-            // Transfer from taker's wallet to this wrapper
-            ITransferProxy(transferProxy).transfer(
-                takerToken,
-                takerTokenAmount,
-                _taker,
-                address(this)
-            );
-
-            // Ensure allowance of transfer from this wrapper to TransferProxy
-            ERC20Wrapper.ensureAllowance(
-                takerToken,
-                address(this),
-                transferProxy,
-                takerTokenAmount
-            );
-
+        uint256 scannedBytes = 0;
+        while (scannedBytes < _transfersData.length) {
             // Record taker token and amount to return values
             uint256 orderCount = scannedBytes >> 6;
-            takerTokens[orderCount] = takerToken;
-            takerTokenAmounts[orderCount] = takerTokenAmount;
+
+            // Transfer the tokens from the taker
+            (takerTokens[orderCount], takerTokenAmounts[orderCount]) = transferFromTaker(
+                _taker,
+                scannedBytes,
+                _transfersData
+            );
 
             // Update scanned bytes with length of each transfer request (64)
             scannedBytes = scannedBytes.add(64);
         }
 
         return (takerTokens, takerTokenAmounts);
     }
+
+    /* ============ Private ============ */
+
+    /**
+     * Parses and executes transfer from the issuance order taker's wallet
+     *
+     * @param  _taker            Taker wallet to transfer components from
+     * @param  _offset           Offset to start scanning for current transfer
+     * @param  _transfersData    Byte array of (multiple) taker wallet transfers
+     * @return address           Address of token transferred
+     * @return uint256           Amount of the token transferred
+     */
+    function transferFromTaker(
+        address _taker,
+        uint256 _offset,
+        bytes _transfersData
+    )
+        private
+        returns (address, uint256)
+    {
+        uint256 transferDataStart = _offset.add(32);
+
+        // Read the next transfer
+        address takerToken;
+        uint256 takerTokenAmount;
+        assembly {
+            takerToken := mload(add(_transfersData, transferDataStart))
+            takerTokenAmount := mload(add(_transfersData, add(transferDataStart, 32)))
+        }
+
+        // Transfer from taker's wallet to this wrapper
+        ITransferProxy(transferProxy).transfer(
+            takerToken,
+            takerTokenAmount,
+            _taker,
+            address(this)
+        );
+
+        // Ensure the component token is allowed to be transferred by Set TransferProxy
+        ERC20Wrapper.ensureAllowance(
+            takerToken,
+            address(this),
+            transferProxy,
+            takerTokenAmount
+        );
+
+        return (takerToken, takerTokenAmount);
+    }
 }