aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorFabio Berger <me@fabioberger.com>2018-08-28 18:19:20 +0800
committerFabio Berger <me@fabioberger.com>2018-08-28 18:19:20 +0800
commit4a40edbbb70afd4c2599f2c6094eacf643ddcedb (patch)
tree2bb48a520aba0ded54a2a15e4ec663a266a3d751
parent2997ba208fbbbcc70785b0a759753851de3e2ca5 (diff)
parent2eab0e30b753fb33729db53d141c8e22017cadec (diff)
downloaddexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar.gz
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar.bz2
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar.lz
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar.xz
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.tar.zst
dexon-sol-tools-4a40edbbb70afd4c2599f2c6094eacf643ddcedb.zip
Merge branch 'development' into dev-dropdown
* development: fix(contracts): Catch cases where the actual error differs from the expected error (#1032) Add clarifying comments Remove redundant mstores from fillOrderNoThrow fix(contracts): Use correct error message for division by zero
-rw-r--r--packages/contracts/src/2.0.0/extensions/Forwarder/MixinExchangeWrapper.sol10
-rw-r--r--packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol1
-rw-r--r--packages/contracts/test/exchange/internal.ts4
-rw-r--r--packages/contracts/test/utils/test_with_reference.ts72
-rw-r--r--packages/contracts/test/utils_test/test_with_reference.ts63
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}',
+ );
+ });
+});