From 4a4d2e70799b6b85bf8c09eaba61f2cc5dc487b6 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Fri, 26 Jul 2019 13:11:29 -0700 Subject: [PATCH 01/17] Added unit tests for SafeMath --- contracts/utils/compiler.json | 3 +- .../utils/contracts/test/TestSafeMath.sol | 66 ++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 4 + contracts/utils/src/wrappers.ts | 2 + contracts/utils/test/safe_math.ts | 146 ++++++++++++++++++ contracts/utils/tsconfig.json | 4 +- packages/utils/src/safe_math_revert_errors.ts | 2 +- 8 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 contracts/utils/contracts/test/TestSafeMath.sol create mode 100644 contracts/utils/test/safe_math.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index 600011d083..e0b173967b 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -35,6 +35,7 @@ "src/interfaces/IOwnable.sol", "test/TestConstants.sol", "test/TestLibAddressArray.sol", - "test/TestLibBytes.sol" + "test/TestLibBytes.sol", + "test/TestSafeMath.sol" ] } diff --git a/contracts/utils/contracts/test/TestSafeMath.sol b/contracts/utils/contracts/test/TestSafeMath.sol new file mode 100644 index 0000000000..5b9d7f7544 --- /dev/null +++ b/contracts/utils/contracts/test/TestSafeMath.sol @@ -0,0 +1,66 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "../src/SafeMath.sol"; + + +contract TestSafeMath is + SafeMath +{ + function externalSafeMul(uint256 a, uint256 b) + external + pure + returns (uint256) + { + return _safeMul(a, b); + } + + function externalSafeSub(uint256 a, uint256 b) + external + pure + returns (uint256) + { + return _safeSub(a, b); + } + + function externalSafeAdd(uint256 a, uint256 b) + external + pure + returns (uint256) + { + return _safeAdd(a, b); + } + + function externalMaxUint256(uint256 a, uint256 b) + external + pure + returns (uint256) + { + return _max256(a, b); + } + + function externalMinUint256(uint256 a, uint256 b) + external + pure + returns (uint256) + { + return _min256(a, b); + } +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 36c46f76e0..73e2f9308e 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index 2b82dbd63f..4028caad50 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -8,6 +8,7 @@ import { ContractArtifact } from 'ethereum-types'; import * as IOwnable from '../generated-artifacts/IOwnable.json'; import * as LibAddress from '../generated-artifacts/LibAddress.json'; import * as LibBytes from '../generated-artifacts/LibBytes.json'; +import * as LibEIP1271 from '../generated-artifacts/LibEIP1271.json'; import * as LibEIP712 from '../generated-artifacts/LibEIP712.json'; import * as Ownable from '../generated-artifacts/Ownable.json'; import * as ReentrancyGuard from '../generated-artifacts/ReentrancyGuard.json'; @@ -16,9 +17,11 @@ import * as SafeMath from '../generated-artifacts/SafeMath.json'; import * as TestConstants from '../generated-artifacts/TestConstants.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; +import * as TestSafeMath from '../generated-artifacts/TestSafeMath.json'; export const artifacts = { LibAddress: LibAddress as ContractArtifact, LibBytes: LibBytes as ContractArtifact, + LibEIP1271: LibEIP1271 as ContractArtifact, LibEIP712: LibEIP712 as ContractArtifact, Ownable: Ownable as ContractArtifact, ReentrancyGuard: ReentrancyGuard as ContractArtifact, @@ -28,4 +31,5 @@ export const artifacts = { TestConstants: TestConstants as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, + TestSafeMath: TestSafeMath as ContractArtifact, }; diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index 94aab4f426..2adf5244da 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -6,6 +6,7 @@ export * from '../generated-wrappers/i_ownable'; export * from '../generated-wrappers/lib_address'; export * from '../generated-wrappers/lib_bytes'; +export * from '../generated-wrappers/lib_e_i_p1271'; export * from '../generated-wrappers/lib_e_i_p712'; export * from '../generated-wrappers/ownable'; export * from '../generated-wrappers/reentrancy_guard'; @@ -14,3 +15,4 @@ export * from '../generated-wrappers/safe_math'; export * from '../generated-wrappers/test_constants'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; +export * from '../generated-wrappers/test_safe_math'; diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts new file mode 100644 index 0000000000..ccbf0117e3 --- /dev/null +++ b/contracts/utils/test/safe_math.ts @@ -0,0 +1,146 @@ +import { chaiSetup, constants, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import { BigNumber, SafeMathRevertErrors } from '@0x/utils'; +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { artifacts, TestSafeMathContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +function toBN(a: number | string): BigNumber { + return new BigNumber(a); +} + +describe('SafeMath', () => { + let safeMath: TestSafeMathContract; + + before(async () => { + await blockchainLifecycle.startAsync(); + // Deploy SafeMath + safeMath = await TestSafeMathContract.deployFrom0xArtifactAsync( + artifacts.TestSafeMath, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('_safeMul', () => { + it('should return zero if first argument is zero', async () => { + const result = await safeMath.externalSafeMul.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 result = await safeMath.externalSafeMul.callAsync(toBN(1), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); + }); + + it('should revert if the multiplication overflows', async () => { + const a = toBN('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number + const b = toBN(2); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, + a, + b, + ); + return expect(safeMath.externalSafeMul.callAsync(a, b)).to.revertWith(expectedError); + }); + + it('should calculate correct value for values that don\'t overflow', async () => { + const result = await safeMath.externalSafeMul.callAsync(toBN(15), toBN(13)); + expect(result).bignumber.to.be.eq(toBN(195)); + }); + }); + + describe('_safeSub', () => { + it('should throw if the subtraction underflows', async () => { + const a = toBN(0); + const b = toBN(1); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256SubtractionUnderflow, + a, + b, + ); + return expect(safeMath.externalSafeSub.callAsync(a, b)).to.revertWith(expectedError); + }); + + it('should calculate correct value for values that are equal', async () => { + const result = await safeMath.externalSafeMul.callAsync(constants.ZERO_AMOUNT, constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); + }); + + it('should calculate correct value for values that are not equal', async () => { + const result = await safeMath.externalSafeSub.callAsync(toBN(15), toBN(13)); + expect(result).bignumber.to.be.eq(toBN(2)); + }); + }); + + describe('_safeAdd', () => { + it('should throw if the addition overflows', async () => { + const a = toBN('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number + const b = toBN(1); + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256AdditionOverflow, + a, + b, + ); + return expect(safeMath.externalSafeAdd.callAsync(a, b)).to.revertWith(expectedError); + }); + + it('should calculate correct value if addition does not overflow', async () => { + const result = await safeMath.externalSafeAdd.callAsync(toBN(15), toBN(13)); + expect(result).bignumber.to.be.eq(toBN(28)); + }); + + it('should calculate correct value if first argument is zero', async () => { + const result = await safeMath.externalSafeAdd.callAsync(constants.ZERO_AMOUNT, toBN(13)); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + + it('should calculate correct value if second argument is zero', async () => { + const result = await safeMath.externalSafeAdd.callAsync(toBN(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + }); + + describe('_maxUint256', () => { + it('should return first argument if it is greater than the second', async () => { + const result = await safeMath.externalMaxUint256.callAsync(toBN(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + + it('should return first argument if it is equal the second', async () => { + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); + }); + + it('should return second argument if it is greater than the first', async () => { + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBN(13)); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + }); + + describe('_minUint256', () => { + it('should return first argument if it is less than the second', async () => { + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBN(13)); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + + it('should return first argument if it is equal the second', async () => { + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); + }); + + it('should return second argument if it is greater than the first', async () => { + const result = await safeMath.externalMaxUint256.callAsync(toBN(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBN(13)); + }); + }); +}); diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index 962e961380..c89e534ee4 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -6,6 +6,7 @@ "generated-artifacts/IOwnable.json", "generated-artifacts/LibAddress.json", "generated-artifacts/LibBytes.json", + "generated-artifacts/LibEIP1271.json", "generated-artifacts/LibEIP712.json", "generated-artifacts/Ownable.json", "generated-artifacts/ReentrancyGuard.json", @@ -13,7 +14,8 @@ "generated-artifacts/SafeMath.json", "generated-artifacts/TestConstants.json", "generated-artifacts/TestLibAddressArray.json", - "generated-artifacts/TestLibBytes.json" + "generated-artifacts/TestLibBytes.json", + "generated-artifacts/TestSafeMath.json" ], "exclude": ["./deploy/solc/solc_bin"] } diff --git a/packages/utils/src/safe_math_revert_errors.ts b/packages/utils/src/safe_math_revert_errors.ts index e91f90ef40..897123f8da 100644 --- a/packages/utils/src/safe_math_revert_errors.ts +++ b/packages/utils/src/safe_math_revert_errors.ts @@ -10,7 +10,7 @@ export enum SafeMathErrorCodes { } export class SafeMathError extends RevertError { - constructor(error?: SafeMathErrorCodes, a?: BigNumber | number | string, b?: BigNumber | number | string) { + constructor(error?: SafeMathErrorCodes, a?: BigNumber, b?: BigNumber) { super('SafeMathError', 'SafeMathError(uint8 error, uint256 a, uint256 b)', { error, a, From f4f922acb53a3e9d01f7fd1784a04cfe26014154 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Fri, 26 Jul 2019 13:14:39 -0700 Subject: [PATCH 02/17] Added unit tests for Ownable --- contracts/utils/compiler.json | 1 + .../utils/contracts/test/TestOwnable.sol | 13 ++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 2 + contracts/utils/src/wrappers.ts | 1 + contracts/utils/test/ownable.ts | 65 +++++++++++++++++++ contracts/utils/tsconfig.json | 1 + 7 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 contracts/utils/contracts/test/TestOwnable.sol create mode 100644 contracts/utils/test/ownable.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index e0b173967b..7d494ae78d 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -36,6 +36,7 @@ "test/TestConstants.sol", "test/TestLibAddressArray.sol", "test/TestLibBytes.sol", + "test/TestOwnable.sol", "test/TestSafeMath.sol" ] } diff --git a/contracts/utils/contracts/test/TestOwnable.sol b/contracts/utils/contracts/test/TestOwnable.sol new file mode 100644 index 0000000000..e79033c5cc --- /dev/null +++ b/contracts/utils/contracts/test/TestOwnable.sol @@ -0,0 +1,13 @@ +pragma solidity ^0.5.9; + +import "../src/Ownable.sol"; + + +contract TestOwnable is + Ownable +{ + function externalOnlyOwner() + external + onlyOwner + {} // solhint-disable-line no-empty-blocks +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 73e2f9308e..9ddbceb9d1 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestSafeMath).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestOwnable|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index 4028caad50..9267a5b6b7 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -17,6 +17,7 @@ import * as SafeMath from '../generated-artifacts/SafeMath.json'; import * as TestConstants from '../generated-artifacts/TestConstants.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; +import * as TestOwnable from '../generated-artifacts/TestOwnable.json'; import * as TestSafeMath from '../generated-artifacts/TestSafeMath.json'; export const artifacts = { LibAddress: LibAddress as ContractArtifact, @@ -31,5 +32,6 @@ export const artifacts = { TestConstants: TestConstants as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, + TestOwnable: TestOwnable as ContractArtifact, TestSafeMath: TestSafeMath as ContractArtifact, }; diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index 2adf5244da..5dffb7b8f4 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -15,4 +15,5 @@ export * from '../generated-wrappers/safe_math'; export * from '../generated-wrappers/test_constants'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; +export * from '../generated-wrappers/test_ownable'; export * from '../generated-wrappers/test_safe_math'; diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts new file mode 100644 index 0000000000..451f9632bc --- /dev/null +++ b/contracts/utils/test/ownable.ts @@ -0,0 +1,65 @@ +import { chaiSetup, constants, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import { OwnableRevertErrors } from '@0x/utils'; +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { artifacts, TestOwnableContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +describe('Ownable', () => { + let ownable: TestOwnableContract; + let owner: string; + let nonOwner: string; + + before(async () => { + const accounts = await web3Wrapper.getAvailableAddressesAsync(); + owner = await accounts[0]; + nonOwner = await accounts[1]; + await blockchainLifecycle.startAsync(); + // Deploy SafeMath from the owner address + txDefaults.from = owner; + ownable = await TestOwnableContract.deployFrom0xArtifactAsync( + artifacts.TestOwnable, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + // tslint:disable:no-unused-expression + describe('onlyOwner', () => { + it('should throw if sender is not 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 () => { + expect(ownable.externalOnlyOwner.callAsync({ from: owner })).to.be.fulfilled; + }); + }); + + describe('transferOwnership', () => { + it('should not transfer ownership if the specified new owner is the zero address', async () => { + 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 () => { + 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/tsconfig.json b/contracts/utils/tsconfig.json index c89e534ee4..3f1913dd7e 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -15,6 +15,7 @@ "generated-artifacts/TestConstants.json", "generated-artifacts/TestLibAddressArray.json", "generated-artifacts/TestLibBytes.json", + "generated-artifacts/TestOwnable.json", "generated-artifacts/TestSafeMath.json" ], "exclude": ["./deploy/solc/solc_bin"] From 6efb7027b593c3e369bb23e446e034b2963c488a Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Fri, 26 Jul 2019 15:42:12 -0700 Subject: [PATCH 03/17] Added unit tests for ReentrancyGuard --- contracts/utils/compiler.json | 2 +- contracts/utils/contracts/src/RichErrors.sol | 58 ------------------- .../contracts/test/TestReentrancyGuard.sol | 54 +++++++++++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 4 +- contracts/utils/src/wrappers.ts | 2 +- contracts/utils/test/reentrancy_guard.ts | 40 +++++++++++++ contracts/utils/tsconfig.json | 2 +- 8 files changed, 100 insertions(+), 64 deletions(-) delete mode 100644 contracts/utils/contracts/src/RichErrors.sol create mode 100644 contracts/utils/contracts/test/TestReentrancyGuard.sol create mode 100644 contracts/utils/test/reentrancy_guard.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index 7d494ae78d..a81005b841 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -30,13 +30,13 @@ "src/LibEIP712.sol", "src/Ownable.sol", "src/ReentrancyGuard.sol", - "src/RichErrors.sol", "src/SafeMath.sol", "src/interfaces/IOwnable.sol", "test/TestConstants.sol", "test/TestLibAddressArray.sol", "test/TestLibBytes.sol", "test/TestOwnable.sol", + "test/TestReentrancyGuard.sol", "test/TestSafeMath.sol" ] } diff --git a/contracts/utils/contracts/src/RichErrors.sol b/contracts/utils/contracts/src/RichErrors.sol deleted file mode 100644 index ebba8b7d4f..0000000000 --- a/contracts/utils/contracts/src/RichErrors.sol +++ /dev/null @@ -1,58 +0,0 @@ -/* - - Copyright 2019 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity ^0.5.9; - - -contract RichErrors { - - // bytes4(keccak256("Error(string)")) - bytes4 internal constant STANDARD_ERROR_SELECTOR = - 0x08c379a0; - - // solhint-disable func-name-mixedcase - /// @dev ABI encode a standard, string revert error payload. - /// This is the same payload that would be included by a `revert(string)` - /// solidity statement. It has the function signature `Error(string)`. - /// @param message The error string. - /// @return The ABI encoded error. - function StandardError( - string memory message - ) - internal - pure - returns (bytes memory) - { - return abi.encodeWithSelector( - STANDARD_ERROR_SELECTOR, - bytes(message) - ); - } - // solhint-enable func-name-mixedcase - - /// @dev Reverts an encoded rich revert reason `errorData`. - /// @param errorData ABI encoded error data. - function _rrevert(bytes memory errorData) - internal - pure - { - assembly { - revert(add(errorData, 0x20), mload(errorData)) - } - } -} diff --git a/contracts/utils/contracts/test/TestReentrancyGuard.sol b/contracts/utils/contracts/test/TestReentrancyGuard.sol new file mode 100644 index 0000000000..54394f712d --- /dev/null +++ b/contracts/utils/contracts/test/TestReentrancyGuard.sol @@ -0,0 +1,54 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "../src/ReentrancyGuard.sol"; + + +contract TestReentrancyGuard is + ReentrancyGuard +{ + uint256 internal counter = 2; + + function guarded(bool shouldBeAttacked) + external + nonReentrant + { + if (shouldBeAttacked) { + this.exploitive(); + } else { + this.nonExploitive(); + } + } + + function exploitive() + external + { + if (counter > 0) { + counter--; + this.guarded(true); + } else { + counter = 2; + } + } + + function nonExploitive() + external + {} // solhint-disable-line no-empty-blocks +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 9ddbceb9d1..b2a95da17a 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|RichErrors|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestOwnable|TestSafeMath).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestOwnable|TestReentrancyGuard|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index 9267a5b6b7..2f171de7a5 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -12,12 +12,12 @@ import * as LibEIP1271 from '../generated-artifacts/LibEIP1271.json'; import * as LibEIP712 from '../generated-artifacts/LibEIP712.json'; import * as Ownable from '../generated-artifacts/Ownable.json'; import * as ReentrancyGuard from '../generated-artifacts/ReentrancyGuard.json'; -import * as RichErrors from '../generated-artifacts/RichErrors.json'; import * as SafeMath from '../generated-artifacts/SafeMath.json'; import * as TestConstants from '../generated-artifacts/TestConstants.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; import * as TestOwnable from '../generated-artifacts/TestOwnable.json'; +import * as TestReentrancyGuard from '../generated-artifacts/TestReentrancyGuard.json'; import * as TestSafeMath from '../generated-artifacts/TestSafeMath.json'; export const artifacts = { LibAddress: LibAddress as ContractArtifact, @@ -26,12 +26,12 @@ export const artifacts = { LibEIP712: LibEIP712 as ContractArtifact, Ownable: Ownable as ContractArtifact, ReentrancyGuard: ReentrancyGuard as ContractArtifact, - RichErrors: RichErrors as ContractArtifact, SafeMath: SafeMath as ContractArtifact, IOwnable: IOwnable as ContractArtifact, TestConstants: TestConstants as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, TestOwnable: TestOwnable as ContractArtifact, + TestReentrancyGuard: TestReentrancyGuard as ContractArtifact, TestSafeMath: TestSafeMath as ContractArtifact, }; diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index 5dffb7b8f4..8b4176ab7f 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -10,10 +10,10 @@ export * from '../generated-wrappers/lib_e_i_p1271'; export * from '../generated-wrappers/lib_e_i_p712'; export * from '../generated-wrappers/ownable'; export * from '../generated-wrappers/reentrancy_guard'; -export * from '../generated-wrappers/rich_errors'; export * from '../generated-wrappers/safe_math'; export * from '../generated-wrappers/test_constants'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; export * from '../generated-wrappers/test_ownable'; +export * from '../generated-wrappers/test_reentrancy_guard'; export * from '../generated-wrappers/test_safe_math'; diff --git a/contracts/utils/test/reentrancy_guard.ts b/contracts/utils/test/reentrancy_guard.ts new file mode 100644 index 0000000000..304c755a8b --- /dev/null +++ b/contracts/utils/test/reentrancy_guard.ts @@ -0,0 +1,40 @@ +import { chaiSetup, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import { ReentrancyGuardRevertErrors } from '@0x/utils'; +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { artifacts, TestReentrancyGuardContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +describe('ReentrancyGuard', () => { + let guard: TestReentrancyGuardContract; + + before(async () => { + await blockchainLifecycle.startAsync(); + // Deploy TestReentrancyGuard + guard = await TestReentrancyGuardContract.deployFrom0xArtifactAsync( + artifacts.TestReentrancyGuard, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('nonReentrant', () => { + it('should throw if reentrancy occurs', async () => { + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); + return expect(guard.guarded.sendTransactionAsync(true)).to.revertWith(expectedError); + }); + + it('should succeed if reentrancy does not occur', async () => { + expect(guard.guarded.sendTransactionAsync(false)).to.be.fulfilled; // tslint:disable-line:no-unused-expression + }); + }); +}); diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index 3f1913dd7e..891fbe342f 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -10,12 +10,12 @@ "generated-artifacts/LibEIP712.json", "generated-artifacts/Ownable.json", "generated-artifacts/ReentrancyGuard.json", - "generated-artifacts/RichErrors.json", "generated-artifacts/SafeMath.json", "generated-artifacts/TestConstants.json", "generated-artifacts/TestLibAddressArray.json", "generated-artifacts/TestLibBytes.json", "generated-artifacts/TestOwnable.json", + "generated-artifacts/TestReentrancyGuard.json", "generated-artifacts/TestSafeMath.json" ], "exclude": ["./deploy/solc/solc_bin"] From 065f46a0209ac3c6d586e2cd296fbef9d660ea63 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 29 Jul 2019 10:24:14 -0700 Subject: [PATCH 04/17] Added unit tests for LibAddress --- contracts/utils/compiler.json | 1 + contracts/utils/contracts/src/LibAddress.sol | 2 +- .../utils/contracts/test/TestLibAddress.sol | 35 ++++++++++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 2 + contracts/utils/src/wrappers.ts | 1 + contracts/utils/test/lib_address.ts | 41 +++++++++++++++++++ contracts/utils/tsconfig.json | 1 + 8 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 contracts/utils/contracts/test/TestLibAddress.sol create mode 100644 contracts/utils/test/lib_address.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index a81005b841..289cf93c27 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -33,6 +33,7 @@ "src/SafeMath.sol", "src/interfaces/IOwnable.sol", "test/TestConstants.sol", + "test/TestLibAddress.sol", "test/TestLibAddressArray.sol", "test/TestLibBytes.sol", "test/TestOwnable.sol", diff --git a/contracts/utils/contracts/src/LibAddress.sol b/contracts/utils/contracts/src/LibAddress.sol index cd89e23cbd..cd0d41792e 100644 --- a/contracts/utils/contracts/src/LibAddress.sol +++ b/contracts/utils/contracts/src/LibAddress.sol @@ -1,6 +1,6 @@ /* - Copyright 2018 ZeroEx Intl. + Copyright 2019 ZeroEx Intl. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/contracts/utils/contracts/test/TestLibAddress.sol b/contracts/utils/contracts/test/TestLibAddress.sol new file mode 100644 index 0000000000..642ae2b99e --- /dev/null +++ b/contracts/utils/contracts/test/TestLibAddress.sol @@ -0,0 +1,35 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "../src/LibAddress.sol"; + + +contract TestLibAddress { + + using LibAddress for address; + + function externalIsContract(address account) + external + view + returns (bool) + { + return account.isContract(); + } +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index b2a95da17a..3ce247b47f 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddressArray|TestLibBytes|TestOwnable|TestReentrancyGuard|TestSafeMath).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestOwnable|TestReentrancyGuard|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index 2f171de7a5..f04bad4ed6 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -14,6 +14,7 @@ import * as Ownable from '../generated-artifacts/Ownable.json'; import * as ReentrancyGuard from '../generated-artifacts/ReentrancyGuard.json'; import * as SafeMath from '../generated-artifacts/SafeMath.json'; import * as TestConstants from '../generated-artifacts/TestConstants.json'; +import * as TestLibAddress from '../generated-artifacts/TestLibAddress.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; import * as TestOwnable from '../generated-artifacts/TestOwnable.json'; @@ -29,6 +30,7 @@ export const artifacts = { SafeMath: SafeMath as ContractArtifact, IOwnable: IOwnable as ContractArtifact, TestConstants: TestConstants as ContractArtifact, + TestLibAddress: TestLibAddress as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, TestOwnable: TestOwnable as ContractArtifact, diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index 8b4176ab7f..e2d4ef54cc 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -12,6 +12,7 @@ export * from '../generated-wrappers/ownable'; export * from '../generated-wrappers/reentrancy_guard'; export * from '../generated-wrappers/safe_math'; export * from '../generated-wrappers/test_constants'; +export * from '../generated-wrappers/test_lib_address'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; export * from '../generated-wrappers/test_ownable'; diff --git a/contracts/utils/test/lib_address.ts b/contracts/utils/test/lib_address.ts new file mode 100644 index 0000000000..f5d3cd9302 --- /dev/null +++ b/contracts/utils/test/lib_address.ts @@ -0,0 +1,41 @@ +import { chaiSetup, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import * as chai from 'chai'; + +import { artifacts, TestLibAddressContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +describe('LibAddress', () => { + let lib: TestLibAddressContract; + let nonContract: string; + + before(async () => { + await blockchainLifecycle.startAsync(); + nonContract = (await web3Wrapper.getAvailableAddressesAsync())[0]; + // Deploy LibAddress + lib = await TestLibAddressContract.deployFrom0xArtifactAsync( + artifacts.TestLibAddress, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('isContract', () => { + it('should return false for a non-contract address', async () => { + const isContract = await lib.externalIsContract.callAsync(nonContract); + expect(isContract).to.be.false(); + }); + + it('should return true for a non-contract address', async () => { + const isContract = await lib.externalIsContract.callAsync(lib.address); + expect(isContract).to.be.true(); + }); + }); +}); diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index 891fbe342f..7c02167004 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -12,6 +12,7 @@ "generated-artifacts/ReentrancyGuard.json", "generated-artifacts/SafeMath.json", "generated-artifacts/TestConstants.json", + "generated-artifacts/TestLibAddress.json", "generated-artifacts/TestLibAddressArray.json", "generated-artifacts/TestLibBytes.json", "generated-artifacts/TestOwnable.json", From f9292a8fb8110d4fee0effe2d7263a83c1a62f04 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 29 Jul 2019 18:26:55 -0700 Subject: [PATCH 05/17] Added unit tests for LibEIP712 --- contracts/test-utils/src/constants.ts | 1 + contracts/utils/compiler.json | 1 + contracts/utils/contracts/src/LibEIP712.sol | 2 +- .../utils/contracts/test/TestLibEIP712.sol | 52 +++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 2 + contracts/utils/src/wrappers.ts | 1 + contracts/utils/test/lib_eip712.ts | 107 ++++++++++++++++++ contracts/utils/tsconfig.json | 1 + packages/utils/src/sign_typed_data_utils.ts | 25 +++- 10 files changed, 191 insertions(+), 3 deletions(-) create mode 100644 contracts/utils/contracts/test/TestLibEIP712.sol create mode 100644 contracts/utils/test/lib_eip712.ts diff --git a/contracts/test-utils/src/constants.ts b/contracts/test-utils/src/constants.ts index 83ea993ed8..3ec8ff987d 100644 --- a/contracts/test-utils/src/constants.ts +++ b/contracts/test-utils/src/constants.ts @@ -42,6 +42,7 @@ export const constants = { NUM_ERC1155_FUNGIBLE_TOKENS_MINT: 4, NUM_ERC1155_NONFUNGIBLE_TOKENS_MINT: 4, NULL_ADDRESS: '0x0000000000000000000000000000000000000000', + NULL_BYTES32: '0x0000000000000000000000000000000000000000000000000000000000000000', UNLIMITED_ALLOWANCE_IN_BASE_UNITS: new BigNumber(2).pow(256).minus(1), TESTRPC_PRIVATE_KEYS: _.map(TESTRPC_PRIVATE_KEYS_STRINGS, privateKeyString => ethUtil.toBuffer(privateKeyString)), INITIAL_ERC20_BALANCE: Web3Wrapper.toBaseUnitAmount(new BigNumber(10000), 18), diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index 289cf93c27..a31630ca81 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -36,6 +36,7 @@ "test/TestLibAddress.sol", "test/TestLibAddressArray.sol", "test/TestLibBytes.sol", + "test/TestLibEIP712.sol", "test/TestOwnable.sol", "test/TestReentrancyGuard.sol", "test/TestSafeMath.sol" diff --git a/contracts/utils/contracts/src/LibEIP712.sol b/contracts/utils/contracts/src/LibEIP712.sol index 62a1f38dfb..32ca6ff117 100644 --- a/contracts/utils/contracts/src/LibEIP712.sol +++ b/contracts/utils/contracts/src/LibEIP712.sol @@ -1,6 +1,6 @@ /* - Copyright 2018 ZeroEx Intl. + Copyright 2019 ZeroEx Intl. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/contracts/utils/contracts/test/TestLibEIP712.sol b/contracts/utils/contracts/test/TestLibEIP712.sol new file mode 100644 index 0000000000..807651e5b3 --- /dev/null +++ b/contracts/utils/contracts/test/TestLibEIP712.sol @@ -0,0 +1,52 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "../src/LibEIP712.sol"; + + +contract TestLibEIP712 is + LibEIP712 +{ + function externalHashEIP712DomainSeperator( + string calldata name, + string calldata version, + uint256 chainid, + address verifyingcontractaddress + ) + external + pure + returns (bytes32) + { + return _hashEIP712Domain( + name, + version, + chainid, + verifyingcontractaddress + ); + } + + function externalHashEIP712Message(bytes32 eip712DomainHash, bytes32 hashStruct) + external + pure + returns (bytes32) + { + return _hashEIP712Message(eip712DomainHash, hashStruct); + } +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 3ce247b47f..f05094d2fa 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestOwnable|TestReentrancyGuard|TestSafeMath).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestLibEIP712|TestOwnable|TestReentrancyGuard|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index f04bad4ed6..b72eac333e 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -17,6 +17,7 @@ import * as TestConstants from '../generated-artifacts/TestConstants.json'; import * as TestLibAddress from '../generated-artifacts/TestLibAddress.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; +import * as TestLibEIP712 from '../generated-artifacts/TestLibEIP712.json'; import * as TestOwnable from '../generated-artifacts/TestOwnable.json'; import * as TestReentrancyGuard from '../generated-artifacts/TestReentrancyGuard.json'; import * as TestSafeMath from '../generated-artifacts/TestSafeMath.json'; @@ -33,6 +34,7 @@ export const artifacts = { TestLibAddress: TestLibAddress as ContractArtifact, TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, + TestLibEIP712: TestLibEIP712 as ContractArtifact, TestOwnable: TestOwnable as ContractArtifact, TestReentrancyGuard: TestReentrancyGuard as ContractArtifact, TestSafeMath: TestSafeMath as ContractArtifact, diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index e2d4ef54cc..5337a00e36 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -15,6 +15,7 @@ export * from '../generated-wrappers/test_constants'; export * from '../generated-wrappers/test_lib_address'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; +export * from '../generated-wrappers/test_lib_e_i_p712'; export * from '../generated-wrappers/test_ownable'; export * from '../generated-wrappers/test_reentrancy_guard'; export * from '../generated-wrappers/test_safe_math'; diff --git a/contracts/utils/test/lib_eip712.ts b/contracts/utils/test/lib_eip712.ts new file mode 100644 index 0000000000..8aac0495b7 --- /dev/null +++ b/contracts/utils/test/lib_eip712.ts @@ -0,0 +1,107 @@ +import { chaiSetup, constants, hexConcat, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import { BigNumber, signTypedDataUtils } from '@0x/utils'; +import * as chai from 'chai'; +import * as ethUtil from 'ethereumjs-util'; +import * as _ from 'lodash'; + +import { artifacts, TestLibEIP712Contract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +/** + * Tests a specific instance of EIP712 domain hashing. + * @param lib The LibEIP712 contract to call. + * @param name The name of the domain. + * @param version The version of the domain. + * @param chainId The chain id of the domain. + * @param verifyingContractAddress The verifying contract address of the domain. + */ +async function testHashEIP712DomainAsync( + lib: TestLibEIP712Contract, + name: string, + version: string, + chainId: number, + verifyingContractAddress: string, +): Promise { + const expectedHash = signTypedDataUtils.generateDomainHash({ + name, + version, + chainId, + verifyingContractAddress, + }); + const actualHash = await lib.externalHashEIP712DomainSeperator.callAsync( + name, + version, + new BigNumber(chainId), + verifyingContractAddress, + ); + expect(actualHash).to.be.eq(hexConcat(expectedHash)); +} + +/** + * Tests a specific instance of EIP712 message hashing. + * @param lib The LibEIP712 contract to call. + * @param domainHash The hash of the EIP712 domain of this instance. + * @param hashStruct The hash of the struct of this instance. + */ +async function testHashEIP712MessageAsync( + lib: TestLibEIP712Contract, + domainHash: string, + hashStruct: string, +): Promise { + const input = '0x1901'.concat(domainHash.slice(2, domainHash.length).concat(hashStruct.slice(2, hashStruct.length))); + const expectedHash = '0x'.concat(ethUtil.sha3(input).toString('hex')); + const actualHash = await lib.externalHashEIP712Message.callAsync( + domainHash, + hashStruct, + ); + expect(actualHash).to.be.eq(expectedHash); +} + +describe('LibEIP712', () => { + let lib: TestLibEIP712Contract; + + before(async () => { + await blockchainLifecycle.startAsync(); + // Deploy SafeMath + lib = await TestLibEIP712Contract.deployFrom0xArtifactAsync( + artifacts.TestLibEIP712, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('_hashEIP712Domain', async () => { + it('should correctly hash empty input', async () => { + await testHashEIP712DomainAsync( + lib, + '', + '', + 0, + constants.NULL_ADDRESS, + ); + }); + }); + + 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/tsconfig.json b/contracts/utils/tsconfig.json index 7c02167004..d63200ed62 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -15,6 +15,7 @@ "generated-artifacts/TestLibAddress.json", "generated-artifacts/TestLibAddressArray.json", "generated-artifacts/TestLibBytes.json", + "generated-artifacts/TestLibEIP712.json", "generated-artifacts/TestOwnable.json", "generated-artifacts/TestReentrancyGuard.json", "generated-artifacts/TestSafeMath.json" diff --git a/packages/utils/src/sign_typed_data_utils.ts b/packages/utils/src/sign_typed_data_utils.ts index 001509c899..824d8f8cc8 100644 --- a/packages/utils/src/sign_typed_data_utils.ts +++ b/packages/utils/src/sign_typed_data_utils.ts @@ -1,10 +1,11 @@ -import { EIP712Object, EIP712ObjectValue, EIP712TypedData, EIP712Types } from '@0x/types'; +import { EIP712DomainWithDefaultSchema, EIP712Object, EIP712ObjectValue, EIP712TypedData, EIP712Types } from '@0x/types'; import * as ethUtil from 'ethereumjs-util'; import * as ethers from 'ethers'; import * as _ from 'lodash'; import { BigNumber } from './configured_bignumber'; + export const signTypedDataUtils = { /** * Generates the EIP712 Typed Data hash for signing @@ -20,6 +21,28 @@ export const signTypedDataUtils = { ]), ); }, + /** + * Generates the hash of a EIP712 Domain with the default schema + * @param domain An EIP712 domain with the default schema containing a name, version, chain id, + * and verifying address. + * @return A buffer that contains the hash of the domain. + */ + generateDomainHash(domain: EIP712Object): Buffer { + return signTypedDataUtils._structHash( + 'EIP712Domain', + domain, + // HACK(jalextowle): When we consolidate our testing packages into test-utils, we can use a constant + // to eliminate code duplication. At the moment, there isn't a good way to do that because of cyclic-dependencies. + { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContractAddress', type: 'address' }, + ] + } as EIP712Types, + ); + }, _findDependencies(primaryType: string, types: EIP712Types, found: string[] = []): string[] { if (found.includes(primaryType) || types[primaryType] === undefined) { return found; From 03fced81f50be7bef74237fd378418b651906437 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 29 Jul 2019 18:37:51 -0700 Subject: [PATCH 06/17] Added unit tests for LibRichErrors --- contracts/utils/compiler.json | 1 + .../contracts/test/TestLibRichErrors.sol | 32 +++++++++++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 2 ++ contracts/utils/src/wrappers.ts | 1 + contracts/utils/test/lib_rich_errors.ts | 34 +++++++++++++++++++ contracts/utils/tsconfig.json | 1 + 7 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 contracts/utils/contracts/test/TestLibRichErrors.sol create mode 100644 contracts/utils/test/lib_rich_errors.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index a31630ca81..c1de9dcb0e 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -37,6 +37,7 @@ "test/TestLibAddressArray.sol", "test/TestLibBytes.sol", "test/TestLibEIP712.sol", + "test/TestLibRichErrors.sol", "test/TestOwnable.sol", "test/TestReentrancyGuard.sol", "test/TestSafeMath.sol" diff --git a/contracts/utils/contracts/test/TestLibRichErrors.sol b/contracts/utils/contracts/test/TestLibRichErrors.sol new file mode 100644 index 0000000000..fe0909c18a --- /dev/null +++ b/contracts/utils/contracts/test/TestLibRichErrors.sol @@ -0,0 +1,32 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "../src/LibRichErrors.sol"; + + +contract TestLibRichErrors { + + function externalRRevert(bytes calldata errorData) + external + pure + { + LibRichErrors._rrevert(errorData); + } +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index f05094d2fa..55aa8618ab 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestLibEIP712|TestOwnable|TestReentrancyGuard|TestSafeMath).json", + "abis": "./generated-artifacts/@(IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestLibEIP712|TestLibRichErrors|TestOwnable|TestReentrancyGuard|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index b72eac333e..52d1002075 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -18,6 +18,7 @@ import * as TestLibAddress from '../generated-artifacts/TestLibAddress.json'; import * as TestLibAddressArray from '../generated-artifacts/TestLibAddressArray.json'; import * as TestLibBytes from '../generated-artifacts/TestLibBytes.json'; import * as TestLibEIP712 from '../generated-artifacts/TestLibEIP712.json'; +import * as TestLibRichErrors from '../generated-artifacts/TestLibRichErrors.json'; import * as TestOwnable from '../generated-artifacts/TestOwnable.json'; import * as TestReentrancyGuard from '../generated-artifacts/TestReentrancyGuard.json'; import * as TestSafeMath from '../generated-artifacts/TestSafeMath.json'; @@ -35,6 +36,7 @@ export const artifacts = { TestLibAddressArray: TestLibAddressArray as ContractArtifact, TestLibBytes: TestLibBytes as ContractArtifact, TestLibEIP712: TestLibEIP712 as ContractArtifact, + TestLibRichErrors: TestLibRichErrors as ContractArtifact, TestOwnable: TestOwnable as ContractArtifact, TestReentrancyGuard: TestReentrancyGuard as ContractArtifact, TestSafeMath: TestSafeMath as ContractArtifact, diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index 5337a00e36..3caa25b828 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -16,6 +16,7 @@ export * from '../generated-wrappers/test_lib_address'; export * from '../generated-wrappers/test_lib_address_array'; export * from '../generated-wrappers/test_lib_bytes'; export * from '../generated-wrappers/test_lib_e_i_p712'; +export * from '../generated-wrappers/test_lib_rich_errors'; export * from '../generated-wrappers/test_ownable'; export * from '../generated-wrappers/test_reentrancy_guard'; export * from '../generated-wrappers/test_safe_math'; diff --git a/contracts/utils/test/lib_rich_errors.ts b/contracts/utils/test/lib_rich_errors.ts new file mode 100644 index 0000000000..7178275162 --- /dev/null +++ b/contracts/utils/test/lib_rich_errors.ts @@ -0,0 +1,34 @@ +import { chaiSetup, constants, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; +import { BlockchainLifecycle } from '@0x/dev-utils'; +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { artifacts, TestLibRichErrorsContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +describe('LibEIP712', () => { + let lib: TestLibRichErrorsContract; + + before(async () => { + await blockchainLifecycle.startAsync(); + // Deploy SafeMath + lib = await TestLibRichErrorsContract.deployFrom0xArtifactAsync( + artifacts.TestLibRichErrors, + provider, + txDefaults, + ); + }); + + after(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('_rrevert', () => { + it('should correctly revert the extra bytes', async () => { + return expect(lib.externalRRevert.callAsync(constants.NULL_BYTES)).to.revertWith(constants.NULL_BYTES); + }); + }); +}); diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index d63200ed62..3e74470d85 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -16,6 +16,7 @@ "generated-artifacts/TestLibAddressArray.json", "generated-artifacts/TestLibBytes.json", "generated-artifacts/TestLibEIP712.json", + "generated-artifacts/TestLibRichErrors.json", "generated-artifacts/TestOwnable.json", "generated-artifacts/TestReentrancyGuard.json", "generated-artifacts/TestSafeMath.json" From b2ada13a2100ab01664295d04d618868721f58b7 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 09:49:57 -0700 Subject: [PATCH 07/17] Ran prettier --- contracts/utils/test/lib_address.ts | 6 +---- contracts/utils/test/lib_eip712.ts | 29 +++++---------------- contracts/utils/test/ownable.ts | 14 +++------- contracts/utils/test/safe_math.ts | 8 ++---- packages/utils/src/sign_typed_data_utils.ts | 11 +++++--- 5 files changed, 22 insertions(+), 46 deletions(-) diff --git a/contracts/utils/test/lib_address.ts b/contracts/utils/test/lib_address.ts index f5d3cd9302..a6a53c2252 100644 --- a/contracts/utils/test/lib_address.ts +++ b/contracts/utils/test/lib_address.ts @@ -16,11 +16,7 @@ describe('LibAddress', () => { await blockchainLifecycle.startAsync(); nonContract = (await web3Wrapper.getAvailableAddressesAsync())[0]; // Deploy LibAddress - lib = await TestLibAddressContract.deployFrom0xArtifactAsync( - artifacts.TestLibAddress, - provider, - txDefaults, - ); + lib = await TestLibAddressContract.deployFrom0xArtifactAsync(artifacts.TestLibAddress, provider, txDefaults); }); after(async () => { diff --git a/contracts/utils/test/lib_eip712.ts b/contracts/utils/test/lib_eip712.ts index 8aac0495b7..07bc5b9e96 100644 --- a/contracts/utils/test/lib_eip712.ts +++ b/contracts/utils/test/lib_eip712.ts @@ -52,12 +52,11 @@ async function testHashEIP712MessageAsync( domainHash: string, hashStruct: string, ): Promise { - const input = '0x1901'.concat(domainHash.slice(2, domainHash.length).concat(hashStruct.slice(2, hashStruct.length))); - const expectedHash = '0x'.concat(ethUtil.sha3(input).toString('hex')); - const actualHash = await lib.externalHashEIP712Message.callAsync( - domainHash, - hashStruct, + const input = '0x1901'.concat( + domainHash.slice(2, domainHash.length).concat(hashStruct.slice(2, hashStruct.length)), ); + const expectedHash = '0x'.concat(ethUtil.sha3(input).toString('hex')); + const actualHash = await lib.externalHashEIP712Message.callAsync(domainHash, hashStruct); expect(actualHash).to.be.eq(expectedHash); } @@ -67,11 +66,7 @@ describe('LibEIP712', () => { before(async () => { await blockchainLifecycle.startAsync(); // Deploy SafeMath - lib = await TestLibEIP712Contract.deployFrom0xArtifactAsync( - artifacts.TestLibEIP712, - provider, - txDefaults, - ); + lib = await TestLibEIP712Contract.deployFrom0xArtifactAsync(artifacts.TestLibEIP712, provider, txDefaults); }); after(async () => { @@ -80,13 +75,7 @@ describe('LibEIP712', () => { describe('_hashEIP712Domain', async () => { it('should correctly hash empty input', async () => { - await testHashEIP712DomainAsync( - lib, - '', - '', - 0, - constants.NULL_ADDRESS, - ); + await testHashEIP712DomainAsync(lib, '', '', 0, constants.NULL_ADDRESS); }); }); @@ -97,11 +86,7 @@ describe('LibEIP712', () => { 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, - ); + await testHashEIP712MessageAsync(lib, constants.NULL_BYTES32, constants.NULL_BYTES32); }); }); }); diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index 451f9632bc..8b7cafa754 100644 --- a/contracts/utils/test/ownable.ts +++ b/contracts/utils/test/ownable.ts @@ -22,11 +22,7 @@ describe('Ownable', () => { await blockchainLifecycle.startAsync(); // Deploy SafeMath from the owner address txDefaults.from = owner; - ownable = await TestOwnableContract.deployFrom0xArtifactAsync( - artifacts.TestOwnable, - provider, - txDefaults, - ); + ownable = await TestOwnableContract.deployFrom0xArtifactAsync(artifacts.TestOwnable, provider, txDefaults); }); after(async () => { @@ -36,10 +32,7 @@ describe('Ownable', () => { // tslint:disable:no-unused-expression describe('onlyOwner', () => { it('should throw if sender is not owner', async () => { - const expectedError = new OwnableRevertErrors.OnlyOwnerError( - nonOwner, - owner, - ); + const expectedError = new OwnableRevertErrors.OnlyOwnerError(nonOwner, owner); return expect(ownable.externalOnlyOwner.callAsync({ from: nonOwner })).to.revertWith(expectedError); }); @@ -50,7 +43,8 @@ describe('Ownable', () => { describe('transferOwnership', () => { it('should not transfer ownership if the specified new owner is the zero address', async () => { - 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); }); diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts index ccbf0117e3..b5ae65b737 100644 --- a/contracts/utils/test/safe_math.ts +++ b/contracts/utils/test/safe_math.ts @@ -20,11 +20,7 @@ describe('SafeMath', () => { before(async () => { await blockchainLifecycle.startAsync(); // Deploy SafeMath - safeMath = await TestSafeMathContract.deployFrom0xArtifactAsync( - artifacts.TestSafeMath, - provider, - txDefaults, - ); + safeMath = await TestSafeMathContract.deployFrom0xArtifactAsync(artifacts.TestSafeMath, provider, txDefaults); }); after(async () => { @@ -53,7 +49,7 @@ describe('SafeMath', () => { return expect(safeMath.externalSafeMul.callAsync(a, b)).to.revertWith(expectedError); }); - it('should calculate correct value for values that don\'t overflow', async () => { + it("should calculate correct value for values that don't overflow", async () => { const result = await safeMath.externalSafeMul.callAsync(toBN(15), toBN(13)); expect(result).bignumber.to.be.eq(toBN(195)); }); diff --git a/packages/utils/src/sign_typed_data_utils.ts b/packages/utils/src/sign_typed_data_utils.ts index 824d8f8cc8..f923ec640d 100644 --- a/packages/utils/src/sign_typed_data_utils.ts +++ b/packages/utils/src/sign_typed_data_utils.ts @@ -1,11 +1,16 @@ -import { EIP712DomainWithDefaultSchema, EIP712Object, EIP712ObjectValue, EIP712TypedData, EIP712Types } from '@0x/types'; +import { + EIP712DomainWithDefaultSchema, + EIP712Object, + EIP712ObjectValue, + EIP712TypedData, + EIP712Types, +} from '@0x/types'; import * as ethUtil from 'ethereumjs-util'; import * as ethers from 'ethers'; import * as _ from 'lodash'; import { BigNumber } from './configured_bignumber'; - export const signTypedDataUtils = { /** * Generates the EIP712 Typed Data hash for signing @@ -39,7 +44,7 @@ export const signTypedDataUtils = { { name: 'version', type: 'string' }, { name: 'chainId', type: 'uint256' }, { name: 'verifyingContractAddress', type: 'address' }, - ] + ], } as EIP712Types, ); }, From 77b4f32274b2e4641bdc0a0449d068713340a16d Mon Sep 17 00:00:00 2001 From: James Towle Date: Tue, 30 Jul 2019 09:51:01 -0700 Subject: [PATCH 08/17] Update contracts/utils/test/ownable.ts Co-Authored-By: Lawrence Forman --- contracts/utils/test/ownable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index 8b7cafa754..2d8d586c70 100644 --- a/contracts/utils/test/ownable.ts +++ b/contracts/utils/test/ownable.ts @@ -37,7 +37,7 @@ describe('Ownable', () => { }); it('should succeed if sender is owner', async () => { - expect(ownable.externalOnlyOwner.callAsync({ from: owner })).to.be.fulfilled; + return expect(ownable.externalOnlyOwner.callAsync({ from: owner })).to.be.fulfilled(''); }); }); From bf1ebe8e53c90a8ae88d7300d12496ec9b7b53e5 Mon Sep 17 00:00:00 2001 From: James Towle Date: Tue, 30 Jul 2019 09:51:12 -0700 Subject: [PATCH 09/17] Update contracts/utils/test/ownable.ts Co-Authored-By: Lawrence Forman --- contracts/utils/test/ownable.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index 2d8d586c70..aba6ccb9fc 100644 --- a/contracts/utils/test/ownable.ts +++ b/contracts/utils/test/ownable.ts @@ -43,8 +43,7 @@ describe('Ownable', () => { describe('transferOwnership', () => { it('should not transfer ownership if the specified new owner is the zero address', async () => { - expect(ownable.transferOwnership.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner })).to.be - .fulfilled; + await expect(ownable.transferOwnership.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner })).to.be.fulfilled(''); const updatedOwner = await ownable.owner.callAsync(); expect(updatedOwner).to.be.eq(owner); }); From 4d39892a11ed08d98e996fd286aa446ab0482639 Mon Sep 17 00:00:00 2001 From: James Towle Date: Tue, 30 Jul 2019 09:51:23 -0700 Subject: [PATCH 10/17] Update contracts/utils/test/ownable.ts Co-Authored-By: Lawrence Forman --- contracts/utils/test/ownable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index aba6ccb9fc..62d2a3443e 100644 --- a/contracts/utils/test/ownable.ts +++ b/contracts/utils/test/ownable.ts @@ -49,7 +49,7 @@ describe('Ownable', () => { }); it('should transfer ownership if the specified new owner is not the zero address', async () => { - expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled; + await expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled(''); const updatedOwner = await ownable.owner.callAsync(); expect(updatedOwner).to.be.eq(nonOwner); }); From f044f364cb095682762ab6b3e998bda86841e24a Mon Sep 17 00:00:00 2001 From: James Towle Date: Tue, 30 Jul 2019 09:51:31 -0700 Subject: [PATCH 11/17] Update contracts/utils/test/reentrancy_guard.ts Co-Authored-By: Lawrence Forman --- contracts/utils/test/reentrancy_guard.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/test/reentrancy_guard.ts b/contracts/utils/test/reentrancy_guard.ts index 304c755a8b..d7f224d74c 100644 --- a/contracts/utils/test/reentrancy_guard.ts +++ b/contracts/utils/test/reentrancy_guard.ts @@ -34,7 +34,7 @@ describe('ReentrancyGuard', () => { }); it('should succeed if reentrancy does not occur', async () => { - expect(guard.guarded.sendTransactionAsync(false)).to.be.fulfilled; // tslint:disable-line:no-unused-expression + return expect(guard.guarded.sendTransactionAsync(false)).to.be.fulfilled(''); }); }); }); From 8cf4fb9adcad6370cd59f031bc716dfc2ad7d845 Mon Sep 17 00:00:00 2001 From: James Towle Date: Tue, 30 Jul 2019 09:51:46 -0700 Subject: [PATCH 12/17] Update contracts/utils/test/safe_math.ts Co-Authored-By: Lawrence Forman --- contracts/utils/test/safe_math.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts index b5ae65b737..e9159b6b01 100644 --- a/contracts/utils/test/safe_math.ts +++ b/contracts/utils/test/safe_math.ts @@ -134,7 +134,7 @@ describe('SafeMath', () => { expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); - it('should return second argument if it is greater than the first', async () => { + it('should return second argument if it is less than the first', async () => { const result = await safeMath.externalMaxUint256.callAsync(toBN(13), constants.ZERO_AMOUNT); expect(result).bignumber.to.be.eq(toBN(13)); }); From 3ca3a2820dd4a061cae78ae0977ba7e198f09aac Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 12:04:03 -0700 Subject: [PATCH 13/17] 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[] { From e5b6921de9ec91b817a1d39d8b2e2235db4abcc4 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 30 Jul 2019 15:56:31 -0700 Subject: [PATCH 14/17] Updated Changelog --- contracts/utils/CHANGELOG.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/utils/CHANGELOG.json b/contracts/utils/CHANGELOG.json index 1f8c882eb8..b457e5c0f3 100644 --- a/contracts/utils/CHANGELOG.json +++ b/contracts/utils/CHANGELOG.json @@ -25,6 +25,10 @@ { "note": "Updated RichErrors to the library pattern, and implemented RichErrors for all remaining reverts and requires", "pr": 1913 + }, + { + "note": "Added unit tests for all of the internal functions in the package", + "pr": 2014 } ] }, From 77feaec44440a88895a136bf578254fdc2d8206e Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Wed, 31 Jul 2019 13:09:41 -0700 Subject: [PATCH 15/17] Fixed lingering review comments --- contracts/utils/test/lib_eip712.ts | 26 ++++++++++++++++++++++++- contracts/utils/test/lib_rich_errors.ts | 2 +- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/contracts/utils/test/lib_eip712.ts b/contracts/utils/test/lib_eip712.ts index 479627b688..a2a0b7218a 100644 --- a/contracts/utils/test/lib_eip712.ts +++ b/contracts/utils/test/lib_eip712.ts @@ -65,7 +65,7 @@ describe('LibEIP712', () => { before(async () => { await blockchainLifecycle.startAsync(); - // Deploy SafeMath + // Deploy LibEIP712 lib = await TestLibEIP712Contract.deployFrom0xArtifactAsync(artifacts.TestLibEIP712, provider, txDefaults); }); @@ -77,11 +77,35 @@ describe('LibEIP712', () => { it('should correctly hash empty input', async () => { await testHashEIP712DomainAsync(lib, '', '', 0, constants.NULL_ADDRESS); }); + + it('should correctly hash non-empty input', async () => { + await testHashEIP712DomainAsync(lib, '_hashEIP712Domain', '1.0', 62, lib.address); + }); + + it('should correctly hash non-empty input', async () => { + await testHashEIP712DomainAsync(lib, '_hashEIP712Domain', '2.0', 0, lib.address); + }); }); describe('_hashEIP712Message', () => { it('should correctly hash empty input', async () => { await testHashEIP712MessageAsync(lib, constants.NULL_BYTES32, constants.NULL_BYTES32); }); + + it('should correctly hash non-empty input', async () => { + await testHashEIP712MessageAsync( + lib, + '0xb10e2d527612073b26eecdfd717e6a320cf44b4afac2b0732d9fcbe2b7fa0cf6', // keccak256(abi.encode(1)) + '0x405787fa12a823e0f2b7631cc41b3ba8828b3321ca811111fa75cd3aa3bb5ace', // keccak256(abi.encode(2)) + ); + }); + + it('should correctly hash non-empty input', async () => { + await testHashEIP712MessageAsync( + lib, + '0x405787fa12a823e0f2b7631cc41b3ba8828b3321ca811111fa75cd3aa3bb5ace', // keccak256(abi.encode(2)) + '0xc2575a0e9e593c00f959f8c92f12db2869c3395a3b0502d05e2516446f71f85b', // keccak256(abi.encode(3)) + ); + }); }); }); diff --git a/contracts/utils/test/lib_rich_errors.ts b/contracts/utils/test/lib_rich_errors.ts index 1245cf0cd2..df3e60b910 100644 --- a/contracts/utils/test/lib_rich_errors.ts +++ b/contracts/utils/test/lib_rich_errors.ts @@ -10,7 +10,7 @@ chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); -describe('LibEIP712', () => { +describe('LibRichErrors', () => { let lib: TestLibRichErrorsContract; before(async () => { From 8c5c81fe705874376552a9883934fbe90a533236 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 1 Aug 2019 15:10:06 -0700 Subject: [PATCH 16/17] Change all instances of throw to revert in it tests --- contracts/asset-proxy/test/authorizable.ts | 16 ++++++------ contracts/erc1155/test/erc1155_token.ts | 12 ++++----- .../erc20/test/unlimited_allowance_token.ts | 6 ++--- contracts/erc20/test/weth9.ts | 4 +-- contracts/exchange/test/core.ts | 26 +++++++++---------- contracts/exchange/test/dispatcher.ts | 6 ++--- contracts/exchange/test/fill_order.ts | 26 +++++++++---------- contracts/exchange/test/match_orders.ts | 20 +++++++------- .../exchange/test/signature_validator.ts | 4 +-- contracts/exchange/test/wrapper.ts | 12 ++++----- contracts/multisig/test/asset_proxy_owner.ts | 12 ++++----- .../multisig/test/multi_sig_with_time_lock.ts | 6 ++--- contracts/utils/test/ownable.ts | 2 +- contracts/utils/test/reentrancy_guard.ts | 2 +- contracts/utils/test/safe_math.ts | 6 ++--- 15 files changed, 80 insertions(+), 80 deletions(-) diff --git a/contracts/asset-proxy/test/authorizable.ts b/contracts/asset-proxy/test/authorizable.ts index ee2eff5884..57cd4abf73 100644 --- a/contracts/asset-proxy/test/authorizable.ts +++ b/contracts/asset-proxy/test/authorizable.ts @@ -46,7 +46,7 @@ describe('Authorizable', () => { await blockchainLifecycle.revertAsync(); }); describe('addAuthorizedAddress', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); const tx = authorizable.addAuthorizedAddress.sendTransactionAsync(notOwner, { from: notOwner }); return expect(tx).to.revertWith(expectedError); @@ -60,7 +60,7 @@ describe('Authorizable', () => { const isAuthorized = await authorizable.authorized.callAsync(address); expect(isAuthorized).to.be.true(); }); - it('should throw if owner attempts to authorize a duplicate address', async () => { + it('should revert if owner attempts to authorize a duplicate address', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -74,7 +74,7 @@ describe('Authorizable', () => { }); describe('removeAuthorizedAddress', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -100,7 +100,7 @@ describe('Authorizable', () => { expect(isAuthorized).to.be.false(); }); - it('should throw if owner attempts to remove an address that is not authorized', async () => { + it('should revert if owner attempts to remove an address that is not authorized', async () => { return expectTransactionFailedAsync( authorizable.removeAuthorizedAddress.sendTransactionAsync(address, { from: owner, @@ -111,7 +111,7 @@ describe('Authorizable', () => { }); describe('removeAuthorizedAddressAtIndex', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -124,7 +124,7 @@ describe('Authorizable', () => { }); return expect(tx).to.revertWith(expectedError); }); - it('should throw if index is >= authorities.length', async () => { + it('should revert if index is >= authorities.length', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -138,7 +138,7 @@ describe('Authorizable', () => { RevertReason.IndexOutOfBounds, ); }); - it('should throw if owner attempts to remove an address that is not authorized', async () => { + it('should revert if owner attempts to remove an address that is not authorized', async () => { const index = new BigNumber(0); return expectTransactionFailedAsync( authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { @@ -147,7 +147,7 @@ describe('Authorizable', () => { RevertReason.TargetNotAuthorized, ); }); - it('should throw if address at index does not match target', async () => { + it('should revert if address at index does not match target', async () => { const address1 = address; const address2 = notOwner; await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( diff --git a/contracts/erc1155/test/erc1155_token.ts b/contracts/erc1155/test/erc1155_token.ts index aa57e590b5..29315c065d 100644 --- a/contracts/erc1155/test/erc1155_token.ts +++ b/contracts/erc1155/test/erc1155_token.ts @@ -166,7 +166,7 @@ describe('ERC1155Token', () => { ]; await erc1155Wrapper.assertBalancesAsync(tokenHolders, [tokenToTransfer], expectedFinalBalances); }); - it('should throw if transfer reverts', async () => { + it('should revert if transfer reverts', async () => { // setup test parameters const tokenToTransfer = fungibleToken; const valueToTransfer = spenderInitialFungibleBalance.plus(1); @@ -187,7 +187,7 @@ describe('ERC1155Token', () => { ); return expect(tx).to.revertWith(expectedError); }); - it('should throw if callback reverts', async () => { + it('should revert if callback reverts', async () => { // setup test parameters const tokenToTransfer = fungibleToken; const valueToTransfer = fungibleValueToTransfer; @@ -342,7 +342,7 @@ describe('ERC1155Token', () => { ]; await erc1155Wrapper.assertBalancesAsync(tokenHolders, tokensToTransfer, expectedFinalBalances); }); - it('should throw if transfer reverts', async () => { + it('should revert if transfer reverts', async () => { // setup test parameters const tokensToTransfer = [fungibleToken]; const valuesToTransfer = [spenderInitialFungibleBalance.plus(1)]; @@ -363,7 +363,7 @@ describe('ERC1155Token', () => { ); return expect(tx).to.revertWith(expectedError); }); - it('should throw if callback reverts', async () => { + it('should revert if callback reverts', async () => { // setup test parameters const tokensToTransfer = [fungibleToken]; const valuesToTransfer = [fungibleValueToTransfer]; @@ -417,7 +417,7 @@ describe('ERC1155Token', () => { ]; await erc1155Wrapper.assertBalancesAsync(tokenHolders, [tokenToTransfer], expectedFinalBalances); }); - it('should throw if trying to transfer tokens via safeTransferFrom by an unapproved account', async () => { + it('should revert if trying to transfer tokens via safeTransferFrom by an unapproved account', async () => { // check approval not set const isApprovedForAllCheck = await erc1155Wrapper.isApprovedForAllAsync(spender, delegatedSpender); expect(isApprovedForAllCheck).to.be.false(); @@ -470,7 +470,7 @@ describe('ERC1155Token', () => { ]; await erc1155Wrapper.assertBalancesAsync(tokenHolders, tokensToTransfer, expectedFinalBalances); }); - it('should throw if trying to transfer tokens via safeBatchTransferFrom by an unapproved account', async () => { + it('should revert if trying to transfer tokens via safeBatchTransferFrom by an unapproved account', async () => { // check approval not set const isApprovedForAllCheck = await erc1155Wrapper.isApprovedForAllAsync(spender, delegatedSpender); expect(isApprovedForAllCheck).to.be.false(); diff --git a/contracts/erc20/test/unlimited_allowance_token.ts b/contracts/erc20/test/unlimited_allowance_token.ts index f0b8e53a46..e31507d7ab 100644 --- a/contracts/erc20/test/unlimited_allowance_token.ts +++ b/contracts/erc20/test/unlimited_allowance_token.ts @@ -54,7 +54,7 @@ describe('UnlimitedAllowanceToken', () => { await blockchainLifecycle.revertAsync(); }); describe('transfer', () => { - it('should throw if owner has insufficient balance', async () => { + it('should revert if owner has insufficient balance', async () => { const ownerBalance = await token.balanceOf.callAsync(owner); const amountToTransfer = ownerBalance.plus(1); return expectContractCallFailedAsync( @@ -89,7 +89,7 @@ describe('UnlimitedAllowanceToken', () => { }); describe('transferFrom', () => { - it('should throw if owner has insufficient balance', async () => { + it('should revert if owner has insufficient balance', async () => { const ownerBalance = await token.balanceOf.callAsync(owner); const amountToTransfer = ownerBalance.plus(1); await web3Wrapper.awaitTransactionSuccessAsync( @@ -104,7 +104,7 @@ describe('UnlimitedAllowanceToken', () => { ); }); - it('should throw if spender has insufficient allowance', async () => { + it('should revert if spender has insufficient allowance', async () => { const ownerBalance = await token.balanceOf.callAsync(owner); const amountToTransfer = ownerBalance; diff --git a/contracts/erc20/test/weth9.ts b/contracts/erc20/test/weth9.ts index 6a3948e2c3..444a8c3ef6 100644 --- a/contracts/erc20/test/weth9.ts +++ b/contracts/erc20/test/weth9.ts @@ -45,7 +45,7 @@ describe('EtherToken', () => { await blockchainLifecycle.revertAsync(); }); describe('deposit', () => { - it('should throw if caller attempts to deposit more Ether than caller balance', async () => { + it('should revert if caller attempts to deposit more Ether than caller balance', async () => { const initEthBalance = await web3Wrapper.getBalanceInWeiAsync(account); const ethToDeposit = initEthBalance.plus(1); @@ -74,7 +74,7 @@ describe('EtherToken', () => { }); describe('withdraw', () => { - it('should throw if caller attempts to withdraw greater than caller balance', async () => { + it('should revert if caller attempts to withdraw greater than caller balance', async () => { const initEthTokenBalance = await etherToken.balanceOf.callAsync(account); const ethTokensToWithdraw = initEthTokenBalance.plus(1); diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index fbcdda23e5..2d5069592e 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -380,7 +380,7 @@ describe('Exchange core', () => { }); }); - it('should throw if fully filled', async () => { + it('should revert if fully filled', async () => { signedOrder = await orderFactory.newSignedOrderAsync(); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); @@ -423,7 +423,7 @@ describe('Exchange core', () => { ).to.be.bignumber.equal(signedOrder.takerFee); }); - it('should throw if order is expired', async () => { + it('should revert if order is expired', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); signedOrder = await orderFactory.newSignedOrderAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), @@ -742,14 +742,14 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync(); }); - it('should throw if not sent by maker', async () => { + it('should revert if not sent by maker', async () => { const orderHash = orderHashUtils.getOrderHashHex(signedOrder); const expectedError = new ExchangeRevertErrors.InvalidMakerError(orderHash, takerAddress); const tx = exchangeWrapper.cancelOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(expectedError); }); - it('should throw if makerAssetAmount is 0', async () => { + it('should revert if makerAssetAmount is 0', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: new BigNumber(0), }); @@ -762,7 +762,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if takerAssetAmount is 0', async () => { + it('should revert if takerAssetAmount is 0', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ takerAssetAmount: new BigNumber(0), }); @@ -800,7 +800,7 @@ describe('Exchange core', () => { expect(orderHashUtils.getOrderHashHex(signedOrder)).to.be.equal(logArgs.orderHash); }); - it('should throw if already cancelled', async () => { + it('should revert if already cancelled', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); const orderHash = orderHashUtils.getOrderHashHex(signedOrder); const expectedError = new ExchangeRevertErrors.OrderStatusError(orderHash, OrderStatus.Cancelled); @@ -808,7 +808,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if order is expired', async () => { + it('should revert if order is expired', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); signedOrder = await orderFactory.newSignedOrderAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), @@ -819,7 +819,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if rounding error is greater than 0.1%', async () => { + it('should revert if rounding error is greater than 0.1%', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: new BigNumber(1001), takerAssetAmount: new BigNumber(3), @@ -936,7 +936,7 @@ describe('Exchange core', () => { }); describe('Testing Exchange of ERC721 Tokens', () => { - it('should throw when maker does not own the token with id makerAssetId', async () => { + it('should revert when maker does not own the token with id makerAssetId', async () => { // Construct Exchange parameters const makerAssetId = erc721TakerAssetIds[0]; const takerAssetId = erc721TakerAssetIds[1]; @@ -963,7 +963,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw when taker does not own the token with id takerAssetId', async () => { + it('should revert when taker does not own the token with id takerAssetId', async () => { // Construct Exchange parameters const makerAssetId = erc721MakerAssetIds[0]; const takerAssetId = erc721MakerAssetIds[1]; @@ -990,7 +990,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw when makerAssetAmount is greater than 1', async () => { + it('should revert when makerAssetAmount is greater than 1', async () => { // Construct Exchange parameters const makerAssetId = erc721MakerAssetIds[0]; const takerAssetId = erc721TakerAssetIds[0]; @@ -1017,7 +1017,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw when takerAssetAmount is greater than 1', async () => { + it('should revert when takerAssetAmount is greater than 1', async () => { // Construct Exchange parameters const makerAssetId = erc721MakerAssetIds[0]; const takerAssetId = erc721TakerAssetIds[0]; @@ -1044,7 +1044,7 @@ describe('Exchange core', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw on partial fill', async () => { + it('should revert on partial fill', async () => { // Construct Exchange parameters const makerAssetId = erc721MakerAssetIds[0]; signedOrder = await orderFactory.newSignedOrderAsync({ diff --git a/contracts/exchange/test/dispatcher.ts b/contracts/exchange/test/dispatcher.ts index befb88d601..c547d44f17 100644 --- a/contracts/exchange/test/dispatcher.ts +++ b/contracts/exchange/test/dispatcher.ts @@ -114,7 +114,7 @@ describe('AssetProxyDispatcher', () => { expect(proxyAddress).to.be.equal(erc721Proxy.address); }); - it('should throw if a proxy with the same id is already registered', async () => { + it('should revert if a proxy with the same id is already registered', async () => { // Initial registration await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, { from: owner, @@ -134,7 +134,7 @@ describe('AssetProxyDispatcher', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if requesting address is not owner', async () => { + it('should revert if requesting address is not owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); const tx = assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: notOwner, @@ -250,7 +250,7 @@ describe('AssetProxyDispatcher', () => { expect(newBalances).to.deep.equal(erc20Balances); }); - it('should throw if dispatching to unregistered proxy', async () => { + it('should revert if dispatching to unregistered proxy', async () => { // Construct metadata for ERC20 proxy const encodedAssetData = assetDataUtils.encodeERC20AssetData(erc20TokenA.address); // Perform a transfer from makerAddress to takerAddress diff --git a/contracts/exchange/test/fill_order.ts b/contracts/exchange/test/fill_order.ts index 8b119de8d9..9516699405 100644 --- a/contracts/exchange/test/fill_order.ts +++ b/contracts/exchange/test/fill_order.ts @@ -175,7 +175,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioSuccessAsync(fillScenario); }); - it('should throw when taker is specified and order is claimed by other', async () => { + it('should revert when taker is specified and order is claimed by other', async () => { const fillScenario = { ...defaultFillScenario, orderScenario: { @@ -186,7 +186,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if makerAssetAmount is 0', async () => { + it('should revert if makerAssetAmount is 0', async () => { const fillScenario = { ...defaultFillScenario, orderScenario: { @@ -198,7 +198,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if takerAssetAmount is 0', async () => { + it('should revert if takerAssetAmount is 0', async () => { const fillScenario = { ...defaultFillScenario, orderScenario: { @@ -210,7 +210,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if takerAssetFillAmount is 0', async () => { + it('should revert if takerAssetFillAmount is 0', async () => { const fillScenario = { ...defaultFillScenario, takerAssetFillAmountScenario: TakerAssetFillAmountScenario.Zero, @@ -218,7 +218,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if an order is expired', async () => { + it('should revert if an order is expired', async () => { const fillScenario = { ...defaultFillScenario, orderScenario: { @@ -331,7 +331,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioSuccessAsync(fillScenario); }); - it('should throw if maker balance is too low to fill order', async () => { + it('should revert if maker balance is too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, makerStateScenario: { @@ -342,7 +342,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if taker balance is too low to fill order', async () => { + it('should revert if taker balance is too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, takerStateScenario: { @@ -353,7 +353,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if maker allowances are too low to fill order', async () => { + it('should revert if maker allowances are too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, makerStateScenario: { @@ -364,7 +364,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if taker allowances are too low to fill order', async () => { + it('should revert if taker allowances are too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, takerStateScenario: { @@ -375,7 +375,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if maker fee balance is too low to fill order', async () => { + it('should revert if maker fee balance is too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, makerStateScenario: { @@ -386,7 +386,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if taker fee balance is too low to fill order', async () => { + it('should revert if taker fee balance is too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, takerStateScenario: { @@ -397,7 +397,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if maker fee allowances are too low to fill order', async () => { + it('should revert if maker fee allowances are too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, makerStateScenario: { @@ -408,7 +408,7 @@ describe('FillOrder Tests', () => { await fillOrderCombinatorialUtils.testFillOrderScenarioFailureAsync(fillScenario); }); - it('should throw if taker fee allowances are too low to fill order', async () => { + it('should revert if taker fee allowances are too low to fill order', async () => { const fillScenario = { ...defaultFillScenario, takerStateScenario: { diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index 1e200778ce..74edbd9acd 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -1286,7 +1286,7 @@ describe('matchOrders', () => { ); }); - it('Should throw if left order is not fillable', async () => { + it('Should revert if left order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -1305,7 +1305,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('Should throw if right order is not fillable', async () => { + it('Should revert if right order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -1324,7 +1324,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if there is not a positive spread', async () => { + it('should revert if there is not a positive spread', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -1342,7 +1342,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if the left maker asset is not equal to the right taker asset ', async () => { + it('should revert if the left maker asset is not equal to the right taker asset ', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -1373,7 +1373,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if the right maker asset is not equal to the left taker asset', async () => { + it('should revert if the right maker asset is not equal to the left taker asset', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), @@ -2416,7 +2416,7 @@ describe('matchOrders', () => { ); }); - it('Should throw if left order is not fillable', async () => { + it('Should revert if left order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -2435,7 +2435,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('Should throw if right order is not fillable', async () => { + it('Should revert if right order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -2454,7 +2454,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if there is not a positive spread', async () => { + it('should revert if there is not a positive spread', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -2472,7 +2472,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if the left maker asset is not equal to the right taker asset ', async () => { + it('should revert if the left maker asset is not equal to the right taker asset ', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), @@ -2503,7 +2503,7 @@ describe('matchOrders', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if the right maker asset is not equal to the left taker asset', async () => { + it('should revert if the right maker asset is not equal to the left taker asset', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 6bc75c06c0..ba5b84822d 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -495,7 +495,7 @@ describe('MixinSignatureValidator', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw when SignatureType=Validator, signature is valid and validator is not approved', async () => { + it('should revert when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false await signatureValidator.setSignatureValidatorApproval.awaitTransactionSuccessAsync( validatorWallet.address, @@ -702,7 +702,7 @@ describe('MixinSignatureValidator', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw when SignatureType=Validator, signature is valid and validator is not approved', async () => { + it('should revert when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false await signatureValidator.setSignatureValidatorApproval.awaitTransactionSuccessAsync( validatorWallet.address, diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index be30975aef..4d48862012 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -234,7 +234,7 @@ describe('Exchange wrappers', () => { ); }); - it('should throw if a signedOrder is expired', async () => { + it('should revert if a signedOrder is expired', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); const signedOrder = await orderFactory.newSignedOrderAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), @@ -245,7 +245,7 @@ describe('Exchange wrappers', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if entire takerAssetFillAmount not filled', async () => { + it('should revert if entire takerAssetFillAmount not filled', async () => { const signedOrder = await orderFactory.newSignedOrderAsync(); await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { @@ -815,7 +815,7 @@ describe('Exchange wrappers', () => { expect(newBalances).to.be.deep.equal(erc20Balances); }); - it('should throw if a single signedOrder does not fill the expected amount', async () => { + it('should revert if a single signedOrder does not fill the expected amount', async () => { const takerAssetFillAmounts: BigNumber[] = []; _.forEach(signedOrders, signedOrder => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); @@ -922,7 +922,7 @@ describe('Exchange wrappers', () => { expect(newBalances).to.be.deep.equal(erc20Balances); }); - it('should not throw if an order is invalid and fill the remaining orders', async () => { + it('should not revert if an order is invalid and fill the remaining orders', async () => { const makerAssetAddress = erc20TokenA.address; const takerAssetAddress = erc20TokenB.address; @@ -1132,7 +1132,7 @@ describe('Exchange wrappers', () => { expect(newBalances).to.be.deep.equal(erc20Balances); }); - it('should throw when a signedOrder does not use the same takerAssetAddress', async () => { + it('should revert when a signedOrder does not use the same takerAssetAddress', async () => { signedOrders = [ await orderFactory.newSignedOrderAsync(), await orderFactory.newSignedOrderAsync({ @@ -1510,7 +1510,7 @@ describe('Exchange wrappers', () => { expect(newBalances).to.be.deep.equal(erc20Balances); }); - it('should throw when a signedOrder does not use the same makerAssetAddress', async () => { + it('should revert when a signedOrder does not use the same makerAssetAddress', async () => { signedOrders = [ await orderFactory.newSignedOrderAsync(), await orderFactory.newSignedOrderAsync({ diff --git a/contracts/multisig/test/asset_proxy_owner.ts b/contracts/multisig/test/asset_proxy_owner.ts index 62081554be..729449e144 100644 --- a/contracts/multisig/test/asset_proxy_owner.ts +++ b/contracts/multisig/test/asset_proxy_owner.ts @@ -115,7 +115,7 @@ describe('AssetProxyOwner', () => { expect(isErc20ProxyRegistered).to.equal(true); expect(isErc721ProxyRegistered).to.equal(true); }); - it('should throw if a null address is included in assetProxyContracts', async () => { + it('should revert if a null address is included in assetProxyContracts', async () => { const assetProxyContractAddresses = [erc20Proxy.address, constants.NULL_ADDRESS]; return expectContractCreationFailedAsync( (AssetProxyOwnerContract.deployFrom0xArtifactAsync( @@ -158,7 +158,7 @@ describe('AssetProxyOwner', () => { }); describe('registerAssetProxy', () => { - it('should throw if not called by multisig', async () => { + it('should revert if not called by multisig', async () => { const isRegistered = true; return expectTransactionFailedWithoutReasonAsync( testAssetProxyOwner.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, isRegistered, { @@ -338,7 +338,7 @@ describe('AssetProxyOwner', () => { }); describe('executeRemoveAuthorizedAddressAtIndex', () => { - it('should throw without the required confirmations', async () => { + it('should revert without the required confirmations', async () => { const removeAuthorizedAddressAtIndexData = erc20Proxy.removeAuthorizedAddressAtIndex.getABIEncodedTransactionData( authorized, erc20Index, @@ -359,7 +359,7 @@ describe('AssetProxyOwner', () => { ); }); - it('should throw if tx destination is not registered', async () => { + it('should revert if tx destination is not registered', async () => { const removeAuthorizedAddressAtIndexData = erc721Proxy.removeAuthorizedAddressAtIndex.getABIEncodedTransactionData( authorized, erc721Index, @@ -382,7 +382,7 @@ describe('AssetProxyOwner', () => { ); }); - it('should throw if tx data is not for removeAuthorizedAddressAtIndex', async () => { + it('should revert if tx data is not for removeAuthorizedAddressAtIndex', async () => { const newAuthorized = owners[1]; const addAuthorizedAddressData = erc20Proxy.addAuthorizedAddress.getABIEncodedTransactionData( newAuthorized, @@ -468,7 +468,7 @@ describe('AssetProxyOwner', () => { expect(isAuthorizedAfter).to.equal(false); }); - it('should throw if already executed', async () => { + it('should revert if already executed', async () => { const removeAuthorizedAddressAtIndexData = erc20Proxy.removeAuthorizedAddressAtIndex.getABIEncodedTransactionData( authorized, erc20Index, diff --git a/contracts/multisig/test/multi_sig_with_time_lock.ts b/contracts/multisig/test/multi_sig_with_time_lock.ts index 9596271514..cd6c6b3571 100644 --- a/contracts/multisig/test/multi_sig_with_time_lock.ts +++ b/contracts/multisig/test/multi_sig_with_time_lock.ts @@ -241,13 +241,13 @@ describe('MultiSigWalletWithTimeLock', () => { multiSigWrapper = new MultiSigWrapper(multiSig, provider); }); - it('should throw when not called by wallet', async () => { + it('should revert when not called by wallet', async () => { return expectTransactionFailedWithoutReasonAsync( multiSig.changeTimeLock.sendTransactionAsync(SECONDS_TIME_LOCKED, { from: owners[0] }), ); }); - it('should throw without enough confirmations', async () => { + it('should revert without enough confirmations', async () => { const destination = multiSig.address; const changeTimeLockData = multiSig.changeTimeLock.getABIEncodedTransactionData(SECONDS_TIME_LOCKED); const res = await multiSigWrapper.submitTransactionAsync(destination, changeTimeLockData, owners[0]); @@ -325,7 +325,7 @@ describe('MultiSigWalletWithTimeLock', () => { await multiSigWrapper.confirmTransactionAsync(txId, owners[1]); }); - it('should throw if it has enough confirmations but is not past the time lock', async () => { + it('should revert if it has enough confirmations but is not past the time lock', async () => { return expectTransactionFailedAsync( multiSig.executeTransaction.sendTransactionAsync(txId, { from: owners[0] }), RevertReason.TimeLockIncomplete, diff --git a/contracts/utils/test/ownable.ts b/contracts/utils/test/ownable.ts index 6fc908e13a..e204a48d3e 100644 --- a/contracts/utils/test/ownable.ts +++ b/contracts/utils/test/ownable.ts @@ -30,7 +30,7 @@ describe('Ownable', () => { }); describe('onlyOwner', () => { - it('should throw if sender is not the owner', async () => { + it('should revert if sender is not the owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(nonOwner, owner); return expect(ownable.externalOnlyOwner.callAsync({ from: nonOwner })).to.revertWith(expectedError); }); diff --git a/contracts/utils/test/reentrancy_guard.ts b/contracts/utils/test/reentrancy_guard.ts index e2bf9c38f0..270c9eab84 100644 --- a/contracts/utils/test/reentrancy_guard.ts +++ b/contracts/utils/test/reentrancy_guard.ts @@ -28,7 +28,7 @@ describe('ReentrancyGuard', () => { }); describe('nonReentrant', () => { - it('should throw if reentrancy occurs', async () => { + it('should revert if reentrancy occurs', async () => { const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); return expect(guard.guarded.sendTransactionAsync(true)).to.revertWith(expectedError); }); diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts index aa4f1f35bc..d273767434 100644 --- a/contracts/utils/test/safe_math.ts +++ b/contracts/utils/test/safe_math.ts @@ -76,7 +76,7 @@ describe('SafeMath', () => { expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); - it('should return zero if second argument is zero', async () => { + it('should revert 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, @@ -85,7 +85,7 @@ describe('SafeMath', () => { }); describe('_safeSub', () => { - it('should throw if the subtraction underflows', async () => { + it('should revert if the subtraction underflows', async () => { const a = toBN(0); const b = toBN(1); const expectedError = new SafeMathRevertErrors.SafeMathError( @@ -108,7 +108,7 @@ describe('SafeMath', () => { }); describe('_safeAdd', () => { - it('should throw if the addition overflows', async () => { + it('should revert if the addition overflows', async () => { const a = toBN('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number const b = toBN(1); const expectedError = new SafeMathRevertErrors.SafeMathError( From e1796a9f0fb4b4ef3530d7d751699f246d191ef7 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 1 Aug 2019 15:39:00 -0700 Subject: [PATCH 17/17] Changed toBN to toBigNumber --- contracts/utils/test/safe_math.ts | 68 +++++++++++++++---------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/contracts/utils/test/safe_math.ts b/contracts/utils/test/safe_math.ts index d273767434..51ac43662b 100644 --- a/contracts/utils/test/safe_math.ts +++ b/contracts/utils/test/safe_math.ts @@ -10,7 +10,7 @@ chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); -function toBN(a: number | string): BigNumber { +function toBigNumber(a: number | string): BigNumber { return new BigNumber(a); } @@ -29,18 +29,18 @@ describe('SafeMath', () => { describe('_safeMul', () => { it('should return zero if first argument is zero', async () => { - const result = await safeMath.externalSafeMul.callAsync(constants.ZERO_AMOUNT, toBN(1)); + const result = await safeMath.externalSafeMul.callAsync(constants.ZERO_AMOUNT, toBigNumber(1)); expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); it('should return zero if second argument is zero', async () => { - const result = await safeMath.externalSafeMul.callAsync(toBN(1), constants.ZERO_AMOUNT); + const result = await safeMath.externalSafeMul.callAsync(toBigNumber(1), constants.ZERO_AMOUNT); expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); it('should revert if the multiplication overflows', async () => { - const a = toBN('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number - const b = toBN(2); + const a = toBigNumber('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number + const b = toBigNumber(2); const expectedError = new SafeMathRevertErrors.SafeMathError( SafeMathRevertErrors.SafeMathErrorCodes.Uint256MultiplicationOverflow, a, @@ -50,35 +50,35 @@ describe('SafeMath', () => { }); it("should calculate correct value for values that don't overflow", async () => { - const result = await safeMath.externalSafeMul.callAsync(toBN(15), toBN(13)); - expect(result).bignumber.to.be.eq(toBN(195)); + const result = await safeMath.externalSafeMul.callAsync(toBigNumber(15), toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(195)); }); }); 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)); + const result = await safeMath.externalSafeDiv.callAsync(toBigNumber(1), toBigNumber(1)); + expect(result).bignumber.to.be.eq(toBigNumber(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)); + const result = await safeMath.externalSafeDiv.callAsync(toBigNumber(3), toBigNumber(2)); + expect(result).bignumber.to.be.eq(toBigNumber(1)); }); it('should return zero if the numerator is smaller than the denominator', async () => { - const result = await safeMath.externalSafeDiv.callAsync(toBN(2), toBN(3)); + const result = await safeMath.externalSafeDiv.callAsync(toBigNumber(2), toBigNumber(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)); + const result = await safeMath.externalSafeDiv.callAsync(constants.ZERO_AMOUNT, toBigNumber(1)); expect(result).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); it('should revert 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( + return expect(safeMath.externalSafeDiv.callAsync(toBigNumber(1), constants.ZERO_AMOUNT)).to.be.rejectedWith( errMessage, ); }); @@ -86,8 +86,8 @@ describe('SafeMath', () => { describe('_safeSub', () => { it('should revert if the subtraction underflows', async () => { - const a = toBN(0); - const b = toBN(1); + const a = toBigNumber(0); + const b = toBigNumber(1); const expectedError = new SafeMathRevertErrors.SafeMathError( SafeMathRevertErrors.SafeMathErrorCodes.Uint256SubtractionUnderflow, a, @@ -102,15 +102,15 @@ describe('SafeMath', () => { }); it('should calculate correct value for values that are not equal', async () => { - const result = await safeMath.externalSafeSub.callAsync(toBN(15), toBN(13)); - expect(result).bignumber.to.be.eq(toBN(2)); + const result = await safeMath.externalSafeSub.callAsync(toBigNumber(15), toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(2)); }); }); describe('_safeAdd', () => { it('should revert if the addition overflows', async () => { - const a = toBN('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number - const b = toBN(1); + const a = toBigNumber('0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'); // The largest uint256 number + const b = toBigNumber(1); const expectedError = new SafeMathRevertErrors.SafeMathError( SafeMathRevertErrors.SafeMathErrorCodes.Uint256AdditionOverflow, a, @@ -120,25 +120,25 @@ describe('SafeMath', () => { }); it('should calculate correct value if addition does not overflow', async () => { - const result = await safeMath.externalSafeAdd.callAsync(toBN(15), toBN(13)); - expect(result).bignumber.to.be.eq(toBN(28)); + const result = await safeMath.externalSafeAdd.callAsync(toBigNumber(15), toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(28)); }); it('should calculate correct value if first argument is zero', async () => { - const result = await safeMath.externalSafeAdd.callAsync(constants.ZERO_AMOUNT, toBN(13)); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalSafeAdd.callAsync(constants.ZERO_AMOUNT, toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); it('should calculate correct value if second argument is zero', async () => { - const result = await safeMath.externalSafeAdd.callAsync(toBN(13), constants.ZERO_AMOUNT); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalSafeAdd.callAsync(toBigNumber(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); }); describe('_maxUint256', () => { it('should return first argument if it is greater than the second', async () => { - const result = await safeMath.externalMaxUint256.callAsync(toBN(13), constants.ZERO_AMOUNT); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalMaxUint256.callAsync(toBigNumber(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); it('should return first argument if it is equal the second', async () => { @@ -147,15 +147,15 @@ describe('SafeMath', () => { }); it('should return second argument if it is greater than the first', async () => { - const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBN(13)); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); }); describe('_minUint256', () => { it('should return first argument if it is less than the second', async () => { - const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBN(13)); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalMaxUint256.callAsync(constants.ZERO_AMOUNT, toBigNumber(13)); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); it('should return first argument if it is equal the second', async () => { @@ -164,8 +164,8 @@ describe('SafeMath', () => { }); it('should return second argument if it is less than the first', async () => { - const result = await safeMath.externalMaxUint256.callAsync(toBN(13), constants.ZERO_AMOUNT); - expect(result).bignumber.to.be.eq(toBN(13)); + const result = await safeMath.externalMaxUint256.callAsync(toBigNumber(13), constants.ZERO_AMOUNT); + expect(result).bignumber.to.be.eq(toBigNumber(13)); }); }); });