Add a test for when the ratio is < 1

2 files changed, 104 insertions, 52 deletions

diff --git a/packages/0x.js/src/order_watcher/remaining_fillable_calculator.ts b/packages/0x.js/src/order_watcher/remaining_fillable_calculator.ts
index 8141dd73c..b98ef3240 100644
--- a/packages/0x.js/src/order_watcher/remaining_fillable_calculator.ts
+++ b/packages/0x.js/src/order_watcher/remaining_fillable_calculator.ts
@@ -1,71 +1,83 @@
-import {
-    SignedOrder,
-} from '../types';
-import { BigNumber } from 'bignumber.js';
+import {SignedOrder} from '../types';
+import {BigNumber} from 'bignumber.js';
+
 export class RemainingFillableCalculator {
-    private _signedOrder: SignedOrder;
-    private _isMakerTokenZRX: boolean;
-    private _transferrableMakerTokenAmount: BigNumber;
-    private _transferrableMakerFeeTokenAmount: BigNumber;
-    private _remainingMakerTokenAmount: BigNumber;
-    private _remainingMakerFeeAmount: BigNumber;
+    private signedOrder: SignedOrder;
+    private isMakerTokenZRX: boolean;
+    // Transferrable Amount is the minimum of Approval and Balance
+    private transferrableMakerTokenAmount: BigNumber;
+    private transferrableMakerFeeTokenAmount: BigNumber;
+    private remainingMakerTokenAmount: BigNumber;
+    private remainingMakerFeeAmount: BigNumber;
     constructor(signedOrder: SignedOrder,
                 isMakerTokenZRX: boolean,
                 transferrableMakerTokenAmount: BigNumber,
                 transferrableMakerFeeTokenAmount: BigNumber,
                 remainingMakerTokenAmount: BigNumber) {
-        this._signedOrder = signedOrder;
-        this._isMakerTokenZRX = isMakerTokenZRX;
-        this._transferrableMakerTokenAmount = transferrableMakerTokenAmount;
-        this._transferrableMakerFeeTokenAmount = transferrableMakerFeeTokenAmount;
-        this._remainingMakerTokenAmount = remainingMakerTokenAmount;
-        this._remainingMakerFeeAmount = remainingMakerTokenAmount.times(signedOrder.makerFee)
-                                                                 .dividedToIntegerBy(signedOrder.makerTokenAmount);
+        this.signedOrder = signedOrder;
+        this.isMakerTokenZRX = isMakerTokenZRX;
+        this.transferrableMakerTokenAmount = transferrableMakerTokenAmount;
+        this.transferrableMakerFeeTokenAmount = transferrableMakerFeeTokenAmount;
+        this.remainingMakerTokenAmount = remainingMakerTokenAmount;
+        this.remainingMakerFeeAmount = remainingMakerTokenAmount.times(signedOrder.makerFee)
+                                                                .dividedToIntegerBy(signedOrder.makerTokenAmount);
     }
     public computeRemainingMakerFillable(): BigNumber {
         if (this.hasSufficientFundsForFeeAndTransferAmount()) {
-            return this._remainingMakerTokenAmount;
+            return this.remainingMakerTokenAmount;
         }
-        if (this._signedOrder.makerFee.isZero()) {
-            return BigNumber.min(this._remainingMakerTokenAmount, this._transferrableMakerTokenAmount);
-        } else {
-            return this.calculatePartiallyFillableMakerTokenAmount();
+        if (this.signedOrder.makerFee.isZero()) {
+            return BigNumber.min(this.remainingMakerTokenAmount, this.transferrableMakerTokenAmount);
         }
+        return this.calculatePartiallyFillableMakerTokenAmount();
     }
     public computeRemainingTakerFillable(): BigNumber {
-        return this.computeRemainingMakerFillable().times(this._signedOrder.takerTokenAmount)
-                                                   .dividedToIntegerBy(this._signedOrder.makerTokenAmount);
+        return this.computeRemainingMakerFillable().times(this.signedOrder.takerTokenAmount)
+                                                   .dividedToIntegerBy(this.signedOrder.makerTokenAmount);
     }
     private hasSufficientFundsForFeeAndTransferAmount(): boolean {
-        if (this._isMakerTokenZRX) {
-            const totalZRXTransferAmountRequired = this._remainingMakerTokenAmount.plus(this._remainingMakerFeeAmount);
-            return this._transferrableMakerTokenAmount.gte(totalZRXTransferAmountRequired);
+        if (this.isMakerTokenZRX) {
+            const totalZRXTransferAmountRequired = this.remainingMakerTokenAmount.plus(this.remainingMakerFeeAmount);
+            const hasSufficientFunds = this.transferrableMakerTokenAmount.greaterThanOrEqualTo(
+                                                                            totalZRXTransferAmountRequired);
+            return hasSufficientFunds;
         } else {
-            const hasSufficientFundsForTransferAmount = this._transferrableMakerTokenAmount.gte(
-                                                            this._remainingMakerTokenAmount);
-            const hasSufficientFundsForFeeAmount = this._transferrableMakerFeeTokenAmount.gte(
-                                                       this._remainingMakerFeeAmount);
-            return (hasSufficientFundsForTransferAmount && hasSufficientFundsForFeeAmount);
+            const hasSufficientFundsForTransferAmount = this.transferrableMakerTokenAmount.greaterThanOrEqualTo(
+                                                            this.remainingMakerTokenAmount);
+            const hasSufficientFundsForFeeAmount = this.transferrableMakerFeeTokenAmount.greaterThanOrEqualTo(
+                                                       this.remainingMakerFeeAmount);
+            const hasSufficientFunds = hasSufficientFundsForTransferAmount && hasSufficientFundsForFeeAmount;
+            return hasSufficientFunds;
         }
     }
-
     private calculatePartiallyFillableMakerTokenAmount(): BigNumber {
-        const orderToFeeRatio = this._signedOrder.makerTokenAmount.dividedToIntegerBy(this._signedOrder.makerFee);
+        // The number of times the maker can fill the order, if each fill only required the transfer of a single
+        // baseUnit of fee tokens.
+        // Given an order for 200 wei for 2 ZRXwei fee, find 100 wei for 1 ZRXwei. Order ratio is then 100:1
+        const orderToFeeRatio = this.signedOrder.makerTokenAmount.dividedBy(this.signedOrder.makerFee);
         // Maximum number of times the Maker can fill the order, given the fees
-        const fillableTimesInFeeTokenUnits = BigNumber.min(this._transferrableMakerFeeTokenAmount,
-                                                           this._remainingMakerFeeAmount);
+        // Given 2 ZRXwei, the maximum amount of times Maker can fill this order, in terms of fees, is 2
+        const fillableTimesInFeeTokenBaseUnits = BigNumber.min(this.transferrableMakerFeeTokenAmount,
+                                                               this.remainingMakerFeeAmount);
         // Maximum number of times the Maker can fill the order, given the Maker Token Balance
-        let fillableTimesInMakerTokenUnits = this._transferrableMakerTokenAmount.dividedToIntegerBy(orderToFeeRatio);
-        if (this._isMakerTokenZRX) {
-            const totalZRXTokenPooled = this._transferrableMakerTokenAmount;
+        // Assuming a balance of 150 wei, maker can fill this order 1 time.
+        let fillableTimesInMakerTokenUnits = this.transferrableMakerTokenAmount.dividedBy(orderToFeeRatio);
+        if (this.isMakerTokenZRX) {
+            // If ZRX is the maker token, the Fee and the Maker amount need to be removed from the same pool;
+            // 200 ZRXwei for 2ZRXwei fee can only be filled once (need 202 ZRXwei)
+            const totalZRXTokenPooled = this.transferrableMakerTokenAmount;
             // The purchasing power here is less as the tokens are taken from the same Pool
             // For every one number of fills, we have to take an extra ZRX out of the pool
-            fillableTimesInMakerTokenUnits = totalZRXTokenPooled.dividedToIntegerBy(
+            fillableTimesInMakerTokenUnits = totalZRXTokenPooled.dividedBy(
                                                                      orderToFeeRatio.plus(new BigNumber(1)));
 
         }
-        const partiallyFillableMakerTokenAmount = fillableTimesInMakerTokenUnits.times(orderToFeeRatio);
-        const partiallyFillableFeeTokenAmount = fillableTimesInFeeTokenUnits.times(orderToFeeRatio);
-        return BigNumber.min(partiallyFillableMakerTokenAmount, partiallyFillableFeeTokenAmount);
+        // When Ratio is not fully divisible there can be remainders which cannot be represented, so they are floored.
+        // This can result in a RoundingError being thrown by the Exchange Contract.
+        const partiallyFillableMakerTokenAmount = fillableTimesInMakerTokenUnits.times(orderToFeeRatio).floor();
+        const partiallyFillableFeeTokenAmount = fillableTimesInFeeTokenBaseUnits.times(orderToFeeRatio).floor();
+        const partiallyFillableAmount = BigNumber.min(partiallyFillableMakerTokenAmount,
+                                                      partiallyFillableFeeTokenAmount);
+        return partiallyFillableAmount;
     }
 }
diff --git a/packages/0x.js/test/remaining_fillable_calculator_test.ts b/packages/0x.js/test/remaining_fillable_calculator_test.ts
index 0b1f517fa..65b65efd8 100644
--- a/packages/0x.js/test/remaining_fillable_calculator_test.ts
+++ b/packages/0x.js/test/remaining_fillable_calculator_test.ts
@@ -18,7 +18,7 @@ describe('RemainingFillableCalculator', () => {
     let remainingMakerTokenAmount: BigNumber;
     let makerAmount: BigNumber;
     let takerAmount: BigNumber;
-    let makerFee: BigNumber;
+    let makerFeeAmount: BigNumber;
     let isMakerTokenZRX: boolean;
     const makerToken: string = '0x1';
     const takerToken: string = '0x2';
@@ -27,9 +27,9 @@ describe('RemainingFillableCalculator', () => {
     const zeroAddress = '0x0';
     const signature: ECSignature = { v: 27, r: '', s: ''};
     beforeEach(async () => {
-        [makerAmount, takerAmount, makerFee] = [ZeroEx.toBaseUnitAmount(new BigNumber(50), decimals),
-                                                ZeroEx.toBaseUnitAmount(new BigNumber(5), decimals),
-                                                ZeroEx.toBaseUnitAmount(new BigNumber(1), decimals)];
+        [makerAmount, takerAmount, makerFeeAmount] = [ZeroEx.toBaseUnitAmount(new BigNumber(50), decimals),
+                                                      ZeroEx.toBaseUnitAmount(new BigNumber(5), decimals),
+                                                      ZeroEx.toBaseUnitAmount(new BigNumber(1), decimals)];
         [transferrableMakerTokenAmount, transferrableMakerFeeTokenAmount] = [
                                                 ZeroEx.toBaseUnitAmount(new BigNumber(50), decimals),
                                                 ZeroEx.toBaseUnitAmount(new BigNumber(5), decimals)];
@@ -40,7 +40,7 @@ describe('RemainingFillableCalculator', () => {
                  feeRecipient: zeroAddress,
                  maker: zeroAddress,
                  taker: zeroAddress,
-                 makerFee: (makerFee || zero),
+                 makerFee: makerFeeAmount,
                  takerFee: zero,
                  makerTokenAmount: makerAmount,
                  takerTokenAmount: takerAmount,
@@ -84,6 +84,45 @@ describe('RemainingFillableCalculator', () => {
                            transferrableMakerTokenAmount, transferrableMakerFeeTokenAmount, remainingMakerTokenAmount);
             expect(calculator.computeRemainingMakerFillable()).to.be.bignumber.equal(transferrableMakerTokenAmount);
         });
+        describe('Order to Fee Ratio is < 1', () => {
+            beforeEach(async () => {
+                [makerAmount, takerAmount, makerFeeAmount] = [ZeroEx.toBaseUnitAmount(new BigNumber(3), decimals),
+                                                              ZeroEx.toBaseUnitAmount(new BigNumber(6), decimals),
+                                                              ZeroEx.toBaseUnitAmount(new BigNumber(6), decimals)];
+            });
+            it('calculates the correct amount when funds unavailable', () => {
+                signedOrder = buildSignedOrder();
+                remainingMakerTokenAmount = signedOrder.makerTokenAmount;
+                const transferredAmount = ZeroEx.toBaseUnitAmount(new BigNumber(2), decimals);
+                transferrableMakerTokenAmount = remainingMakerTokenAmount.minus(transferredAmount);
+                calculator = new RemainingFillableCalculator(signedOrder, isMakerTokenZRX,
+                                 transferrableMakerTokenAmount, transferrableMakerFeeTokenAmount,
+                                 remainingMakerTokenAmount);
+                expect(calculator.computeRemainingMakerFillable()).to.be.bignumber.equal(transferrableMakerTokenAmount);
+            });
+        });
+        describe('Ratio is not evenly divisble', () => {
+            beforeEach(async () => {
+                [makerAmount, takerAmount, makerFeeAmount] = [ZeroEx.toBaseUnitAmount(new BigNumber(3), decimals),
+                                                              ZeroEx.toBaseUnitAmount(new BigNumber(7), decimals),
+                                                              ZeroEx.toBaseUnitAmount(new BigNumber(7), decimals)];
+            });
+            it('calculates the correct amount when funds unavailable', () => {
+                signedOrder = buildSignedOrder();
+                remainingMakerTokenAmount = signedOrder.makerTokenAmount;
+                const transferredAmount = ZeroEx.toBaseUnitAmount(new BigNumber(2), decimals);
+                transferrableMakerTokenAmount = remainingMakerTokenAmount.minus(transferredAmount);
+                calculator = new RemainingFillableCalculator(signedOrder, isMakerTokenZRX,
+                                 transferrableMakerTokenAmount, transferrableMakerFeeTokenAmount,
+                                 remainingMakerTokenAmount);
+                const calculatedFillableAmount = calculator.computeRemainingMakerFillable();
+                expect(calculatedFillableAmount.lessThanOrEqualTo(transferrableMakerTokenAmount)).to.be.true();
+                expect(calculatedFillableAmount).to.be.bignumber.greaterThan(new BigNumber(0));
+                const orderToFeeRatio = signedOrder.makerTokenAmount.dividedBy(signedOrder.makerFee);
+                const calculatedFeeAmount = calculatedFillableAmount.dividedBy(orderToFeeRatio);
+                expect(calculatedFeeAmount).to.be.bignumber.lessThan(transferrableMakerFeeTokenAmount);
+            });
+        });
     });
     describe('Maker Token is ZRX', () => {
         before(async () => {
@@ -91,7 +130,7 @@ describe('RemainingFillableCalculator', () => {
         });
         it('calculates the correct amount when unfilled and funds available', () => {
             signedOrder = buildSignedOrder();
-            transferrableMakerTokenAmount = makerAmount.plus(makerFee);
+            transferrableMakerTokenAmount = makerAmount.plus(makerFeeAmount);
             transferrableMakerFeeTokenAmount = transferrableMakerTokenAmount;
             remainingMakerTokenAmount = signedOrder.makerTokenAmount;
             calculator = new RemainingFillableCalculator(signedOrder, isMakerTokenZRX,
@@ -122,15 +161,16 @@ describe('RemainingFillableCalculator', () => {
             transferrableMakerFeeTokenAmount = transferrableMakerTokenAmount;
 
             const orderToFeeRatio = signedOrder.makerTokenAmount.dividedToIntegerBy(signedOrder.makerFee);
-            const expectedFillableAmount = new BigNumber(450950);
-            const numberOfFillsInRatio = expectedFillableAmount.dividedToIntegerBy(orderToFeeRatio);
+            const expectedFillableAmount = new BigNumber(450980);
             calculator = new RemainingFillableCalculator(signedOrder, isMakerTokenZRX,
                            transferrableMakerTokenAmount, transferrableMakerFeeTokenAmount, remainingMakerTokenAmount);
             const calculatedFillableAmount = calculator.computeRemainingMakerFillable();
+            const numberOfFillsInRatio = calculatedFillableAmount.dividedToIntegerBy(orderToFeeRatio);
             const calculatedFillableAmountPlusFees = calculatedFillableAmount.plus(numberOfFillsInRatio);
-            expect(calculatedFillableAmount).to.be.bignumber.equal(expectedFillableAmount);
             expect(calculatedFillableAmountPlusFees).to.be.bignumber.lessThan(transferrableMakerTokenAmount);
             expect(calculatedFillableAmountPlusFees).to.be.bignumber.lessThan(remainingMakerTokenAmount);
+            expect(calculatedFillableAmount).to.be.bignumber.equal(expectedFillableAmount);
+            expect(numberOfFillsInRatio.decimalPlaces()).to.be.equal(0);
         });
     });
 });