From 5b64b3ea937326978b5742ec1b3692ebe5c41991 Mon Sep 17 00:00:00 2001 From: Alex Browne Date: Mon, 2 Jul 2018 18:44:37 -0700 Subject: Improve robustness of revert reason assertions --- packages/contracts/test/utils/assertions.ts | 161 ++++++++++++++++++---------- 1 file changed, 103 insertions(+), 58 deletions(-) (limited to 'packages/contracts/test/utils/assertions.ts') diff --git a/packages/contracts/test/utils/assertions.ts b/packages/contracts/test/utils/assertions.ts index baba892d3..89e90ad2f 100644 --- a/packages/contracts/test/utils/assertions.ts +++ b/packages/contracts/test/utils/assertions.ts @@ -1,108 +1,153 @@ import { RevertReason } from '@0xproject/types'; +import { logUtils } from '@0xproject/utils'; +import { NodeType } from '@0xproject/web3-wrapper'; import * as chai from 'chai'; import { TransactionReceipt, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import * as _ from 'lodash'; -import { constants } from './constants'; import { web3Wrapper } from './web3_wrapper'; const expect = chai.expect; -function _expectEitherErrorAsync(p: Promise, error1: string, error2: string): PromiseLike { - return expect(p) - .to.be.rejected() - .then(e => { - expect(e).to.satisfy( - (err: Error) => _.includes(err.message, error1) || _.includes(err.message, error2), - `expected promise to reject with error message that includes "${error1}" or "${error2}", but got: ` + - `"${e.message}"\n`, - ); - }); +// Represents the return value of a `sendTransaction` call. The Promise should +// resolve with either a transaction receipt or a transaction hash. +export type sendTransactionResult = Promise; + +async function _getGanacheOrGethError(ganacheError: string, gethError: string): Promise { + const nodeType = await web3Wrapper.getNodeTypeAsync(); + switch (nodeType) { + case NodeType.Ganache: + return ganacheError; + case NodeType.Geth: + return gethError; + default: + throw new Error(`Unknown node type: ${nodeType}`); + } +} + +async function _getInsufficientFundsErrorMessageAsync(): Promise { + return _getGanacheOrGethError("sender doesn't have enough funds", 'insufficient funds'); +} + +async function _getTransactionFailedErrorMessageAsync(): Promise { + return _getGanacheOrGethError('revert', 'always failing transaction'); +} + +async function _getContractCallFailedErrorMessageAsync(): Promise { + return _getGanacheOrGethError('revert', 'Contract call failed'); } /** * Rejects if the given Promise does not reject with an error indicating * insufficient funds. - * @param p the Promise which is expected to reject + * @param p a promise resulting from a contract call or sendTransaction call. * @returns a new Promise which will reject if the conditions are not met and * otherwise resolve with no value. */ -export function expectInsufficientFundsAsync(p: Promise): PromiseLike { - return _expectEitherErrorAsync(p, 'insufficient funds', "sender doesn't have enough funds"); +export async function expectInsufficientFundsAsync(p: Promise): Promise { + const errMessage = await _getInsufficientFundsErrorMessageAsync(); + return expect(p).to.be.rejectedWith(errMessage); } /** - * Rejects if the given Promise does not reject with a "revert" error or the - * given otherError. - * @param p the Promise which is expected to reject - * @param otherError the other error which is accepted as a valid reject error. + * Resolves if the the sendTransaction call fails with the given revert reason. + * However, since Geth does not support revert reasons for sendTransaction, this + * falls back to expectTransactionFailedWithoutReasonAsync if the backing + * Ethereum node is Geth. + * @param p a Promise resulting from a sendTransaction call + * @param reason a specific revert reason * @returns a new Promise which will reject if the conditions are not met and * otherwise resolve with no value. */ -export function expectRevertOrOtherErrorAsync(p: Promise, otherError: string): PromiseLike { - return _expectEitherErrorAsync(p, constants.REVERT, otherError); -} +export async function expectTransactionFailedAsync(p: sendTransactionResult, reason: RevertReason): Promise { + // HACK(albrow): This dummy `catch` should not be necessary, but if you + // remove it, there is an uncaught exception and the Node process will + // forcibly exit. It's possible this is a false positive in + // make-promises-safe. + p.catch(e => { + _.noop(e); + }); -/** - * Rejects if the given Promise does not reject with a "revert" or "always - * failing transaction" error. - * @param p the Promise which is expected to reject - * @returns a new Promise which will reject if the conditions are not met and - * otherwise resolve with no value. - */ -export function expectRevertOrAlwaysFailingTransactionAsync(p: Promise): PromiseLike { - return expectRevertOrOtherErrorAsync(p, 'always failing transaction'); + const nodeType = await web3Wrapper.getNodeTypeAsync(); + switch (nodeType) { + case NodeType.Ganache: + return expect(p).to.be.rejectedWith(reason); + case NodeType.Geth: + logUtils.warn( + 'WARNING: Geth does not support revert reasons for sendTransaction. This test will pass if the transaction fails for any reason.', + ); + return expectTransactionFailedWithoutReasonAsync(p); + default: + throw new Error(`Unknown node type: ${nodeType}`); + } } /** - * Rejects if at least one the following conditions is not met: - * 1) The given Promise rejects with the given revert reason. - * 2) The given Promise rejects with an error containing "always failing transaction" - * 3) The given Promise fulfills with a txReceipt that has a status of 0 or '0', indicating the transaction failed. - * 4) The given Promise fulfills with a txHash and corresponding txReceipt has a status of 0 or '0'. - * @param p the Promise which is expected to reject - * @param reason a specific revert reason + * Resolves if the transaction fails without a revert reason, or if the + * corresponding transactionReceipt has a status of 0 or '0', indicating + * failure. + * @param p a Promise resulting from a sendTransaction call * @returns a new Promise which will reject if the conditions are not met and * otherwise resolve with no value. */ -export async function expectRevertReasonOrAlwaysFailingTransactionAsync( - p: Promise, - reason: RevertReason, -): Promise { +export async function expectTransactionFailedWithoutReasonAsync(p: sendTransactionResult): Promise { return p .then(async result => { - let txReceiptStatus: string | 0 | 1 | null; - if (typeof result === 'string') { - // Result is a txHash. We need to make a web3 call to get the receipt. + let txReceiptStatus: null | string | 0 | 1; + if (_.isString(result)) { + // Result is a txHash. We need to make a web3 call to get the + // receipt, then get the status from the receipt. const txReceipt = await web3Wrapper.awaitTransactionMinedAsync(result); txReceiptStatus = txReceipt.status; } else if ('status' in result) { - // Result is a TransactionReceiptWithDecodedLogs or TransactionReceipt - // and status is a field of result. + // Result is a transaction receipt, so we can get the status + // directly. txReceiptStatus = result.status; } else { - throw new Error('Unexpected result type'); + throw new Error('Unexpected result type: ' + typeof result); } expect(_.toString(txReceiptStatus)).to.equal( '0', - 'transactionReceipt had a non-zero status, indicating success', + 'Expected transaction to fail but receipt had a non-zero status, indicating success', ); }) - .catch(err => { - expect(err.message).to.satisfy( - (msg: string) => _.includes(msg, reason) || _.includes(msg, 'always failing transaction'), - `Expected ${reason} or 'always failing transaction' but error message was ${err.message}`, - ); + .catch(async err => { + // If the promise rejects, we expect a specific error message, + // depending on the backing Ethereum node type. + const errMessage = await _getTransactionFailedErrorMessageAsync(); + expect(err.message).to.include(errMessage); }); } /** - * Rejects if the given Promise does not reject with a "revert" or "Contract - * call failed" error. - * @param p the Promise which is expected to reject + * Resolves if the the contract call fails with the given revert reason. + * @param p a Promise resulting from a contract call + * @param reason a specific revert reason + * @returns a new Promise which will reject if the conditions are not met and + * otherwise resolve with no value. + */ +export async function expectContractCallFailed(p: Promise, reason: RevertReason): Promise { + return expect(p).to.be.rejectedWith(reason); +} + +/** + * Resolves if the contract call fails without a revert reason. + * @param p a Promise resulting from a contract call + * @returns a new Promise which will reject if the conditions are not met and + * otherwise resolve with no value. + */ +export async function expectContractCallFailedWithoutReasonAsync(p: Promise): Promise { + const errMessage = await _getContractCallFailedErrorMessageAsync(); + return expect(p).to.be.rejectedWith(errMessage); +} + +/** + * Resolves if the contract creation/deployment fails without a revert reason. + * @param p a Promise resulting from a contract creation/deployment * @returns a new Promise which will reject if the conditions are not met and * otherwise resolve with no value. */ -export function expectRevertOrContractCallFailedAsync(p: Promise): PromiseLike { - return expectRevertOrOtherErrorAsync(p, 'Contract call failed'); +export async function expectContractCreationFailedWithoutReason(p: Promise): Promise { + const errMessage = await _getTransactionFailedErrorMessageAsync(); + return expect(p).to.be.rejectedWith(errMessage); } -- cgit From d2ebf4a7772070e3ec255d0551de8c9f2822d4c3 Mon Sep 17 00:00:00 2001 From: Alex Browne Date: Tue, 3 Jul 2018 10:49:35 -0700 Subject: Add TransactionReceiptStatus type to ethereum-types --- packages/contracts/test/utils/assertions.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'packages/contracts/test/utils/assertions.ts') diff --git a/packages/contracts/test/utils/assertions.ts b/packages/contracts/test/utils/assertions.ts index 89e90ad2f..c8031c8a1 100644 --- a/packages/contracts/test/utils/assertions.ts +++ b/packages/contracts/test/utils/assertions.ts @@ -2,7 +2,7 @@ import { RevertReason } from '@0xproject/types'; import { logUtils } from '@0xproject/utils'; import { NodeType } from '@0xproject/web3-wrapper'; import * as chai from 'chai'; -import { TransactionReceipt, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; +import { TransactionReceipt, TransactionReceiptStatus, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import * as _ from 'lodash'; import { web3Wrapper } from './web3_wrapper'; @@ -93,7 +93,7 @@ export async function expectTransactionFailedAsync(p: sendTransactionResult, rea export async function expectTransactionFailedWithoutReasonAsync(p: sendTransactionResult): Promise { return p .then(async result => { - let txReceiptStatus: null | string | 0 | 1; + let txReceiptStatus: TransactionReceiptStatus; if (_.isString(result)) { // Result is a txHash. We need to make a web3 call to get the // receipt, then get the status from the receipt. -- cgit From dc956020ef7c6d3f1880263700422b31253c8da3 Mon Sep 17 00:00:00 2001 From: Alex Browne Date: Tue, 3 Jul 2018 12:55:05 -0700 Subject: Move NodeType caching out of web3-wrapper and into our internal code --- packages/contracts/test/utils/assertions.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'packages/contracts/test/utils/assertions.ts') diff --git a/packages/contracts/test/utils/assertions.ts b/packages/contracts/test/utils/assertions.ts index c8031c8a1..112a470f6 100644 --- a/packages/contracts/test/utils/assertions.ts +++ b/packages/contracts/test/utils/assertions.ts @@ -9,12 +9,16 @@ import { web3Wrapper } from './web3_wrapper'; const expect = chai.expect; +let nodeType: NodeType | undefined; + // Represents the return value of a `sendTransaction` call. The Promise should // resolve with either a transaction receipt or a transaction hash. export type sendTransactionResult = Promise; async function _getGanacheOrGethError(ganacheError: string, gethError: string): Promise { - const nodeType = await web3Wrapper.getNodeTypeAsync(); + if (_.isUndefined(nodeType)) { + nodeType = await web3Wrapper.getNodeTypeAsync(); + } switch (nodeType) { case NodeType.Ganache: return ganacheError; @@ -68,7 +72,9 @@ export async function expectTransactionFailedAsync(p: sendTransactionResult, rea _.noop(e); }); - const nodeType = await web3Wrapper.getNodeTypeAsync(); + if (_.isUndefined(nodeType)) { + nodeType = await web3Wrapper.getNodeTypeAsync(); + } switch (nodeType) { case NodeType.Ganache: return expect(p).to.be.rejectedWith(reason); -- cgit