Update Whitelist

5 files changed, 54 insertions, 24 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
index 6c018683e..8ef961a81 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
@@ -43,7 +43,8 @@ contract MixinSignatureValidator is
     function preSign(
         bytes32 hash,
         address signer,
-        bytes signature)
+        bytes signature
+    )
         external
     {
         require(
@@ -56,7 +57,10 @@ contract MixinSignatureValidator is
     /// @dev Approves a Validator contract to verify signatures on signer's behalf.
     /// @param validator Address of Validator contract.
     /// @param approval Approval or disapproval of  Validator contract.
-    function approveSignatureValidator(address validator, bool approval)
+    function approveSignatureValidator(
+        address validator,
+        bool approval
+    )
         external
     {
         allowedValidators[msg.sender][validator] = approval;
@@ -70,19 +74,19 @@ contract MixinSignatureValidator is
     function isValidSignature(
         bytes32 hash,
         address signer,
-        bytes memory signature)
+        bytes memory signature
+    )
         internal
         view
         returns (bool isValid)
     {
         // TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.)
-
         require(
             signature.length >= 1,
             INVALID_SIGNATURE_LENGTH
         );
 
-        // Pop last byte off of 
+        // Pop last byte off of signature byte array.
         SignatureType signatureType = SignatureType(uint8(popByte(signature)));
 
         // Variables are not scoped in Solidity.
@@ -90,7 +94,7 @@ contract MixinSignatureValidator is
         bytes32 r;
         bytes32 s;
         address recovered;
-
+        
         // Always illegal signature.
         // This is always an implicit option since a signer can create a
         // signature array with invalid type or length. We may as well make
@@ -173,7 +177,9 @@ contract MixinSignatureValidator is
         // | 0x00 + x | 20     | Address of validator contract   |
         // | 0x14 + x | 1      | Signature type is always "\x06" |
         } else if (signatureType == SignatureType.Validator) {
+            // Pop last 20 bytes off of signature byte array.
             address validator = popAddress(signature);
+            // Ensure signer has approved validator.
             if (!allowedValidators[signer][validator]) {
                 return false;
             }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
index d28df11d8..12bba1ef2 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
@@ -43,7 +43,8 @@ contract MixinTransactions is
         uint256 salt,
         address signer,
         bytes data,
-        bytes signature)
+        bytes signature
+    )
         external
     {
         // Prevent reentrancy
diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
index cd5206eb8..b835e70d0 100644
--- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
@@ -30,7 +30,7 @@ contract TestLibBytes is
     /// @return The byte that was popped off.
     function publicPopByte(bytes memory b)
         public
-        returns (bytes memory, byte result)
+        returns (bytes memory, bytes1 result)
     {
         result = popByte(b);
         return (b, result);
diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
index 11576fd89..034222915 100644
--- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
+++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
@@ -27,7 +27,9 @@ contract Whitelist is
     Ownable
 {
     // Revert reasons
-    string constant ADDRESS_NOT_WHITELISTED = "Address not whitelisted.";
+    string constant MAKER_NOT_WHITELISTED = "Maker address not whitelisted.";
+    string constant TAKER_NOT_WHITELISTED = "Taker address not whitelisted.";
+    string constant INVALID_SENDER = "Sender must equal transaction origin.";
 
     // Mapping of address => whitelist status.
     mapping (address => bool) public isWhitelisted;
@@ -42,7 +44,7 @@ contract Whitelist is
         public
     {
         EXCHANGE = IExchange(_exchange);
-        TX_ORIGIN_SIGNATURE = abi.encodePacked(VALIDATOR_SIGNATURE_BYTE, address(this));
+        TX_ORIGIN_SIGNATURE = abi.encodePacked(address(this), VALIDATOR_SIGNATURE_BYTE);
     }
 
     /// @dev Adds or removes an address from the whitelist.
@@ -67,24 +69,28 @@ contract Whitelist is
         LibOrder.Order memory order,
         uint256 takerAssetFillAmount,
         uint256 salt,
-        bytes memory orderSignature)
+        bytes memory orderSignature
+    )
         public
     {
         address takerAddress = msg.sender;
     
         // This contract must be the entry point for the transaction.
-        require(takerAddress == tx.origin);
+        require(
+            takerAddress == tx.origin,
+            INVALID_SENDER
+        );
 
         // Check if maker is on the whitelist.
         require(
             isWhitelisted[order.makerAddress],
-            ADDRESS_NOT_WHITELISTED
+            MAKER_NOT_WHITELISTED
         );
 
         // Check if taker is on the whitelist.
         require(
             isWhitelisted[takerAddress],
-            ADDRESS_NOT_WHITELISTED
+            TAKER_NOT_WHITELISTED
         );
 
         // Encode arguments into byte array.
@@ -111,7 +117,8 @@ contract Whitelist is
     function isValidSignature(
         bytes32 hash,
         address signer,
-        bytes signature)
+        bytes signature
+    )
         external
         view
         returns (bool isValid)
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index f0a622fe4..df2221c93 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -32,19 +32,23 @@ contract LibBytes {
     /// @return The byte that was popped off.
     function popByte(bytes memory b)
         internal
-        returns (byte result)
+        pure
+        returns (bytes1 result)
     {
         require(
             b.length > 0,
             GT_ZERO_LENGTH_REQUIRED
         );
 
+        // Store last byte.
         result = b[b.length - 1];
+        
         assembly {
+            // Decrement length of byte array.
             let newLen := sub(mload(b), 1)
             mstore(b, newLen)
         }
-        result;
+        return result;
     }
 
     /// @dev Pops the last 20 bytes off of a byte array by modifying its length.
@@ -52,6 +56,7 @@ contract LibBytes {
     /// @return The 20 byte address that was popped off.
     function popAddress(bytes memory b)
         internal
+        pure
         returns (address result)
     {
         require(
@@ -59,9 +64,11 @@ contract LibBytes {
             GTE_20_LENGTH_REQUIRED
         );
 
+        // Store last 20 bytes.
         result = readAddress(b, b.length - 20);
 
         assembly {
+            // Subtract 20 from byte array length.
             let newLen := sub(mload(b), 20)
             mstore(b, newLen)
         }
@@ -72,7 +79,10 @@ contract LibBytes {
     /// @param lhs First byte array to compare.
     /// @param rhs Second byte array to compare.
     /// @return True if arrays are the same. False otherwise.
-    function areBytesEqual(bytes memory lhs, bytes memory rhs)
+    function areBytesEqual(
+        bytes memory lhs,
+        bytes memory rhs
+    )
         internal
         pure
         returns (bool equal)
@@ -106,7 +116,8 @@ contract LibBytes {
     /// @return address from byte array.
     function readAddress(
         bytes memory b,
-        uint256 index)
+        uint256 index
+    )
         internal
         pure
         returns (address result)
@@ -138,7 +149,8 @@ contract LibBytes {
     function writeAddress(
         bytes memory b,
         uint256 index,
-        address input)
+        address input
+    )
         internal
         pure
     {
@@ -175,7 +187,8 @@ contract LibBytes {
     /// @return bytes32 value from byte array.
     function readBytes32(
         bytes memory b,
-        uint256 index)
+        uint256 index
+    )
         internal
         pure
         returns (bytes32 result)
@@ -202,7 +215,8 @@ contract LibBytes {
     function writeBytes32(
         bytes memory b,
         uint256 index,
-        bytes32 input)
+        bytes32 input
+    )
         internal
         pure
     {
@@ -226,7 +240,8 @@ contract LibBytes {
     /// @return uint256 value from byte array.
     function readUint256(
         bytes memory b,
-        uint256 index)
+        uint256 index
+    )
         internal
         pure
         returns (uint256 result)
@@ -241,7 +256,8 @@ contract LibBytes {
     function writeUint256(
         bytes memory b,
         uint256 index,
-        uint256 input)
+        uint256 input
+    )
         internal
         pure
     {