Fix validateOrderFillableOrThrowAsync method so it also checks order signature, cancelled, cancelledUpTo, and throws helpful error messages

6 files changed, 31 insertions, 24 deletions

diff --git a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts
index de096b7d9..9e240f9ef 100644
--- a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts
+++ b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts
@@ -1,3 +1,4 @@
+import { SignedOrder } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 
 /**
@@ -17,6 +18,6 @@ export abstract class AbstractOrderFilledCancelledFetcher {
      * @param orderHash OrderHash of order we are interested in
      * @return Whether or not the order is cancelled
      */
-    public abstract async isOrderCancelledAsync(orderHash: string): Promise<boolean>;
+    public abstract async isOrderCancelledAsync(signedOrder: SignedOrder): Promise<boolean>;
     public abstract getZRXAssetData(): string;
 }
diff --git a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts
index d9e66db06..186521401 100644
--- a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts
+++ b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts
@@ -1,8 +1,9 @@
+import { SignedOrder } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 
 export abstract class AbstractOrderFilledCancelledLazyStore {
     public abstract async getFilledTakerAmountAsync(orderHash: string): Promise<BigNumber>;
-    public abstract async getIsCancelledAsync(orderHash: string): Promise<boolean>;
+    public abstract async getIsCancelledAsync(signedOrder: SignedOrder): Promise<boolean>;
     public abstract setFilledTakerAmount(orderHash: string, balance: BigNumber): void;
     public abstract deleteFilledTakerAmount(orderHash: string): void;
     public abstract setIsCancelled(orderHash: string, isCancelled: boolean): void;
diff --git a/packages/order-utils/src/order_state_utils.ts b/packages/order-utils/src/order_state_utils.ts
index 159aeeb09..fe0d6c773 100644
--- a/packages/order-utils/src/order_state_utils.ts
+++ b/packages/order-utils/src/order_state_utils.ts
@@ -117,7 +117,7 @@ export class OrderStateUtils {
     public async getOpenOrderStateAsync(signedOrder: SignedOrder, transactionHash?: string): Promise<OrderState> {
         const orderRelevantState = await this.getOpenOrderRelevantStateAsync(signedOrder);
         const orderHash = orderHashUtils.getOrderHashHex(signedOrder);
-        const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash);
+        const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder);
         const sidedOrderRelevantState = {
             isMakerSide: true,
             traderBalance: orderRelevantState.makerBalance,
@@ -256,7 +256,7 @@ export class OrderStateUtils {
         const filledTakerAssetAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash);
         const totalMakerAssetAmount = signedOrder.makerAssetAmount;
         const totalTakerAssetAmount = signedOrder.takerAssetAmount;
-        const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash);
+        const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder);
         const remainingTakerAssetAmount = isOrderCancelled
             ? new BigNumber(0)
             : totalTakerAssetAmount.minus(filledTakerAssetAmount);
diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts
index a40069f63..766fbaf0f 100644
--- a/packages/order-utils/src/order_validation_utils.ts
+++ b/packages/order-utils/src/order_validation_utils.ts
@@ -109,14 +109,6 @@ export class OrderValidationUtils {
             throw new Error(RevertReason.TransferFailed);
         }
     }
-    private static _validateRemainingFillAmountNotZeroOrThrow(
-        takerAssetAmount: BigNumber,
-        filledTakerTokenAmount: BigNumber,
-    ): void {
-        if (takerAssetAmount.eq(filledTakerTokenAmount)) {
-            throw new Error(RevertReason.OrderUnfillable);
-        }
-    }
     private static _validateOrderNotExpiredOrThrow(expirationTimeSeconds: BigNumber): void {
         const currentUnixTimestampSec = utils.getCurrentUnixTimestampSec();
         if (expirationTimeSeconds.lessThan(currentUnixTimestampSec)) {
@@ -125,12 +117,15 @@ export class OrderValidationUtils {
     }
     /**
      * Instantiate OrderValidationUtils
-     * @param orderFilledCancelledFetcher A module that implements the AbstractOrderFilledCancelledFetcher
+     * @param orderFilledCancelledFetcher A module that implements the AbstractOrderInfoFetcher
      * @return An instance of OrderValidationUtils
      */
     constructor(orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher) {
         this._orderFilledCancelledFetcher = orderFilledCancelledFetcher;
     }
+    // TODO(fabio): remove this method once the smart contracts have been refactored
+    // to return helpful revert reasons instead of ORDER_UNFILLABLE. Instruct devs
+    // to make "calls" to validate order fillability + getOrderInfo for fillable amount.
     /**
      * Validate if the supplied order is fillable, and throw if it isn't
      * @param exchangeTradeEmulator ExchangeTradeEmulator instance
@@ -146,12 +141,19 @@ export class OrderValidationUtils {
         expectedFillTakerTokenAmount?: BigNumber,
     ): Promise<void> {
         const orderHash = orderHashUtils.getOrderHashHex(signedOrder);
+        const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder);
+        if (isCancelled) {
+            throw new Error('CANCELLED');
+        }
         const filledTakerTokenAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash);
-        OrderValidationUtils._validateRemainingFillAmountNotZeroOrThrow(
-            signedOrder.takerAssetAmount,
-            filledTakerTokenAmount,
-        );
+        if (signedOrder.takerAssetAmount.eq(filledTakerTokenAmount)) {
+            throw new Error('FULLY_FILLED');
+        }
+        try {
         OrderValidationUtils._validateOrderNotExpiredOrThrow(signedOrder.expirationTimeSeconds);
+        } catch (err) {
+            throw new Error('EXPIRED');
+        }
         let fillTakerAssetAmount = signedOrder.takerAssetAmount.minus(filledTakerTokenAmount);
         if (!_.isUndefined(expectedFillTakerTokenAmount)) {
             fillTakerAssetAmount = expectedFillTakerTokenAmount;
@@ -198,10 +200,9 @@ export class OrderValidationUtils {
             throw new Error(OrderError.InvalidSignature);
         }
         const filledTakerTokenAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash);
-        OrderValidationUtils._validateRemainingFillAmountNotZeroOrThrow(
-            signedOrder.takerAssetAmount,
-            filledTakerTokenAmount,
-        );
+        if (signedOrder.takerAssetAmount.eq(filledTakerTokenAmount)) {
+            throw new Error(RevertReason.OrderUnfillable);
+        }
         if (signedOrder.takerAddress !== constants.NULL_ADDRESS && signedOrder.takerAddress !== takerAddress) {
             throw new Error(RevertReason.InvalidTaker);
         }
diff --git a/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts b/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts
index 1d84ffdaa..afd6f1108 100644
--- a/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts
+++ b/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts
@@ -1,8 +1,10 @@
+import { SignedOrder } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 import * as _ from 'lodash';
 
 import { AbstractOrderFilledCancelledFetcher } from '../abstract/abstract_order_filled_cancelled_fetcher';
 import { AbstractOrderFilledCancelledLazyStore } from '../abstract/abstract_order_filled_cancelled_lazy_store';
+import { orderHashUtils } from '../order_hash';
 
 /**
  * Copy on read store for balances/proxyAllowances of tokens/accounts
@@ -58,9 +60,10 @@ export class OrderFilledCancelledLazyStore implements AbstractOrderFilledCancell
      * @param orderHash OrderHash from order of interest
      * @return Whether the order has been cancelled
      */
-    public async getIsCancelledAsync(orderHash: string): Promise<boolean> {
+    public async getIsCancelledAsync(signedOrder: SignedOrder): Promise<boolean> {
+        const orderHash = orderHashUtils.getOrderHashHex(signedOrder);
         if (_.isUndefined(this._isCancelled[orderHash])) {
-            const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash);
+            const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder);
             this.setIsCancelled(orderHash, isCancelled);
         }
         const cachedIsCancelled = this._isCancelled[orderHash]; // tslint:disable-line:boolean-naming
diff --git a/packages/order-utils/test/order_state_utils_test.ts b/packages/order-utils/test/order_state_utils_test.ts
index 39c4c362f..42acd54c6 100644
--- a/packages/order-utils/test/order_state_utils_test.ts
+++ b/packages/order-utils/test/order_state_utils_test.ts
@@ -1,3 +1,4 @@
+import { SignedOrder } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 import * as chai from 'chai';
 import 'mocha';
@@ -33,7 +34,7 @@ describe('OrderStateUtils', () => {
                 async getFilledTakerAmountAsync(_orderHash: string): Promise<BigNumber> {
                     return filledAmount;
                 },
-                async isOrderCancelledAsync(_orderHash: string): Promise<boolean> {
+                async isOrderCancelledAsync(_signedOrder: SignedOrder): Promise<boolean> {
                     return cancelled;
                 },
                 getZRXAssetData(): string {