Merge pull request #181 from 0xProject/feature/order-validation-zrx

Fees validations when one of the tokens is ZRX

3 files changed, 136 insertions, 36 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b3ce29f82..7c6b9ae62 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,7 @@
 v0.20.0 - _October 3, 2017_
     * Add `zeroEx.token.getLogsAsync` (#178)
     * Add `zeroEx.exchange.getLogsAsync` (#178)
+    * Fixed fees validation when one of the tokens transferred is ZRX (#181)
 
 v0.19.0 - _September 29, 2017_
     * Made order validation optional  (#172)
diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts
index 6f7522c41..0450d26a6 100644
--- a/src/utils/order_validation_utils.ts
+++ b/src/utils/order_validation_utils.ts
@@ -121,7 +121,11 @@ export class OrderValidationUtils {
         }
 
         if (!isMakerTokenZRX) {
-            const makerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, signedOrder.maker);
+            let makerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, signedOrder.maker);
+            const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress;
+            if (isTakerTokenZRX) {
+                makerZRXBalance = makerZRXBalance.plus(fillTakerAmount);
+            }
             const makerZRXAllowance = await this.tokenWrapper.getProxyAllowanceAsync(
                 zrxTokenAddress, signedOrder.maker);
 
@@ -141,6 +145,9 @@ export class OrderValidationUtils {
             signedOrder.takerTokenAddress, senderAddress);
 
         const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress;
+        // exchangeRate is the price of one maker token denominated in taker tokens
+        const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount);
+        const fillMakerAmount = fillTakerAmount.div(exchangeRate);
 
         const requiredTakerAmount = isTakerTokenZRX ? fillTakerAmount.plus(signedOrder.takerFee) : fillTakerAmount;
         if (requiredTakerAmount.greaterThan(takerBalance)) {
@@ -151,7 +158,11 @@ export class OrderValidationUtils {
         }
 
         if (!isTakerTokenZRX) {
-            const takerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress);
+            const isMakerTokenZRX = signedOrder.makerTokenAddress === zrxTokenAddress;
+            let takerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress);
+            if (isMakerTokenZRX) {
+                takerZRXBalance = takerZRXBalance.plus(fillMakerAmount);
+            }
             const takerZRXAllowance = await this.tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, senderAddress);
 
             if (signedOrder.takerFee.greaterThan(takerZRXBalance)) {
diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts
index f625433eb..c1a0a6c8c 100644
--- a/test/order_validation_test.ts
+++ b/test/order_validation_test.ts
@@ -207,7 +207,7 @@ describe('OrderValidation', () => {
                 .to.be.rejectedWith(ExchangeContractErrs.OrderAlreadyCancelledOrFilled);
         });
     });
-    describe('#validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync', () => {
+    describe('#validateFillOrderMakerBalancesAllowancesThrowIfInvalidAsync', () => {
         describe('should throw when not enough balance or allowance to fulfill the order', () => {
             const balanceToSubtractFromMaker = new BigNumber(3);
             const balanceToSubtractFromTaker = new BigNumber(3);
@@ -218,22 +218,6 @@ describe('OrderValidation', () => {
                     makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount,
                 );
             });
-            it('should throw when taker balance is less than fill amount', async () => {
-                await zeroEx.token.transferAsync(
-                    takerTokenAddress, takerAddress, coinbase, balanceToSubtractFromTaker,
-                );
-                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
-                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
-                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerBalance);
-            });
-            it('should throw when taker allowance is less than fill amount', async () => {
-                const newAllowanceWhichIsLessThanFillAmount = fillTakerAmount.minus(lackingAllowance);
-                await zeroEx.token.setProxyAllowanceAsync(takerTokenAddress, takerAddress,
-                    newAllowanceWhichIsLessThanFillAmount);
-                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
-                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
-                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance);
-            });
             it('should throw when maker balance is less than maker fill amount', async () => {
                 await zeroEx.token.transferAsync(
                     makerTokenAddress, makerAddress, coinbase, balanceToSubtractFromMaker,
@@ -278,23 +262,6 @@ describe('OrderValidation', () => {
                     signedOrder, fillTakerAmount, zrxTokenAddress,
                 )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerFeeAllowance);
             });
-            it('should throw when taker doesn\'t have enough balance to pay fees', async () => {
-                const balanceToSubtractFromTaker = new BigNumber(1);
-                await zeroEx.token.transferAsync(
-                    zrxTokenAddress, takerAddress, coinbase, balanceToSubtractFromTaker,
-                );
-                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
-                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
-                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeBalance);
-            });
-            it('should throw when taker doesn\'t have enough allowance to pay fees', async () => {
-                const newAllowanceWhichIsLessThanFees = makerFee.minus(1);
-                await zeroEx.token.setProxyAllowanceAsync(zrxTokenAddress, takerAddress,
-                    newAllowanceWhichIsLessThanFees);
-                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
-                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
-                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeAllowance);
-            });
         });
         describe('should throw on insufficient balance or allowance when makerToken is ZRX',
             () => {
@@ -326,6 +293,95 @@ describe('OrderValidation', () => {
                 )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerAllowance);
             });
         });
+        describe('should correctly validate fees amounts if taker token is ZRX',
+            () => {
+            let signedOrder: SignedOrder;
+            let txHash: string;
+            it('should not throw if maker will have enough ZRX to pay fees after the transfer', async () => {
+                const makerFee = new BigNumber(2);
+                const takerFee = new BigNumber(2);
+                signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                    makerTokenAddress, zrxTokenAddress, makerFee, takerFee,
+                    makerAddress, takerAddress, fillableAmount, feeRecipient,
+                );
+                txHash = await zeroEx.token.transferAsync(zrxTokenAddress, makerAddress, coinbase, makerFee);
+                await zeroEx.awaitTransactionMinedAsync(txHash);
+                await (orderValidationUtils as any).validateFillOrderMakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, zrxTokenAddress,
+                );
+            });
+            it('should throw if maker will not have enough ZRX to pay fees even after the transfer', async () => {
+                const makerFee = fillableAmount.plus(1);
+                const takerFee = fillableAmount.plus(1);
+                signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                    makerTokenAddress, zrxTokenAddress, makerFee, takerFee,
+                    makerAddress, takerAddress, fillableAmount, feeRecipient,
+                );
+                txHash = await zeroEx.token.transferAsync(zrxTokenAddress, makerAddress, coinbase, makerFee);
+                await zeroEx.awaitTransactionMinedAsync(txHash);
+                return expect(
+                    (orderValidationUtils as any).validateFillOrderMakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerFeeBalance);
+            });
+        });
+    });
+    describe('#validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync', () => {
+        describe('should throw when not enough balance or allowance to fulfill the order', () => {
+            const balanceToSubtractFromMaker = new BigNumber(3);
+            const balanceToSubtractFromTaker = new BigNumber(3);
+            const lackingAllowance = new BigNumber(3);
+            let signedOrder: SignedOrder;
+            beforeEach('create fillable signed order', async () => {
+                signedOrder = await fillScenarios.createFillableSignedOrderAsync(
+                    makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount,
+                );
+            });
+            it('should throw when taker balance is less than fill amount', async () => {
+                await zeroEx.token.transferAsync(
+                    takerTokenAddress, takerAddress, coinbase, balanceToSubtractFromTaker,
+                );
+                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerBalance);
+            });
+            it('should throw when taker allowance is less than fill amount', async () => {
+                const newAllowanceWhichIsLessThanFillAmount = fillTakerAmount.minus(lackingAllowance);
+                await zeroEx.token.setProxyAllowanceAsync(takerTokenAddress, takerAddress,
+                    newAllowanceWhichIsLessThanFillAmount);
+                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance);
+            });
+        });
+        describe('should throw when not enough balance or allowance to pay fees', () => {
+            const makerFee = new BigNumber(2);
+            const takerFee = new BigNumber(2);
+            let signedOrder: SignedOrder;
+            beforeEach('setup', async () => {
+                signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                    makerTokenAddress, takerTokenAddress, makerFee, takerFee,
+                    makerAddress, takerAddress, fillableAmount, feeRecipient,
+                );
+            });
+            it('should throw when taker doesn\'t have enough balance to pay fees', async () => {
+                const balanceToSubtractFromTaker = new BigNumber(1);
+                await zeroEx.token.transferAsync(
+                    zrxTokenAddress, takerAddress, coinbase, balanceToSubtractFromTaker,
+                );
+                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeBalance);
+            });
+            it('should throw when taker doesn\'t have enough allowance to pay fees', async () => {
+                const newAllowanceWhichIsLessThanFees = makerFee.minus(1);
+                await zeroEx.token.setProxyAllowanceAsync(zrxTokenAddress, takerAddress,
+                    newAllowanceWhichIsLessThanFees);
+                return expect((orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeAllowance);
+            });
+        });
         describe('should throw on insufficient balance or allowance when takerToken is ZRX',
             () => {
             const makerFee = new BigNumber(2);
@@ -356,5 +412,37 @@ describe('OrderValidation', () => {
                 )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance);
             });
         });
+        describe('should correctly validate fees amounts if maker token is ZRX',
+            () => {
+            let signedOrder: SignedOrder;
+            let txHash: string;
+            it('should not throw if taker will have enough ZRX to pay fees after the transfer', async () => {
+                const makerFee = new BigNumber(2);
+                const takerFee = new BigNumber(2);
+                signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                    zrxTokenAddress, takerTokenAddress, makerFee, takerFee,
+                    makerAddress, takerAddress, fillableAmount, feeRecipient,
+                );
+                txHash = await zeroEx.token.transferAsync(zrxTokenAddress, takerAddress, coinbase, takerFee);
+                await zeroEx.awaitTransactionMinedAsync(txHash);
+                await (orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                );
+            });
+            it('should throw if maker will not have enough ZRX to pay fees even after the transfer', async () => {
+                const makerFee = fillableAmount.plus(1);
+                const takerFee = fillableAmount.plus(1);
+                signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                    zrxTokenAddress, takerTokenAddress, makerFee, takerFee,
+                    makerAddress, takerAddress, fillableAmount, feeRecipient,
+                );
+                txHash = await zeroEx.token.transferAsync(zrxTokenAddress, takerAddress, coinbase, takerFee);
+                await zeroEx.awaitTransactionMinedAsync(txHash);
+                return expect(
+                    (orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync(
+                    signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress,
+                )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeBalance);
+            });
+        });
     });
 });