Check length before accessing indices, add awaitTransactionSuccess where needed, and rename function

5 files changed, 43 insertions, 25 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index 7c25c9770..79824fbbb 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -53,16 +53,18 @@ contract ERC20Proxy is
     {
         // Data must be intended for this proxy.
         uint256 length = assetMetadata.length;
+
+        require(
+            length == 21,
+            INVALID_METADATA_LENGTH
+        );
+
         require(
             uint8(assetMetadata[length - 1]) == PROXY_ID,
             PROXY_ID_MISMATCH
         );
 
         // Decode metadata.
-        require(
-            length == 21,
-            INVALID_METADATA_LENGTH
-        );
         address token = readAddress(assetMetadata, 0);
 
         // Transfer tokens.
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 59045f73a..ace3570bc 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -53,6 +53,12 @@ contract ERC721Proxy is
     {
         // Data must be intended for this proxy.
         uint256 length = assetMetadata.length;
+
+        require(
+            length == 53,
+            INVALID_METADATA_LENGTH
+        );
+
         require(
             uint8(assetMetadata[length - 1]) == PROXY_ID,
             PROXY_ID_MISMATCH
@@ -65,10 +71,6 @@ contract ERC721Proxy is
         );
 
         // Decode metadata
-        require(
-            length == 53,
-            INVALID_METADATA_LENGTH
-        );
         address token = readAddress(assetMetadata, 0);
         uint256 tokenId = readUint256(assetMetadata, 20);
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
index 7f4e20b5b..d40974e5f 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
@@ -56,10 +56,10 @@ contract MixinSignatureValidator is
         preSigned[hash][signer] = true;
     }
 
-    /// @dev Approves a Validator contract to verify signatures on signer's behalf.
+    /// @dev Approves/unnapproves 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(
+    function setSignatureValidatorApproval(
         address validator,
         bool approval
     )
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol
index 72f15a2a4..26e360c91 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol
@@ -31,10 +31,10 @@ contract ISignatureValidator {
     )
         external;
     
-    /// @dev Approves a Validator contract to verify signatures on signer's behalf.
+    /// @dev Approves/unnapproves 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(
+    function setSignatureValidatorApproval(
         address validator,
         bool approval
     )
diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts
index 2b2236e2e..6b3083ae5 100644
--- a/packages/contracts/test/exchange/transactions.ts
+++ b/packages/contracts/test/exchange/transactions.ts
@@ -220,9 +220,11 @@ describe('Exchange transactions', () => {
                 exchange.address,
             );
             const isApproved = true;
-            await exchange.approveSignatureValidator.sendTransactionAsync(whitelist.address, isApproved, {
-                from: takerAddress,
-            });
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await exchange.approveSignatureValidator.sendTransactionAsync(whitelist.address, isApproved, {
+                    from: takerAddress,
+                }),
+            );
             const defaultOrderParams = {
                 ...constants.STATIC_ORDER_PARAMS,
                 senderAddress: whitelist.address,
@@ -242,7 +244,9 @@ describe('Exchange transactions', () => {
 
         it('should revert if maker has not been whitelisted', async () => {
             const isApproved = true;
-            await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner });
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }),
+            );
 
             const orderStruct = orderUtils.getOrderStruct(signedOrder);
             const takerAssetFillAmount = signedOrder.takerAssetAmount;
@@ -260,7 +264,9 @@ describe('Exchange transactions', () => {
 
         it('should revert if taker has not been whitelisted', async () => {
             const isApproved = true;
-            await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner });
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }),
+            );
 
             const orderStruct = orderUtils.getOrderStruct(signedOrder);
             const takerAssetFillAmount = signedOrder.takerAssetAmount;
@@ -278,19 +284,27 @@ describe('Exchange transactions', () => {
 
         it('should fill the order if maker and taker have been whitelisted', async () => {
             const isApproved = true;
-            await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner });
-            await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner });
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }),
+            );
+
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }),
+            );
 
             const orderStruct = orderUtils.getOrderStruct(signedOrder);
             const takerAssetFillAmount = signedOrder.takerAssetAmount;
             const salt = generatePseudoRandomSalt();
-            await whitelist.fillOrderIfWhitelisted.sendTransactionAsync(
-                orderStruct,
-                takerAssetFillAmount,
-                salt,
-                signedOrder.signature,
-                { from: takerAddress },
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await whitelist.fillOrderIfWhitelisted.sendTransactionAsync(
+                    orderStruct,
+                    takerAssetFillAmount,
+                    salt,
+                    signedOrder.signature,
+                    { from: takerAddress },
+                ),
             );
+
             const newBalances = await erc20Wrapper.getBalancesAsync();
 
             const makerAssetFillAmount = signedOrder.makerAssetAmount;