Merge pull request #2020 from jalextowle/feature/contracts/3.0/rewrite-dispatch-transfer-from

Rewrite _dispatchTransferFrom with a Solidity implementation

contracts/exchange/CHANGELOG.json

@@ -113,6 +113,10 @@
             {
                 "note": "Updated RichErrors to the library pattern",
                 "pr": 1913
+            },
+            {
+                "note": "Rewrote _dispatchTransferFrom in Solidity",
+                "pr": 2020
             }
         ]
     },

contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol

@@ -19,6 +19,7 @@
 pragma solidity ^0.5.9;
 
 import "@0x/contracts-utils/contracts/src/Ownable.sol";
+import "@0x/contracts-utils/contracts/src/LibBytes.sol";
 import "@0x/contracts-utils/contracts/src/LibRichErrors.sol";
 import "./interfaces/IAssetProxy.sol";
 import "./interfaces/IAssetProxyDispatcher.sol";
@@ -30,6 +31,8 @@ contract MixinAssetProxyDispatcher is
     Ownable,
     IAssetProxyDispatcher
 {
+    using LibBytes for bytes;
+
     // Mapping from Asset Proxy Id's to their respective Asset Proxy
     mapping (bytes4 => address) public assetProxies;
 
@@ -92,14 +95,8 @@ contract MixinAssetProxyDispatcher is
                 ));
             }
 
-            // Lookup assetProxy. We do not use `LibBytes.readBytes4` for gas efficiency reasons.
+            // Lookup assetProxy.
-            bytes4 assetProxyId;
+            bytes4 assetProxyId = assetData.readBytes4(0);
-            assembly {
-                assetProxyId := and(mload(
-                    add(assetData, 32)),
-                    0xFFFFFFFF00000000000000000000000000000000000000000000000000000000
-                )
-            }
             address assetProxy = assetProxies[assetProxyId];
 
             // Ensure that assetProxy exists
@@ -111,89 +108,19 @@ contract MixinAssetProxyDispatcher is
                 ));
             }
 
-            // Whether the AssetProxy transfer succeeded.
+            // Construct the calldata for the transferFrom call.
-            bool didSucceed;
+            bytes memory proxyCalldata = abi.encodeWithSelector(
-            // On failure, the revert data thrown by the asset proxy.
+                IAssetProxy(address(0)).transferFrom.selector,
-            bytes memory revertData;
+                assetData,
+                from,
+                to,
+                amount
+            );
 
-            // We construct calldata for the `assetProxy.transferFrom` ABI.
+            // Call the asset proxy's transferFrom function with the constructed calldata.
-            // The layout of this calldata is in the table below.
+            (bool didSucceed, bytes memory revertData) = assetProxy.call(proxyCalldata);
-            //
-            // | 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                          |
-
-            assembly {
-                /////// 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`.
-                // bytes4(keccak256("transferFrom(bytes,address,address,uint256)")) = 0xa85e59e4
-                mstore(cdStart, 0xa85e59e400000000000000000000000000000000000000000000000000000000)
-
-                /////// 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)
-                let assetDataArea := assetData
-                // solhint-disable-next-line no-empty-blocks
-                for {} lt(dataArea, cdEnd) {} {
-                    mstore(dataArea, mload(assetDataArea))
-                    dataArea := add(dataArea, 32)
-                    assetDataArea := add(assetDataArea, 32)
-                }
-
-                /////// Call `assetProxy.transferFrom` using the constructed calldata ///////
-                didSucceed := call(
-                    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
-                    0,                      // don't store the returndata
-                    0                       // don't store the returndata
-                )
-
-                if iszero(didSucceed) { // Call reverted.
-                    let revertDataSize := returndatasize()
-                    // Construct a `bytes memory` type to hold the revert data
-                    // at `cdStart`.
-                    // The first 32 bytes are the length of the data.
-                    mstore(cdStart, revertDataSize)
-                    // Copy the revert data immediately after the length.
-                    returndatacopy(add(cdStart, 32), 0, revertDataSize)
-                    // We need to move the free memory pointer because we
-                    // still have solidity logic that executes after this assembly.
-                    mstore(64, add(cdStart, add(revertDataSize, 32)))
-                    revertData := cdStart
-                }
-            }
 
+            // If the transaction did not succeed, revert with the returned data.
             if (!didSucceed) {
                 LibRichErrors._rrevert(LibExchangeRichErrors.AssetProxyTransferError(
                     orderHash,

contracts/exchange/contracts/src/interfaces/IAssetProxy.sol

@@ -33,7 +33,7 @@ contract IAssetProxy {
         uint256 amount
     )
         external;
-    
+
     /// @dev Gets the proxy id associated with the proxy address.
     /// @return Proxy id.
     function getProxyId()