Fixed review comments

contracts/exchange/contracts/src/MixinMatchOrders.sol

@@ -17,7 +17,6 @@ pragma experimental ABIEncoderV2;
 import "@0x/contracts-utils/contracts/src/LibBytes.sol";
 import "@0x/contracts-utils/contracts/src/LibRichErrors.sol";
 import "@0x/contracts-utils/contracts/src/ReentrancyGuard.sol";
-import "@0x/contracts-utils/contracts/src/RichErrors.sol";
 import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
 import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol";
 import "./interfaces/IAssetProxyDispatcher.sol";

contracts/utils/contracts/test/TestOwnable.sol

@@ -9,5 +9,8 @@ contract TestOwnable is
     function externalOnlyOwner()
         external
         onlyOwner
-    {} // solhint-disable-line no-empty-blocks
+        returns (bool)
+    {
+        return true;
+    }
 }

contracts/utils/contracts/test/TestReentrancyGuard.sol

@@ -26,29 +26,42 @@ contract TestReentrancyGuard is
 {
     uint256 internal counter = 2;
 
+    /// @dev Exposes the nonReentrant modifier publicly.
+    /// @param shouldBeAttacked True if the contract should be attacked.
+    /// @return True if successful.
     function guarded(bool shouldBeAttacked)
         external
         nonReentrant
+        returns (bool)
     {
         if (shouldBeAttacked) {
-            this.exploitive();
+            return this.exploitive();
         } else {
-            this.nonExploitive();
+            return this.nonExploitive();
         }
     }
 
+    /// @dev This is a function that will reenter the current contract.
+    /// @return True if successful.
     function exploitive()
         external
+        returns (bool)
     {
         if (counter > 0) {
             counter--;
             this.guarded(true);
         } else {
             counter = 2;
+            return true;
         }
     }
 
+    /// @dev This is a function that will not reenter the current contract.
+    /// @return True if successful.
     function nonExploitive()
         external
-    {} // solhint-disable-line no-empty-blocks
+        returns (bool)
+    {
+        return true;
+    }
 }

contracts/utils/contracts/test/TestSafeMath.sol

@@ -32,6 +32,14 @@ contract TestSafeMath is
         return _safeMul(a, b);
     }
 
+    function externalSafeDiv(uint256 a, uint256 b)
+        external
+        pure
+        returns (uint256)
+    {
+        return _safeDiv(a, b);
+    }
+
     function externalSafeSub(uint256 a, uint256 b)
         external
         pure

contracts/utils/test/lib_eip712.ts

@@ -81,11 +81,6 @@ describe('LibEIP712', () => {
 
     describe('_hashEIP712Message', () => {
         it('should correctly hash empty input', async () => {
-            /*
-            const expectedHash = hashEIP712Message(constants.NULL_BYTES32, constants.NULL_BYTES32);
-            const actualHash = await lib.externalHashEIP712Message.callAsync(constants.NULL_BYTES32, constants.NULL_BYTES32);
-            expect(actualHash).to.be.eq(expectedHash);
-             */
             await testHashEIP712MessageAsync(lib, constants.NULL_BYTES32, constants.NULL_BYTES32);
         });
     });

contracts/utils/test/lib_rich_errors.ts

@@ -1,5 +1,6 @@
 import { chaiSetup, constants, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils';
 import { BlockchainLifecycle } from '@0x/dev-utils';
+import { StringRevertError } from '@0x/utils';
 import * as chai from 'chai';
 import * as _ from 'lodash';
 
@@ -30,5 +31,10 @@ describe('LibEIP712', () => {
         it('should correctly revert the extra bytes', async () => {
             return expect(lib.externalRRevert.callAsync(constants.NULL_BYTES)).to.revertWith(constants.NULL_BYTES);
         });
+
+        it('should correctly revert a StringRevertError', async () => {
+            const error = new StringRevertError('foo');
+            return expect(lib.externalRRevert.callAsync(error.encode())).to.revertWith(error);
+        });
     });
 });

contracts/utils/test/ownable.ts

@@ -20,7 +20,7 @@ describe('Ownable', () => {
         owner = await accounts[0];
         nonOwner = await accounts[1];
         await blockchainLifecycle.startAsync();
-        // Deploy SafeMath from the owner address
+        // Deploy Ownable from the owner address
         txDefaults.from = owner;
         ownable = await TestOwnableContract.deployFrom0xArtifactAsync(artifacts.TestOwnable, provider, txDefaults);
     });
@@ -29,30 +29,31 @@ describe('Ownable', () => {
         await blockchainLifecycle.revertAsync();
     });
 
-    // tslint:disable:no-unused-expression
     describe('onlyOwner', () => {
-        it('should throw if sender is not owner', async () => {
+        it('should throw if sender is not the owner', async () => {
             const expectedError = new OwnableRevertErrors.OnlyOwnerError(nonOwner, owner);
             return expect(ownable.externalOnlyOwner.callAsync({ from: nonOwner })).to.revertWith(expectedError);
         });
 
-        it('should succeed if sender is owner', async () => {
+        it('should succeed if sender is the owner', async () => {
-            return expect(ownable.externalOnlyOwner.callAsync({ from: owner })).to.be.fulfilled('');
+            const isSuccessful = await ownable.externalOnlyOwner.callAsync({ from: owner });
+            expect(isSuccessful).to.be.true();
         });
     });
 
     describe('transferOwnership', () => {
         it('should not transfer ownership if the specified new owner is the zero address', async () => {
-            await expect(ownable.transferOwnership.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner })).to.be.fulfilled('');
+            expect(
+                ownable.transferOwnership.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner }),
+            ).to.be.fulfilled('');
             const updatedOwner = await ownable.owner.callAsync();
             expect(updatedOwner).to.be.eq(owner);
         });
 
         it('should transfer ownership if the specified new owner is not the zero address', async () => {
-            await expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled('');
+            expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled('');
             const updatedOwner = await ownable.owner.callAsync();
             expect(updatedOwner).to.be.eq(nonOwner);
         });
     });
-    // tslint:enable:no-unused-expression
 });

contracts/utils/test/reentrancy_guard.ts

@@ -34,7 +34,8 @@ describe('ReentrancyGuard', () => {
         });
 
         it('should succeed if reentrancy does not occur', async () => {
-            return expect(guard.guarded.sendTransactionAsync(false)).to.be.fulfilled('');
+            const isSuccessful = await guard.guarded.callAsync(false);
+            expect(isSuccessful).to.be.true();
         });
     });
 });

contracts/utils/test/safe_math.ts

@@ -55,6 +55,35 @@ describe('SafeMath', () => {
         });
     });
 
+    describe('_safeDiv', () => {
+        it('should return the correct value if both values are the same', async () => {
+            const result = await safeMath.externalSafeDiv.callAsync(toBN(1), toBN(1));
+            expect(result).bignumber.to.be.eq(toBN(1));
+        });
+
+        it('should return the correct value if the values are different', async () => {
+            const result = await safeMath.externalSafeDiv.callAsync(toBN(3), toBN(2));
+            expect(result).bignumber.to.be.eq(toBN(1));
+        });
+
+        it('should return zero if the numerator is smaller than the denominator', async () => {
+            const result = await safeMath.externalSafeDiv.callAsync(toBN(2), toBN(3));
+            expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT);
+        });
+
+        it('should return zero if first argument is zero', async () => {
+            const result = await safeMath.externalSafeDiv.callAsync(constants.ZERO_AMOUNT, toBN(1));
+            expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT);
+        });
+
+        it('should return zero if second argument is zero', async () => {
+            const errMessage = 'VM Exception while processing transaction: invalid opcode';
+            return expect(safeMath.externalSafeDiv.callAsync(toBN(1), constants.ZERO_AMOUNT)).to.be.rejectedWith(
+                errMessage,
+            );
+        });
+    });
+
     describe('_safeSub', () => {
         it('should throw if the subtraction underflows', async () => {
             const a = toBN(0);

packages/utils/src/sign_typed_data_utils.ts

@@ -1,10 +1,4 @@
-import {
+import { EIP712Object, EIP712ObjectValue, EIP712TypedData, EIP712Types } from '@0x/types';
-    EIP712DomainWithDefaultSchema,
-    EIP712Object,
-    EIP712ObjectValue,
-    EIP712TypedData,
-    EIP712Types,
-} from '@0x/types';
 import * as ethUtil from 'ethereumjs-util';
 import * as ethers from 'ethers';
 import * as _ from 'lodash';
@@ -45,7 +39,7 @@ export const signTypedDataUtils = {
                     { name: 'chainId', type: 'uint256' },
                     { name: 'verifyingContractAddress', type: 'address' },
                 ],
-            } as EIP712Types,
+            },
         );
     },
     _findDependencies(primaryType: string, types: EIP712Types, found: string[] = []): string[] {