Refactor OrderValidationUtils to check for the case when ZRX is one of the tokens traded

2 files changed, 81 insertions, 55 deletions

diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts
index 232531a83..f75f8d9bb 100644
--- a/src/contract_wrappers/exchange_wrapper.ts
+++ b/src/contract_wrappers/exchange_wrapper.ts
@@ -35,7 +35,7 @@ import {
 import {assert} from '../utils/assert';
 import {utils} from '../utils/utils';
 import {eventUtils} from '../utils/event_utils';
-import {orderValidationUtils} from '../utils/order_validation_utils';
+import {OrderValidationUtils} from '../utils/order_validation_utils';
 import {ContractWrapper} from './contract_wrapper';
 import {ProxyWrapper} from './proxy_wrapper';
 import {ecSignatureSchema} from '../schemas/ec_signature_schema';
@@ -668,7 +668,7 @@ export class ExchangeWrapper extends ContractWrapper {
             throw new Error(ExchangeContractErrs.OrderFillExpired);
         }
         const zrxTokenAddress = await this._getZRXTokenAddressAsync(signedOrder.exchangeContractAddress);
-        await orderValidationUtils.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(
+        await OrderValidationUtils.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(
             this._tokenWrapper, signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress,
         );
 
diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts
index 924e9aebd..2d7acd905 100644
--- a/src/utils/order_validation_utils.ts
+++ b/src/utils/order_validation_utils.ts
@@ -1,66 +1,92 @@
 import {ExchangeContractErrs, SignedOrder} from '../types';
 import {TokenWrapper} from '../contract_wrappers/token_wrapper';
 
-export const orderValidationUtils = {
-    /**
-     * 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.
-     * Handling the edge-cases that arise when this happens would require making sure that the user has sufficient
-     * funds to pay both the fees and the transfer amount. We decided to punt on this for now as the contracts
-     * will throw for these edge-cases.
-     * TODO: Throw errors before calling the smart contract for these edge-cases in order to minimize
-     * the callers gas costs.
-     */
-    async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(
+export class OrderValidationUtils {
+    public static async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(
         tokenWrapper: TokenWrapper, signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber,
         senderAddress: string, zrxTokenAddress: string): Promise<void> {
-        const makerBalance = await tokenWrapper.getBalanceAsync(signedOrder.makerTokenAddress,
-                                                                     signedOrder.maker);
-        const takerBalance = await tokenWrapper.getBalanceAsync(signedOrder.takerTokenAddress, senderAddress);
-        const makerAllowance = await tokenWrapper.getProxyAllowanceAsync(signedOrder.makerTokenAddress,
-                                                                              signedOrder.maker);
-        const takerAllowance = await tokenWrapper.getProxyAllowanceAsync(signedOrder.takerTokenAddress,
-                                                                              senderAddress);
+        await OrderValidationUtils.validateFillOrderMakerBalancesAndAllowancesAndThrowIfInvalidAsync(
+            tokenWrapper, signedOrder, fillTakerAmount, zrxTokenAddress,
+        );
+        await OrderValidationUtils.validateFillOrderTakerBalancesAndAllowancesAndThrowIfInvalidAsync(
+            tokenWrapper, signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress,
+        );
+    }
+    private static async validateFillOrderMakerBalancesAndAllowancesAndThrowIfInvalidAsync(
+        tokenWrapper: TokenWrapper, signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber,
+        zrxTokenAddress: string,
+    ): Promise<void> {
+        const makerBalance = await tokenWrapper.getBalanceAsync(
+            signedOrder.makerTokenAddress, signedOrder.maker);
+        const makerAllowance = await tokenWrapper.getProxyAllowanceAsync(
+            signedOrder.makerTokenAddress, signedOrder.maker);
+        const makerZRXBalance = await tokenWrapper.getBalanceAsync(
+            zrxTokenAddress, signedOrder.maker);
+        const makerZRXAllowance = await tokenWrapper.getProxyAllowanceAsync(
+            zrxTokenAddress, signedOrder.maker);
 
+        const isMakerTokenZRX = signedOrder.makerTokenAddress === zrxTokenAddress;
         // exchangeRate is the price of one maker token denominated in taker tokens
         const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount);
-        const fillMakerAmountInBaseUnits = fillTakerAmount.div(exchangeRate);
-
-        const isMakerTokenZRX = signedOrder.makerTokenAddress === zrxTokenAddress;
-        const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress;
+        const fillMakerAmount = fillTakerAmount.div(exchangeRate);
 
-        if (fillTakerAmount.greaterThan(takerBalance)) {
-            throw new Error(ExchangeContractErrs.InsufficientTakerBalance);
-        }
-        if (fillTakerAmount.greaterThan(takerAllowance)) {
-            throw new Error(ExchangeContractErrs.InsufficientTakerAllowance);
-        }
-        if (fillMakerAmountInBaseUnits.greaterThan(makerBalance)) {
-            throw new Error(ExchangeContractErrs.InsufficientMakerBalance);
-        }
-        if (fillMakerAmountInBaseUnits.greaterThan(makerAllowance)) {
-            throw new Error(ExchangeContractErrs.InsufficientMakerAllowance);
+        if (isMakerTokenZRX) {
+            const requiredMakerAmount = fillMakerAmount.plus(signedOrder.makerFee);
+            if (requiredMakerAmount.greaterThan(makerBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerBalance);
+            }
+            if (requiredMakerAmount.greaterThan(makerAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerAllowance);
+            }
+        } else {
+            if (fillMakerAmount.greaterThan(makerBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerBalance);
+            }
+            if (fillMakerAmount.greaterThan(makerAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerAllowance);
+            }
+            if (signedOrder.makerFee.greaterThan(makerZRXBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerFeeBalance);
+            }
+            if (signedOrder.makerFee.greaterThan(makerZRXAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientMakerFeeAllowance);
+            }
         }
+    }
+    private static async validateFillOrderTakerBalancesAndAllowancesAndThrowIfInvalidAsync(
+        tokenWrapper: TokenWrapper, signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber,
+        senderAddress: string, zrxTokenAddress: string,
+    ): Promise<void> {
+        const takerBalance = await tokenWrapper.getBalanceAsync(signedOrder.takerTokenAddress, senderAddress);
+        const takerAllowance = await tokenWrapper.getProxyAllowanceAsync(
+            signedOrder.takerTokenAddress, senderAddress);
+        const takerZRXBalance = await tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress);
+        const takerZRXAllowance = await tokenWrapper.getProxyAllowanceAsync(
+            zrxTokenAddress, senderAddress);
 
-        const makerFeeBalance = await tokenWrapper.getBalanceAsync(zrxTokenAddress,
-                                                                        signedOrder.maker);
-        const takerFeeBalance = await tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress);
-        const makerFeeAllowance = await tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress,
-                                                                                 signedOrder.maker);
-        const takerFeeAllowance = await tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress,
-                                                                                 senderAddress);
+        const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress;
 
-        if (signedOrder.takerFee.greaterThan(takerFeeBalance)) {
-            throw new Error(ExchangeContractErrs.InsufficientTakerFeeBalance);
-        }
-        if (signedOrder.takerFee.greaterThan(takerFeeAllowance)) {
-            throw new Error(ExchangeContractErrs.InsufficientTakerFeeAllowance);
-        }
-        if (signedOrder.makerFee.greaterThan(makerFeeBalance)) {
-            throw new Error(ExchangeContractErrs.InsufficientMakerFeeBalance);
-        }
-        if (signedOrder.makerFee.greaterThan(makerFeeAllowance)) {
-            throw new Error(ExchangeContractErrs.InsufficientMakerFeeAllowance);
+        if (isTakerTokenZRX) {
+            const requiredTakerAmount = fillTakerAmount.plus(signedOrder.takerFee);
+            if (requiredTakerAmount.greaterThan(takerBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerBalance);
+            }
+            if (requiredTakerAmount.greaterThan(takerAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerAllowance);
+            }
+        } else {
+            if (fillTakerAmount.greaterThan(takerBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerBalance);
+            }
+            if (fillTakerAmount.greaterThan(takerAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerAllowance);
+            }
+            if (signedOrder.takerFee.greaterThan(takerZRXBalance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerFeeBalance);
+            }
+            if (signedOrder.takerFee.greaterThan(takerZRXAllowance)) {
+                throw new Error(ExchangeContractErrs.InsufficientTakerFeeAllowance);
+            }
         }
-    },
-};
+    }
+}