Move readFirst4 to LibBytes

5 files changed, 53 insertions, 39 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
index 1061d0b9a..1b74dfc44 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
@@ -19,8 +19,10 @@
 pragma solidity ^0.4.10;
 
 import "../../multisig/MultiSigWalletWithTimeLock.sol";
+import "../../utils/LibBytes/LibBytes.sol";
 
 contract AssetProxyOwner is
+    LibBytes,
     MultiSigWalletWithTimeLock
 {
 
@@ -31,6 +33,8 @@ contract AssetProxyOwner is
 
     bytes4 constant REMOVE_AUTHORIZED_ADDRESS_SELECTOR = bytes4(keccak256("removeAuthorizedAddress(address)"));
 
+    /// @dev Function will revert if the transaction does not call `removeAuthorizedAddress`
+    ///      on an approved AssetProxy contract.
     modifier validRemoveAuthorizedAddressTx(uint256 transactionId) {
         Transaction storage tx = transactions[transactionId];
         require(isAssetProxyRegistered[tx.destination]);
@@ -41,20 +45,22 @@ contract AssetProxyOwner is
     /// @dev Contract constructor sets initial owners, required number of confirmations,
     ///      time lock, and list of AssetProxy addresses.
     /// @param _owners List of initial owners.
+    /// @param _assetProxyContracts Array of AssetProxy contract addresses.
     /// @param _required Number of required confirmations.
     /// @param _secondsTimeLocked Duration needed after a transaction is confirmed and before it becomes executable, in seconds.
-    /// @param _assetProxyContracts Array of AssetProxy contract addresses.
     function AssetProxyOwner(
         address[] memory _owners,
+        address[] memory _assetProxyContracts,
         uint256 _required,
-        uint256 _secondsTimeLocked,
-        address[] memory _assetProxyContracts)
+        uint256 _secondsTimeLocked
+    )
         public
         MultiSigWalletWithTimeLock(_owners, _required, _secondsTimeLocked)
     {
         for (uint256 i = 0; i < _assetProxyContracts.length; i++) {
-            require(_assetProxyContracts[i] != address(0));
-            isAssetProxyRegistered[_assetProxyContracts[i]] = true;
+            address assetProxy = _assetProxyContracts[i];
+            require(assetProxy != address(0));
+            isAssetProxyRegistered[assetProxy] = true;
         }
     }
 
@@ -101,19 +107,4 @@ contract AssetProxyOwner is
         require(REMOVE_AUTHORIZED_ADDRESS_SELECTOR == first4Bytes);
         return true;
     }
-
-    /// @dev Reads the first 4 bytes from a byte array of arbitrary length.
-    /// @param data Byte array to read first 4 bytes from.
-    /// @return First 4 bytes of data.
-    function readFirst4(bytes memory data)
-        public
-        pure
-        returns (bytes4 result)
-    {
-        require(data.length >= 4);
-        assembly {
-            result := mload(add(data, 32))
-        }
-        return result;
-    }
 }
diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
index ac4602933..5a6801262 100644
--- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
@@ -130,4 +130,16 @@ contract TestLibBytes is
         writeUint256(b, index, input);
         return b;
     }
+
+    /// @dev Reads the first 4 bytes from a byte array of arbitrary length.
+    /// @param b Byte array to read first 4 bytes from.
+    /// @return First 4 bytes of data.
+    function publicReadFirst4(bytes memory b)
+        public
+        pure
+        returns (bytes4 result)
+    {
+        result = readFirst4(b);
+        return result;
+    }
 }
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index 2c5d9e756..975565773 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -21,6 +21,7 @@ pragma solidity ^0.4.24;
 contract LibBytes {
 
     // Revert reasons
+    string constant GTE_4_LENGTH_REQUIRED = "Length must be greater than or equal to 4.";
     string constant GTE_20_LENGTH_REQUIRED = "Length must be greater than or equal to 20.";
     string constant GTE_32_LENGTH_REQUIRED = "Length must be greater than or equal to 32.";
 
@@ -203,4 +204,22 @@ contract LibBytes {
     {
         writeBytes32(b, index, bytes32(input));
     }
+
+    /// @dev Reads the first 4 bytes from a byte array of arbitrary length.
+    /// @param b Byte array to read first 4 bytes from.
+    /// @return First 4 bytes of data.
+    function readFirst4(bytes memory b)
+        internal
+        pure
+        returns (bytes4 result)
+    {
+        require(
+            b.length >= 4,
+            GTE_4_LENGTH_REQUIRED
+        );
+        assembly {
+            result := mload(add(b, 32))
+        }
+        return result;
+    }
 }
diff --git a/packages/contracts/test/asset_proxy_owner.ts b/packages/contracts/test/asset_proxy_owner.ts
index 6e999dd99..e3c6a5324 100644
--- a/packages/contracts/test/asset_proxy_owner.ts
+++ b/packages/contracts/test/asset_proxy_owner.ts
@@ -56,9 +56,9 @@ describe('AssetProxyOwner', () => {
             provider,
             txDefaults,
             owners,
+            defaultAssetProxyContractAddresses,
             REQUIRED_APPROVALS,
             SECONDS_TIME_LOCKED,
-            defaultAssetProxyContractAddresses,
         );
         multiSigWrapper = new MultiSigWrapper(multiSig, zeroEx);
         await erc20Proxy.transferOwnership.sendTransactionAsync(multiSig.address, { from: initialOwner });
@@ -79,9 +79,9 @@ describe('AssetProxyOwner', () => {
                 provider,
                 txDefaults,
                 owners,
+                assetProxyContractAddresses,
                 REQUIRED_APPROVALS,
                 SECONDS_TIME_LOCKED,
-                assetProxyContractAddresses,
             );
             const isErc20ProxyRegistered = await newMultiSig.isAssetProxyRegistered.callAsync(erc20Proxy.address);
             const isErc721ProxyRegistered = await newMultiSig.isAssetProxyRegistered.callAsync(erc721Proxy.address);
@@ -96,29 +96,13 @@ describe('AssetProxyOwner', () => {
                     provider,
                     txDefaults,
                     owners,
+                    assetProxyContractAddresses,
                     REQUIRED_APPROVALS,
                     SECONDS_TIME_LOCKED,
-                    assetProxyContractAddresses,
                 ),
             ).to.be.rejectedWith(constants.REVERT);
         });
     });
-    describe('readFirst4', () => {
-        it('should return the first 4 bytes of a byte array of arbitrary length', async () => {
-            const addAuthorizedAddressData = erc20Proxy.addAuthorizedAddress.getABIEncodedTransactionData(owners[0]);
-            const removeAuthorizedAddressData = erc20Proxy.removeAuthorizedAddress.getABIEncodedTransactionData(
-                owners[0],
-            );
-            const expectedAddAuthorizedAddressSelector = addAuthorizedAddressData.slice(0, 10);
-            const expectedRemoveAuthorizedAddressSelector = removeAuthorizedAddressData.slice(0, 10);
-            const [addAuthorizedAddressSelector, removeAuthorizedAddressSelector] = await Promise.all([
-                multiSig.readFirst4.callAsync(addAuthorizedAddressData),
-                multiSig.readFirst4.callAsync(removeAuthorizedAddressData),
-            ]);
-            expect(expectedAddAuthorizedAddressSelector).to.equal(addAuthorizedAddressSelector);
-            expect(expectedRemoveAuthorizedAddressSelector).to.equal(removeAuthorizedAddressSelector);
-        });
-    });
 
     describe('isFunctionRemoveAuthorizedAddress', () => {
         it('should throw if data is not for removeAuthorizedAddress', async () => {
diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts
index ce3adbdae..fc28c363b 100644
--- a/packages/contracts/test/libraries/lib_bytes.ts
+++ b/packages/contracts/test/libraries/lib_bytes.ts
@@ -248,4 +248,12 @@ describe('LibBytes', () => {
         it('should fail if the length between the offset and end of the byte array is too short to hold a uint256)', async () => {});
     });
     */
+
+    describe('readFirst4', () => {
+        it('should return the first 4 bytes of a byte array of arbitrary length', async () => {
+            const first4Bytes = libBytes.publicReadFirst4.callAsync(byteArrayLongerThan32Bytes);
+            const expectedFirst4Bytes = byteArrayLongerThan32Bytes.slice(0, 10);
+            expect(first4Bytes).to.equal(expectedFirst4Bytes);
+        });
+    });
 });