Merge pull request #278 from 0xProject/fix/error-taker-format

Throw a better error message when taker is null|undefined or anything but not a string

7 files changed, 88 insertions, 24 deletions

diff --git a/packages/0x.js/CHANGELOG.md b/packages/0x.js/CHANGELOG.md
index 962d312d3..c69edd88c 100644
--- a/packages/0x.js/CHANGELOG.md
+++ b/packages/0x.js/CHANGELOG.md
@@ -6,6 +6,7 @@ v0.x.x - _TBD, 2017_
     * Removed accidentally included `unsubscribeAll` method from `zeroEx.proxy`, `zeroEx.etherToken` and `zeroEx.tokenRegistry` (#267)
     * Removed `etherTokenContractAddress` from `ZeroEx` constructor arg `ZeroExConfig` (#267)
     * Rename `SubscriptionOpts` to `BlockRange` (#272)
+    * Improve the error message when taker is not a string (#278)
 
 v0.27.1 - _November 28, 2017_
 ------------------------
diff --git a/packages/0x.js/src/0x.ts b/packages/0x.js/src/0x.ts
index 41fefb993..e4965f9a2 100644
--- a/packages/0x.js/src/0x.ts
+++ b/packages/0x.js/src/0x.ts
@@ -25,6 +25,7 @@ import {
 import {AbiDecoder} from './utils/abi_decoder';
 import {assert} from './utils/assert';
 import {constants} from './utils/constants';
+import {decorators} from './utils/decorators';
 import {signatureUtils} from './utils/signature_utils';
 import {utils} from './utils/utils';
 
@@ -155,6 +156,7 @@ export class ZeroEx {
      * @param   order   An object that conforms to the Order or SignedOrder interface definitions.
      * @return  The resulting orderHash from hashing the supplied order.
      */
+    @decorators.syncZeroExErrorHandler
     public static getOrderHashHex(order: Order|SignedOrder): string {
         assert.doesConformToSchema('order', order, schemas.orderSchema);
         const orderHashHex = utils.getOrderHashHex(order);
diff --git a/packages/0x.js/src/contract_wrappers/exchange_wrapper.ts b/packages/0x.js/src/contract_wrappers/exchange_wrapper.ts
index df793aa06..70d2be7e9 100644
--- a/packages/0x.js/src/contract_wrappers/exchange_wrapper.ts
+++ b/packages/0x.js/src/contract_wrappers/exchange_wrapper.ts
@@ -162,7 +162,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   orderTransactionOpts                        Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async fillOrderAsync(signedOrder: SignedOrder, fillTakerTokenAmount: BigNumber,
                                 shouldThrowOnInsufficientBalanceOrAllowance: boolean,
                                 takerAddress: string,
@@ -218,7 +218,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   orderTransactionOpts                        Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async fillOrdersUpToAsync(signedOrders: SignedOrder[], fillTakerTokenAmount: BigNumber,
                                      shouldThrowOnInsufficientBalanceOrAllowance: boolean,
                                      takerAddress: string,
@@ -299,7 +299,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   orderTransactionOpts                            Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async batchFillOrdersAsync(orderFillRequests: OrderFillRequest[],
                                       shouldThrowOnInsufficientBalanceOrAllowance: boolean,
                                       takerAddress: string,
@@ -372,7 +372,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   orderTransactionOpts    Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async fillOrKillOrderAsync(signedOrder: SignedOrder, fillTakerTokenAmount: BigNumber,
                                       takerAddress: string,
                                       orderTransactionOpts: OrderTransactionOpts = {}): Promise<string> {
@@ -417,7 +417,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   orderTransactionOpts        Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async batchFillOrKillAsync(orderFillRequests: OrderFillRequest[],
                                       takerAddress: string,
                                       orderTransactionOpts: OrderTransactionOpts = {}): Promise<string> {
@@ -485,7 +485,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   transactionOpts         Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async cancelOrderAsync(order: Order|SignedOrder,
                                   cancelTakerTokenAmount: BigNumber,
                                   orderTransactionOpts: OrderTransactionOpts = {}): Promise<string> {
@@ -526,7 +526,7 @@ export class ExchangeWrapper extends ContractWrapper {
      * @param   transactionOpts             Optional arguments this method accepts.
      * @return  Transaction hash.
      */
-    @decorators.contractCallErrorHandler
+    @decorators.asyncZeroExErrorHandler
     public async batchCancelOrdersAsync(orderCancellationRequests: OrderCancellationRequest[],
                                         orderTransactionOpts: OrderTransactionOpts = {}): Promise<string> {
         assert.doesConformToSchema('orderCancellationRequests', orderCancellationRequests,
diff --git a/packages/0x.js/src/types.ts b/packages/0x.js/src/types.ts
index 844ac9ed9..704e59ce5 100644
--- a/packages/0x.js/src/types.ts
+++ b/packages/0x.js/src/types.ts
@@ -235,6 +235,7 @@ export interface OrderFillRequest {
 }
 
 export type AsyncMethod = (...args: any[]) => Promise<any>;
+export type SyncMethod = (...args: any[]) => any;
 
 /**
  * We re-export the `Web3.Provider` type specified in the Web3 Typescript typings
diff --git a/packages/0x.js/src/utils/constants.ts b/packages/0x.js/src/utils/constants.ts
index 3de3f5bc1..6001e1c7f 100644
--- a/packages/0x.js/src/utils/constants.ts
+++ b/packages/0x.js/src/utils/constants.ts
@@ -6,6 +6,7 @@ export const constants = {
     MAX_DIGITS_IN_UNSIGNED_256_INT: 78,
     INVALID_JUMP_PATTERN: 'invalid JUMP at',
     OUT_OF_GAS_PATTERN: 'out of gas',
+    INVALID_TAKER_FORMAT: 'instance.taker is not of a type(s) string',
     UNLIMITED_ALLOWANCE_IN_BASE_UNITS: new BigNumber(2).pow(256).minus(1),
     DEFAULT_BLOCK_POLLING_INTERVAL: 1000,
 };
diff --git a/packages/0x.js/src/utils/decorators.ts b/packages/0x.js/src/utils/decorators.ts
index 1760d8872..2a823b9ac 100644
--- a/packages/0x.js/src/utils/decorators.ts
+++ b/packages/0x.js/src/utils/decorators.ts
@@ -1,17 +1,37 @@
 import * as _ from 'lodash';
 
-import {AsyncMethod, ZeroExError} from '../types';
+import {AsyncMethod, SyncMethod, ZeroExError} from '../types';
 
 import {constants} from './constants';
 
-export const decorators = {
-    /**
-     * Source: https://stackoverflow.com/a/29837695/3546986
-     */
-    contractCallErrorHandler(target: object,
-                             key: string|symbol,
-                             descriptor: TypedPropertyDescriptor<AsyncMethod>,
-    ): TypedPropertyDescriptor<AsyncMethod> {
+type ErrorTransformer = (err: Error) => Error;
+
+const contractCallErrorTransformer = (error: Error) => {
+    if (_.includes(error.message, constants.INVALID_JUMP_PATTERN)) {
+        return new Error(ZeroExError.InvalidJump);
+    }
+    if (_.includes(error.message, constants.OUT_OF_GAS_PATTERN)) {
+        return new Error(ZeroExError.OutOfGas);
+    }
+    return error;
+};
+
+const schemaErrorTransformer = (error: Error) => {
+    if (_.includes(error.message, constants.INVALID_TAKER_FORMAT)) {
+        // tslint:disable-next-line:max-line-length
+        const errMsg = 'Order taker must be of type string. If you want anyone to be able to fill an order - pass ZeroEx.NULL_ADDRESS';
+        return new Error(errMsg);
+    }
+    return error;
+};
+
+/**
+ * Source: https://stackoverflow.com/a/29837695/3546986
+ */
+const asyncErrorHandlerFactory = (errorTransformer: ErrorTransformer) => {
+    const asyncErrorHandlingDecorator = (
+        target: object, key: string|symbol, descriptor: TypedPropertyDescriptor<AsyncMethod>,
+    ) => {
         const originalMethod = (descriptor.value as AsyncMethod);
 
         // Do not use arrow syntax here. Use a function expression in
@@ -22,16 +42,46 @@ export const decorators = {
                 const result = await originalMethod.apply(this, args);
                 return result;
             } catch (error) {
-                if (_.includes(error.message, constants.INVALID_JUMP_PATTERN)) {
-                    throw new Error(ZeroExError.InvalidJump);
-                }
-                if (_.includes(error.message, constants.OUT_OF_GAS_PATTERN)) {
-                    throw new Error(ZeroExError.OutOfGas);
-                }
-                throw error;
+                const transformedError = errorTransformer(error);
+                throw transformedError;
             }
         };
 
         return descriptor;
-    },
+    };
+
+    return asyncErrorHandlingDecorator;
+};
+
+const syncErrorHandlerFactory = (errorTransformer: ErrorTransformer) => {
+    const syncErrorHandlingDecorator = (
+        target: object, key: string|symbol, descriptor: TypedPropertyDescriptor<SyncMethod>,
+    ) => {
+        const originalMethod = (descriptor.value as SyncMethod);
+
+        // Do not use arrow syntax here. Use a function expression in
+        // order to use the correct value of `this` in this method
+        // tslint:disable-next-line:only-arrow-functions
+        descriptor.value = function(...args: any[]) {
+            try {
+                const result = originalMethod.apply(this, args);
+                return result;
+            } catch (error) {
+                const transformedError = errorTransformer(error);
+                throw transformedError;
+            }
+        };
+
+        return descriptor;
+    };
+
+    return syncErrorHandlingDecorator;
+};
+
+// _.flow(f, g) = f ∘ g
+const zeroExErrorTransformer = _.flow(schemaErrorTransformer, contractCallErrorTransformer);
+
+export const decorators = {
+    asyncZeroExErrorHandler: asyncErrorHandlerFactory(zeroExErrorTransformer),
+    syncZeroExErrorHandler: syncErrorHandlerFactory(zeroExErrorTransformer),
 };
diff --git a/packages/0x.js/test/0x.js_test.ts b/packages/0x.js/test/0x.js_test.ts
index 819ac12f9..8d62b3518 100644
--- a/packages/0x.js/test/0x.js_test.ts
+++ b/packages/0x.js/test/0x.js_test.ts
@@ -152,6 +152,15 @@ describe('ZeroEx library', () => {
             const orderHash = ZeroEx.getOrderHashHex(order);
             expect(orderHash).to.be.equal(expectedOrderHash);
         });
+        it('throws a readable error message if taker format is invalid', async () => {
+            const orderWithInvalidtakerFormat = {
+                ...order,
+                taker: null as any as string,
+            };
+            // tslint:disable-next-line:max-line-length
+            const expectedErrorMessage = 'Order taker must be of type string. If you want anyone to be able to fill an order - pass ZeroEx.NULL_ADDRESS';
+            expect(() => ZeroEx.getOrderHashHex(orderWithInvalidtakerFormat)).to.throw(expectedErrorMessage);
+        });
     });
     describe('#signOrderHashAsync', () => {
         let stubs: Sinon.SinonStub[] = [];