Add Greg's documentation to MixinErc20Transfer

1 files changed, 35 insertions, 13 deletions

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),