Add test when the order was already cancelled or filled

3 files changed, 44 insertions, 31 deletions

diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts
index b5d5152dd..4b7bd172f 100644
--- a/src/contract_wrappers/exchange_wrapper.ts
+++ b/src/contract_wrappers/exchange_wrapper.ts
@@ -42,6 +42,24 @@ export class ExchangeWrapper extends ContractWrapper {
     private exchangeContractIfExists?: ExchangeContract;
     private exchangeLogEventObjs: ContractEventObj[];
     private tokenWrapper: TokenWrapper;
+    private static getOrderAddressesAndValues(order: Order): [OrderAddresses, OrderValues] {
+        const orderAddresses: OrderAddresses = [
+            order.maker,
+            order.taker,
+            order.makerTokenAddress,
+            order.takerTokenAddress,
+            order.feeRecipient,
+        ];
+        const orderValues: OrderValues = [
+            order.makerTokenAmount,
+            order.takerTokenAmount,
+            order.makerFee,
+            order.takerFee,
+            order.expirationUnixTimestampSec,
+            order.salt,
+        ];
+        return [orderAddresses, orderValues];
+    }
     constructor(web3Wrapper: Web3Wrapper, tokenWrapper: TokenWrapper) {
         super(web3Wrapper);
         this.tokenWrapper = tokenWrapper;
@@ -126,21 +144,7 @@ export class ExchangeWrapper extends ContractWrapper {
         const exchangeInstance = await this.getExchangeContractAsync();
         await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress);
 
-        const orderAddresses: OrderAddresses = [
-            signedOrder.maker,
-            signedOrder.taker,
-            signedOrder.makerTokenAddress,
-            signedOrder.takerTokenAddress,
-            signedOrder.feeRecipient,
-        ];
-        const orderValues: OrderValues = [
-            signedOrder.makerTokenAmount,
-            signedOrder.takerTokenAmount,
-            signedOrder.makerFee,
-            signedOrder.takerFee,
-            signedOrder.expirationUnixTimestampSec,
-            signedOrder.salt,
-        ];
+        const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(signedOrder);
         const gas = await exchangeInstance.fill.estimateGas(
             orderAddresses,
             orderValues,
@@ -181,21 +185,7 @@ export class ExchangeWrapper extends ContractWrapper {
         const exchangeInstance = await this.getExchangeContractAsync();
         await this.validateCancelOrderAndThrowIfInvalidAsync(order, cancelAmount);
 
-        const orderAddresses: OrderAddresses = [
-            order.maker,
-            order.taker,
-            order.makerTokenAddress,
-            order.takerTokenAddress,
-            order.feeRecipient,
-        ];
-        const orderValues: OrderValues = [
-            order.makerTokenAmount,
-            order.takerTokenAmount,
-            order.makerFee,
-            order.takerFee,
-            order.expirationUnixTimestampSec,
-            order.salt,
-        ];
+        const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order);
         const gas = await exchangeInstance.cancel.estimateGas(
             orderAddresses,
             orderValues,
@@ -241,6 +231,16 @@ export class ExchangeWrapper extends ContractWrapper {
         logEventObj.watch(callback);
         this.exchangeLogEventObjs.push(logEventObj);
     }
+
+    /**
+     * Get order hash
+     */
+    public async getOrderHashAsync(order: Order): Promise<string> {
+        const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order);
+        const exchangeInstance = await this.getExchangeContractAsync();
+        const orderHash = await exchangeInstance.getOrderHash.call(orderAddresses, orderValues);
+        return orderHash;
+    }
     private async stopWatchingExchangeLogEventsAsync() {
         const stopWatchingPromises = _.map(this.exchangeLogEventObjs, logEventObj => {
             return promisify(logEventObj.stopWatching, logEventObj)();
@@ -277,12 +277,16 @@ export class ExchangeWrapper extends ContractWrapper {
         if (cancelAmount.eq(0)) {
             throw new Error(ExchangeContractErrs.ORDER_CANCEL_AMOUNT_ZERO);
         }
+        const orderHash = await this.getOrderHashAsync(order);
+        const unavailableAmount = await this.getUnavailableTakerAmountAsync(orderHash);
+        if (order.takerTokenAmount.minus(unavailableAmount).eq(0)) {
+            throw new Error(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED);
+        }
         const currentUnixTimestampSec = Date.now() / 1000;
         if (order.expirationUnixTimestampSec.lessThan(currentUnixTimestampSec)) {
             throw new Error(ExchangeContractErrs.ORDER_CANCEL_EXPIRED);
         }
     }
-
     /**
      * This method does not currently validate the edge-case where the makerToken or takerToken is also the token used
      * to pay fees  (ZRX). It is possible for them to have enough for fees and the transfer but not both.
diff --git a/src/types.ts b/src/types.ts
index 32148df7e..a89c27576 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -80,6 +80,9 @@ export interface ExchangeContract {
     cancelled: {
         call: (orderHash: string) => BigNumber.BigNumber;
     };
+    getOrderHash: {
+        call: (orderAddresses: OrderAddresses, orderValues: OrderValues) => string;
+    }
 }
 
 export interface TokenContract {
@@ -123,6 +126,7 @@ export const ExchangeContractErrs = strEnum([
     'ORDER_FILL_EXPIRED',
     'ORDER_CANCEL_EXPIRED',
     'ORDER_CANCEL_AMOUNT_ZERO',
+    'ORDER_ALREADY_CANCELLED_OR_FILLED',
     'ORDER_REMAINING_FILL_AMOUNT_ZERO',
     'ORDER_FILL_ROUNDING_ERROR',
     'FILL_BALANCE_ALLOWANCE_ERROR',
diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts
index b9400bf04..90ebe5b38 100644
--- a/test/exchange_wrapper_test.ts
+++ b/test/exchange_wrapper_test.ts
@@ -358,6 +358,11 @@ describe('ExchangeWrapper', () => {
                 return expect(zeroEx.exchange.cancelOrderAsync(expiredSignedOrder, cancelAmount))
                     .to.be.rejectedWith(ExchangeContractErrs.ORDER_CANCEL_EXPIRED);
             });
+            it.only('should throw when order is already cancelled or filled', async () => {
+                await zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount);
+                return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount))
+                    .to.be.rejectedWith(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED);
+            });
         });
         describe('successful cancels', () => {
             it('should cancel an order', async () => {