Add comments to dispatchTransferFrom

1 files changed, 56 insertions, 30 deletions

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(