Merge pull request #925 from 0xProject/refactor/contracts/hashing

[contracts] Fix comments, styling and small optimization for hashOrder

3 files changed, 56 insertions, 29 deletions

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
index 88d2da7d7..b5de1a5de 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol
@@ -123,19 +123,23 @@ contract MixinTransactions is
         bytes32 dataHash = keccak256(data);
 
         // Assembly for more efficiently computing:
-        // keccak256(abi.encode(
+        // keccak256(abi.encodePacked(
         //     EIP712_ZEROEX_TRANSACTION_SCHEMA_HASH,
         //     salt,
-        //     signerAddress,
+        //     bytes32(signerAddress),
         //     keccak256(data)
         // ));
 
         assembly {
+            // Load free memory pointer
             let memPtr := mload(64)
-            mstore(memPtr, schemaHash)
-            mstore(add(memPtr, 32), salt)
-            mstore(add(memPtr, 64), and(signerAddress, 0xffffffffffffffffffffffffffffffffffffffff))
-            mstore(add(memPtr, 96), dataHash)
+
+            mstore(memPtr, schemaHash)                                                               // hash of schema
+            mstore(add(memPtr, 32), salt)                                                            // salt
+            mstore(add(memPtr, 64), and(signerAddress, 0xffffffffffffffffffffffffffffffffffffffff))  // signerAddress
+            mstore(add(memPtr, 96), dataHash)                                                        // hash of data
+
+            // Compute hash
             result := keccak256(memPtr, 128)
         }
 
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibEIP712.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibEIP712.sol
index 1fc41dafd..b02f7632e 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibEIP712.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibEIP712.sol
@@ -30,7 +30,7 @@ contract LibEIP712 {
     string constant internal EIP712_DOMAIN_VERSION = "2";
 
     // Hash of the EIP712 Domain Separator Schema
-    bytes32 public constant EIP712_DOMAIN_SEPARATOR_SCHEMA_HASH = keccak256(abi.encodePacked(
+    bytes32 constant internal EIP712_DOMAIN_SEPARATOR_SCHEMA_HASH = keccak256(abi.encodePacked(
         "EIP712Domain(",
         "string name,",
         "string version,",
@@ -45,11 +45,11 @@ contract LibEIP712 {
     constructor ()
         public
     {
-        EIP712_DOMAIN_HASH = keccak256(abi.encode(
+        EIP712_DOMAIN_HASH = keccak256(abi.encodePacked(
             EIP712_DOMAIN_SEPARATOR_SCHEMA_HASH,
             keccak256(bytes(EIP712_DOMAIN_NAME)),
             keccak256(bytes(EIP712_DOMAIN_VERSION)),
-            address(this)
+            bytes32(address(this))
         ));
     }
 
@@ -59,8 +59,28 @@ contract LibEIP712 {
     function hashEIP712Message(bytes32 hashStruct)
         internal
         view
-        returns (bytes32)
+        returns (bytes32 result)
     {
-        return keccak256(abi.encodePacked(EIP191_HEADER, EIP712_DOMAIN_HASH, hashStruct));
+        bytes32 eip712DomainHash = EIP712_DOMAIN_HASH;
+
+        // Assembly for more efficient computing:
+        // keccak256(abi.encodePacked(
+        //     EIP191_HEADER,
+        //     EIP712_DOMAIN_HASH,
+        //     hashStruct    
+        // ));
+
+        assembly {
+            // Load free memory pointer
+            let memPtr := mload(64)
+
+            mstore(memPtr, 0x1901000000000000000000000000000000000000000000000000000000000000)  // EIP191 header
+            mstore(add(memPtr, 2), eip712DomainHash)                                            // EIP712 domain hash
+            mstore(add(memPtr, 34), hashStruct)                                                 // Hash of struct
+
+            // Compute hash
+            result := keccak256(memPtr, 66)
+        }
+        return result;
     }
 }
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibOrder.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibOrder.sol
index 4031ff26b..68f4f5f1b 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibOrder.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibOrder.sol
@@ -103,11 +103,12 @@ contract LibOrder is
         bytes32 takerAssetDataHash = keccak256(order.takerAssetData);
 
         // Assembly for more efficiently computing:
-        // keccak256(abi.encode(
-        //     order.makerAddress,
-        //     order.takerAddress,
-        //     order.feeRecipientAddress,
-        //     order.senderAddress,
+        // keccak256(abi.encodePacked(
+        //     EIP712_ORDER_SCHEMA_HASH,
+        //     bytes32(order.makerAddress),
+        //     bytes32(order.takerAddress),
+        //     bytes32(order.feeRecipientAddress),
+        //     bytes32(order.senderAddress),
         //     order.makerAssetAmount,
         //     order.takerAssetAmount,
         //     order.makerFee,
@@ -119,24 +120,26 @@ contract LibOrder is
         // ));
 
         assembly {
+            // Calculate memory addresses that will be swapped out before hashing
+            let pos1 := sub(order, 32)
+            let pos2 := add(order, 320)
+            let pos3 := add(order, 352)
+
             // Backup
-            // solhint-disable-next-line space-after-comma
-            let temp1 := mload(sub(order,  32))
-            let temp2 := mload(add(order, 320))
-            let temp3 := mload(add(order, 352))
+            let temp1 := mload(pos1)
+            let temp2 := mload(pos2)
+            let temp3 := mload(pos3)
             
             // Hash in place
-            // solhint-disable-next-line space-after-comma
-            mstore(sub(order,  32), schemaHash)
-            mstore(add(order, 320), makerAssetDataHash)
-            mstore(add(order, 352), takerAssetDataHash)
-            result := keccak256(sub(order, 32), 416)
+            mstore(pos1, schemaHash)
+            mstore(pos2, makerAssetDataHash)
+            mstore(pos3, takerAssetDataHash)
+            result := keccak256(pos1, 416)
             
             // Restore
-            // solhint-disable-next-line space-after-comma
-            mstore(sub(order,  32), temp1)
-            mstore(add(order, 320), temp2)
-            mstore(add(order, 352), temp3)
+            mstore(pos1, temp1)
+            mstore(pos2, temp2)
+            mstore(pos3, temp3)
         }
         return result;
     }