Cover all possible branches of order validation errors with tests

2 files changed, 67 insertions, 25 deletions

diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts
index 2d7acd905..4452a7aef 100644
--- a/src/utils/order_validation_utils.ts
+++ b/src/utils/order_validation_utils.ts
@@ -16,14 +16,9 @@ export class OrderValidationUtils {
         tokenWrapper: TokenWrapper, signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber,
         zrxTokenAddress: string,
     ): Promise<void> {
-        const makerBalance = await tokenWrapper.getBalanceAsync(
-            signedOrder.makerTokenAddress, signedOrder.maker);
+        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
@@ -39,6 +34,9 @@ export class OrderValidationUtils {
                 throw new Error(ExchangeContractErrs.InsufficientMakerAllowance);
             }
         } else {
+            const makerZRXBalance = await tokenWrapper.getBalanceAsync(zrxTokenAddress, signedOrder.maker);
+            const makerZRXAllowance = await tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, signedOrder.maker);
+
             if (fillMakerAmount.greaterThan(makerBalance)) {
                 throw new Error(ExchangeContractErrs.InsufficientMakerBalance);
             }
@@ -58,11 +56,7 @@ export class OrderValidationUtils {
         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 takerAllowance = await tokenWrapper.getProxyAllowanceAsync(signedOrder.takerTokenAddress, senderAddress);
 
         const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress;
 
@@ -75,6 +69,9 @@ export class OrderValidationUtils {
                 throw new Error(ExchangeContractErrs.InsufficientTakerAllowance);
             }
         } else {
+            const takerZRXBalance = await tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress);
+            const takerZRXAllowance = await tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, senderAddress);
+
             if (fillTakerAmount.greaterThan(takerBalance)) {
                 throw new Error(ExchangeContractErrs.InsufficientTakerBalance);
             }
diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts
index 813afdddd..2488e6428 100644
--- a/test/exchange_wrapper_test.ts
+++ b/test/exchange_wrapper_test.ts
@@ -303,20 +303,65 @@ describe('ExchangeWrapper', () => {
                         )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeAllowance);
                     });
                 });
-                it('should throw when maker has balance to cover fees or transfer but not both', async () => {
-                    const makerFee = new BigNumber(1);
-                    const takerFee = new BigNumber(1);
-                    const signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
-                        zrxTokenAddress, takerTokenAddress, makerFee, takerFee,
-                        makerAddress, takerAddress, fillableAmount, feeRecipient,
-                    );
-                    const balanceToSubtractFromMaker = new BigNumber(1);
-                    await zeroEx.token.transferAsync(
-                        zrxTokenAddress, makerAddress, coinbase, balanceToSubtractFromMaker,
-                    );
-                    return expect(
-                        zeroEx.exchange.fillOrderAsync(signedOrder, fillTakerAmount, shouldCheckTransfer, takerAddress),
-                    ).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerBalance);
+                describe('should throw on insufficient balance or allowance when makerToken is ZRX',
+                () => {
+                    const makerFee = new BigNumber(2);
+                    const takerFee = new BigNumber(2);
+                    let signedOrder: SignedOrder;
+                    beforeEach(async () => {
+                        signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                            zrxTokenAddress, takerTokenAddress, makerFee, takerFee,
+                            makerAddress, takerAddress, fillableAmount, feeRecipient,
+                        );
+                    });
+                    it('should throw on insufficient balance when makerToken is ZRX', async () => {
+                        const balanceToSubtractFromMaker = new BigNumber(1);
+                        await zeroEx.token.transferAsync(
+                            zrxTokenAddress, makerAddress, coinbase, balanceToSubtractFromMaker,
+                        );
+                        return expect(zeroEx.exchange.fillOrderAsync(
+                            signedOrder, fillTakerAmount, shouldCheckTransfer, takerAddress,
+                        )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerBalance);
+                    });
+                    it('should throw on insufficient allowance when makerToken is ZRX', async () => {
+                        const oldAllowance = await zeroEx.token.getProxyAllowanceAsync(zrxTokenAddress, makerAddress);
+                        const newAllowanceWhichIsInsufficient = oldAllowance.minus(1);
+                        await zeroEx.token.setProxyAllowanceAsync(
+                            zrxTokenAddress, makerAddress, newAllowanceWhichIsInsufficient);
+                        return expect(zeroEx.exchange.fillOrderAsync(
+                            signedOrder, fillTakerAmount, shouldCheckTransfer, takerAddress,
+                        )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerAllowance);
+                    });
+                });
+                describe('should throw on insufficient balance or allowance when takerToken is ZRX',
+                () => {
+                    const makerFee = new BigNumber(2);
+                    const takerFee = new BigNumber(2);
+                    let signedOrder: SignedOrder;
+                    before(async () => {
+                        signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                            makerTokenAddress, zrxTokenAddress, makerFee, takerFee,
+                            makerAddress, takerAddress, fillableAmount, feeRecipient,
+                        );
+                    });
+                    it('should throw on insufficient balance when takerToken is ZRX', async () => {
+                        const balanceToSubtractFromTaker = new BigNumber(1);
+                        await zeroEx.token.transferAsync(
+                            zrxTokenAddress, takerAddress, coinbase, balanceToSubtractFromTaker,
+                        );
+                        return expect(zeroEx.exchange.fillOrderAsync(
+                            signedOrder, fillTakerAmount, shouldCheckTransfer, takerAddress,
+                        )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerBalance);
+                    });
+                    it('should throw on insufficient allowance when takerToken is ZRX', async () => {
+                        const oldAllowance = await zeroEx.token.getProxyAllowanceAsync(zrxTokenAddress, takerAddress);
+                        const newAllowanceWhichIsInsufficient = oldAllowance.minus(1);
+                        await zeroEx.token.setProxyAllowanceAsync(
+                            zrxTokenAddress, takerAddress, newAllowanceWhichIsInsufficient);
+                        return expect(zeroEx.exchange.fillOrderAsync(
+                            signedOrder, fillTakerAmount, shouldCheckTransfer, takerAddress,
+                        )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance);
+                    });
                 });
             });
             describe('successful fills', () => {