diff options
5 files changed, 113 insertions, 37 deletions
diff --git a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol index a7ff400b9..9e816716c 100644 --- a/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol +++ b/packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol @@ -69,20 +69,14 @@ contract MixinExchangeWrapper is fillOrderCalldata, // write output over input 128 // output size is 128 bytes ) - switch success - case 0 { - mstore(fillResults, 0) - mstore(add(fillResults, 32), 0) - mstore(add(fillResults, 64), 0) - mstore(add(fillResults, 96), 0) - } - case 1 { + if success { mstore(fillResults, mload(fillOrderCalldata)) mstore(add(fillResults, 32), mload(add(fillOrderCalldata, 32))) mstore(add(fillResults, 64), mload(add(fillOrderCalldata, 64))) mstore(add(fillResults, 96), mload(add(fillOrderCalldata, 96))) } } + // fillResults values will be 0 by default if call was unsuccessful return fillResults; } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol index 39fa724cc..a5459a21e 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol @@ -96,6 +96,7 @@ contract MixinWrapperFunctions is mstore(add(fillResults, 96), mload(add(fillOrderCalldata, 96))) } } + // fillResults values will be 0 by default if call was unsuccessful return fillResults; } diff --git a/packages/contracts/test/exchange/internal.ts b/packages/contracts/test/exchange/internal.ts index 2521665c2..de381fca3 100644 --- a/packages/contracts/test/exchange/internal.ts +++ b/packages/contracts/test/exchange/internal.ts @@ -67,9 +67,7 @@ describe('Exchange core internal functions', () => { overflowErrorForSendTransaction = new Error( await getRevertReasonOrErrorMessageForSendTransactionAsync(RevertReason.Uint256Overflow), ); - divisionByZeroErrorForCall = new Error( - await getRevertReasonOrErrorMessageForSendTransactionAsync(RevertReason.DivisionByZero), - ); + divisionByZeroErrorForCall = new Error(RevertReason.DivisionByZero); invalidOpcodeErrorForCall = new Error(await getInvalidOpcodeErrorMessageForCallAsync()); }); // Note(albrow): Don't forget to add beforeEach and afterEach calls to reset diff --git a/packages/contracts/test/utils/test_with_reference.ts b/packages/contracts/test/utils/test_with_reference.ts index 599b1eed4..b80be4a6c 100644 --- a/packages/contracts/test/utils/test_with_reference.ts +++ b/packages/contracts/test/utils/test_with_reference.ts @@ -6,6 +6,34 @@ import { chaiSetup } from './chai_setup'; chaiSetup.configure(); const expect = chai.expect; +class Value<T> { + public value: T; + constructor(value: T) { + this.value = value; + } +} + +// tslint:disable-next-line: max-classes-per-file +class ErrorMessage { + public error: string; + constructor(message: string) { + this.error = message; + } +} + +type PromiseResult<T> = Value<T> | ErrorMessage; + +// TODO(albrow): This seems like a generic utility function that could exist in +// lodash. We should replace it by a library implementation, or move it to our +// own. +async function evaluatePromise<T>(promise: Promise<T>): Promise<PromiseResult<T>> { + try { + return new Value<T>(await promise); + } catch (e) { + return new ErrorMessage(e.message); + } +} + export async function testWithReferenceFuncAsync<P0, R>( referenceFunc: (p0: P0) => Promise<R>, testFunc: (p0: P0) => Promise<R>, @@ -64,39 +92,31 @@ export async function testWithReferenceFuncAsync( testFuncAsync: (...args: any[]) => Promise<any>, values: any[], ): Promise<void> { - let expectedResult: any; - let expectedErr: string | undefined; - try { - expectedResult = await referenceFuncAsync(...values); - } catch (e) { - expectedErr = e.message; - } - let actualResult: any | undefined; - try { - actualResult = await testFuncAsync(...values); - if (!_.isUndefined(expectedErr)) { + // Measure correct behaviour + const expected = await evaluatePromise(referenceFuncAsync(...values)); + + // Measure actual behaviour + const actual = await evaluatePromise(testFuncAsync(...values)); + + // Compare behaviour + if (expected instanceof ErrorMessage) { + // If we expected an error, check if the actual error message contains the + // expected error message. + if (!(actual instanceof ErrorMessage)) { throw new Error( - `Expected error containing ${expectedErr} but got no error\n\tTest case: ${_getTestCaseString( + `Expected error containing ${expected.error} but got no error\n\tTest case: ${_getTestCaseString( referenceFuncAsync, values, )}`, ); } - } catch (e) { - if (_.isUndefined(expectedErr)) { - throw new Error(`${e.message}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`); - } else { - expect(e.message).to.contain( - expectedErr, - `${e.message}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`, - ); - } - } - if (!_.isUndefined(actualResult) && !_.isUndefined(expectedResult)) { - expect(actualResult).to.deep.equal( - expectedResult, - `Test case: ${_getTestCaseString(referenceFuncAsync, values)}`, + expect(actual.error).to.contain( + expected.error, + `${actual.error}\n\tTest case: ${_getTestCaseString(referenceFuncAsync, values)}`, ); + } else { + // If we do not expect an error, compare actual and expected directly. + expect(actual).to.deep.equal(expected, `Test case ${_getTestCaseString(referenceFuncAsync, values)}`); } } diff --git a/packages/contracts/test/utils_test/test_with_reference.ts b/packages/contracts/test/utils_test/test_with_reference.ts new file mode 100644 index 000000000..8d633cd1f --- /dev/null +++ b/packages/contracts/test/utils_test/test_with_reference.ts @@ -0,0 +1,63 @@ +import * as chai from 'chai'; + +import { chaiSetup } from '../utils/chai_setup'; +import { testWithReferenceFuncAsync } from '../utils/test_with_reference'; + +chaiSetup.configure(); +const expect = chai.expect; + +async function divAsync(x: number, y: number): Promise<number> { + if (y === 0) { + throw new Error('MathError: divide by zero'); + } + return x / y; +} + +// returns an async function that always returns the given value. +function alwaysValueFunc(value: number): (x: number, y: number) => Promise<number> { + return async (x: number, y: number) => value; +} + +// returns an async function which always throws/rejects with the given error +// message. +function alwaysFailFunc(errMessage: string): (x: number, y: number) => Promise<number> { + return async (x: number, y: number) => { + throw new Error(errMessage); + }; +} + +describe('testWithReferenceFuncAsync', () => { + it('passes when both succeed and actual === expected', async () => { + await testWithReferenceFuncAsync(alwaysValueFunc(0.5), divAsync, [1, 2]); + }); + + it('passes when both fail and actual error contains expected error', async () => { + await testWithReferenceFuncAsync(alwaysFailFunc('divide by zero'), divAsync, [1, 0]); + }); + + it('fails when both succeed and actual !== expected', async () => { + expect(testWithReferenceFuncAsync(alwaysValueFunc(3), divAsync, [1, 2])).to.be.rejectedWith( + 'Test case {"x":1,"y":2}: expected { value: 0.5 } to deeply equal { value: 3 }', + ); + }); + + it('fails when both fail and actual error does not contain expected error', async () => { + expect( + testWithReferenceFuncAsync(alwaysFailFunc('Unexpected math error'), divAsync, [1, 0]), + ).to.be.rejectedWith( + 'MathError: divide by zero\n\tTest case: {"x":1,"y":0}: expected \'MathError: divide by zero\' to include \'Unexpected math error\'', + ); + }); + + it('fails when referenceFunc succeeds and testFunc fails', async () => { + expect(testWithReferenceFuncAsync(alwaysValueFunc(0), divAsync, [1, 0])).to.be.rejectedWith( + 'Test case {"x":1,"y":0}: expected { error: \'MathError: divide by zero\' } to deeply equal { value: 0 }', + ); + }); + + it('fails when referenceFunc fails and testFunc succeeds', async () => { + expect(testWithReferenceFuncAsync(alwaysFailFunc('divide by zero'), divAsync, [1, 2])).to.be.rejectedWith( + 'Expected error containing divide by zero but got no error\n\tTest case: {"x":1,"y":2}', + ); + }); +}); |