From 3ca3a2820dd4a061cae78ae0977ba7e198f09aac Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 12:04:03 -0700 Subject: [PATCH] Fixed review comments --- .../contracts/src/MixinMatchOrders.sol | 1 - .../utils/contracts/test/TestOwnable.sol | 5 +++- .../contracts/test/TestReentrancyGuard.sol | 19 ++++++++++-- .../utils/contracts/test/TestSafeMath.sol | 8 +++++ contracts/utils/test/lib_eip712.ts | 5 ---- contracts/utils/test/lib_rich_errors.ts | 6 ++++ contracts/utils/test/ownable.ts | 17 ++++++----- contracts/utils/test/reentrancy_guard.ts | 3 +- contracts/utils/test/safe_math.ts | 29 +++++++++++++++++++ packages/utils/src/sign_typed_data_utils.ts | 10 ++----- 10 files changed, 76 insertions(+), 27 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index c3954e62a3..04ddb08a9b 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/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"; diff --git a/contracts/utils/contracts/test/TestOwnable.sol b/contracts/utils/contracts/test/TestOwnable.sol index e79033c5cc..a62d3c713d 100644 --- a/contracts/utils/contracts/test/TestOwnable.sol +++ b/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; + } } diff --git a/contracts/utils/contracts/test/TestReentrancyGuard.sol b/contracts/utils/contracts/test/TestReentrancyGuard.sol index 54394f712d..7a878fd072 100644 --- a/contracts/utils/contracts/test/TestReentrancyGuard.sol +++ b/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; + } } diff --git a/contracts/utils/contracts/test/TestSafeMath.sol b/contracts/utils/contracts/test/TestSafeMath.sol index 5b9d7f7544..36670e13af 100644 --- a/contracts/utils/contracts/test/TestSafeMath.sol +++ b/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 diff --git a/contracts/utils/test/lib_eip712.ts b/contracts/utils/test/lib_eip712.ts index 07bc5b9e96..479627b688 100644 --- a/contracts/utils/test/lib_eip712.ts +++ b/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); }); }); diff --git a/contracts/utils/test/lib_rich_errors.ts b/contracts/utils/test/lib_rich_errors.ts index 7178275162..1245cf0cd2 100644 --- a/contracts/utils/test/lib_rich_errors.ts +++ b/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); + }); }); }); diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index 62d2a3443e..6fc908e13a 100644 --- a/contracts/utils/test/ownable.ts +++ b/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 () => { - return expect(ownable.externalOnlyOwner.callAsync({ from: owner })).to.be.fulfilled(''); + it('should succeed if sender is the owner', async () => { + 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 }); diff --git a/contracts/utils/test/reentrancy_guard.ts b/contracts/utils/test/reentrancy_guard.ts index d7f224d74c..e2bf9c38f0 100644 --- a/contracts/utils/test/reentrancy_guard.ts +++ b/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(); }); }); }); diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts index e9159b6b01..aa4f1f35bc 100644 --- a/contracts/utils/test/safe_math.ts +++ b/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); diff --git a/packages/utils/src/sign_typed_data_utils.ts b/packages/utils/src/sign_typed_data_utils.ts index f923ec640d..b9b9502a84 100644 --- a/packages/utils/src/sign_typed_data_utils.ts +++ b/packages/utils/src/sign_typed_data_utils.ts @@ -1,10 +1,4 @@ -import { - EIP712DomainWithDefaultSchema, - EIP712Object, - EIP712ObjectValue, - EIP712TypedData, - EIP712Types, -} from '@0x/types'; +import { 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[] {