Merge pull request #197 from 0xProject/fix/rounding

Fix rounding of maker fill amount and correctly validate partial fees

3 files changed, 72 insertions, 11 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5a60daca4..1148929e5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # CHANGELOG
 
+v0.22.2 - _October 24, 2017_
+------------------------
+    * Fixed rounding of maker fill amount and incorrect validation of partial fees (#197)
+
 v0.22.0 - _October 16, 2017_
 ------------------------
     * Started using `OrderFillRequest` interface instead of `OrderFillOrKillRequest` interface for `zeroEx.exchange.batchFillOrKill` (#187)
diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts
index 1d9aac884..b7eae7e2f 100644
--- a/src/utils/order_validation_utils.ts
+++ b/src/utils/order_validation_utils.ts
@@ -27,13 +27,22 @@ export class OrderValidationUtils {
         if (!_.isUndefined(expectedFillTakerTokenAmount)) {
             fillTakerTokenAmount = expectedFillTakerTokenAmount;
         }
-        const fillMakerTokenAmount = this.getFillMakerTokenAmount(signedOrder, fillTakerTokenAmount);
+        const fillMakerTokenAmount = this.getPartialAmount(
+            fillTakerTokenAmount,
+            signedOrder.takerTokenAmount,
+            signedOrder.makerTokenAmount,
+        );
         await exchangeTradeEmulator.transferFromAsync(
             signedOrder.makerTokenAddress, signedOrder.maker, signedOrder.taker, fillMakerTokenAmount,
             TradeSide.Maker, TransferType.Trade,
         );
+        const makerFeeAmount = this.getPartialAmount(
+            fillTakerTokenAmount,
+            signedOrder.takerTokenAmount,
+            signedOrder.makerFee,
+        );
         await exchangeTradeEmulator.transferFromAsync(
-            zrxTokenAddress, signedOrder.maker, signedOrder.feeRecipient, signedOrder.makerFee,
+            zrxTokenAddress, signedOrder.maker, signedOrder.feeRecipient, makerFeeAmount,
             TradeSide.Maker, TransferType.Fee,
         );
     }
@@ -100,7 +109,11 @@ export class OrderValidationUtils {
     public async validateFillOrderBalancesAllowancesThrowIfInvalidAsync(
         exchangeTradeEmulator: ExchangeTransferSimulator, signedOrder: SignedOrder,
         fillTakerTokenAmount: BigNumber.BigNumber, senderAddress: string, zrxTokenAddress: string): Promise<void> {
-        const fillMakerTokenAmount = this.getFillMakerTokenAmount(signedOrder, fillTakerTokenAmount);
+        const fillMakerTokenAmount = this.getPartialAmount(
+            fillTakerTokenAmount,
+            signedOrder.takerTokenAmount,
+            signedOrder.makerTokenAmount,
+        );
         await exchangeTradeEmulator.transferFromAsync(
             signedOrder.makerTokenAddress, signedOrder.maker, senderAddress, fillMakerTokenAmount,
             TradeSide.Maker, TransferType.Trade,
@@ -109,12 +122,22 @@ export class OrderValidationUtils {
             signedOrder.takerTokenAddress, senderAddress, signedOrder.maker, fillTakerTokenAmount,
             TradeSide.Taker, TransferType.Trade,
         );
+        const makerFeeAmount = this.getPartialAmount(
+            fillTakerTokenAmount,
+            signedOrder.takerTokenAmount,
+            signedOrder.makerFee,
+        );
         await exchangeTradeEmulator.transferFromAsync(
-            zrxTokenAddress, signedOrder.maker, signedOrder.feeRecipient, signedOrder.makerFee, TradeSide.Maker,
+            zrxTokenAddress, signedOrder.maker, signedOrder.feeRecipient, makerFeeAmount, TradeSide.Maker,
             TransferType.Fee,
         );
+        const takerFeeAmount = this.getPartialAmount(
+            fillTakerTokenAmount,
+            signedOrder.takerTokenAmount,
+            signedOrder.takerFee,
+        );
         await exchangeTradeEmulator.transferFromAsync(
-            zrxTokenAddress, senderAddress, signedOrder.feeRecipient, signedOrder.takerFee, TradeSide.Taker,
+            zrxTokenAddress, senderAddress, signedOrder.feeRecipient, takerFeeAmount, TradeSide.Taker,
             TransferType.Fee,
         );
     }
@@ -131,10 +154,12 @@ export class OrderValidationUtils {
             throw new Error(ExchangeContractErrs.OrderFillExpired);
         }
     }
-    private getFillMakerTokenAmount(signedOrder: Order,
-                                    fillTakerTokenAmount: BigNumber.BigNumber): BigNumber.BigNumber {
-        const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount);
-        const fillMakerTokenAmount = fillTakerTokenAmount.div(exchangeRate);
+    private getPartialAmount(numerator: BigNumber.BigNumber, denominator: BigNumber.BigNumber,
+                             target: BigNumber.BigNumber): BigNumber.BigNumber {
+        const fillMakerTokenAmount = numerator
+                                     .mul(target)
+                                     .div(denominator)
+                                     .round(0);
         return fillMakerTokenAmount;
     }
 }
diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts
index 784fa9ec4..dfcf1d43d 100644
--- a/test/order_validation_test.ts
+++ b/test/order_validation_test.ts
@@ -210,14 +210,14 @@ describe('OrderValidation', () => {
     });
     describe('#validateFillOrderBalancesAllowancesThrowIfInvalidAsync', () => {
         let exchangeTransferSimulator: ExchangeTransferSimulator;
-        let transferFromAsync: any;
+        let transferFromAsync: Sinon.SinonSpy;
         const bigNumberMatch = (expected: BigNumber.BigNumber) => {
             return Sinon.match((value: BigNumber.BigNumber) => value.eq(expected));
         };
         beforeEach('create exchangeTransferSimulator', async () => {
             exchangeTransferSimulator = new ExchangeTransferSimulator(zeroEx.token);
             transferFromAsync = Sinon.spy();
-            exchangeTransferSimulator.transferFromAsync = transferFromAsync;
+            exchangeTransferSimulator.transferFromAsync = transferFromAsync as any;
         });
         it('should call exchangeTransferSimulator.transferFrom in a correct order', async () => {
             const makerFee = new BigNumber(2);
@@ -291,5 +291,37 @@ describe('OrderValidation', () => {
                 ),
             ).to.be.true();
         });
+        it('should correctly round the fillMakerTokenAmount', async () => {
+            const makerTokenAmount = new BigNumber(3);
+            const takerTokenAmount = new BigNumber(1);
+            const signedOrder = await fillScenarios.createAsymmetricFillableSignedOrderAsync(
+                makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, makerTokenAmount, takerTokenAmount,
+            );
+            await orderValidationUtils.validateFillOrderBalancesAllowancesThrowIfInvalidAsync(
+                exchangeTransferSimulator, signedOrder, takerTokenAmount, takerAddress, zrxTokenAddress,
+            );
+            expect(transferFromAsync.callCount).to.be.equal(4);
+            const makerFillAmount = transferFromAsync.getCall(0).args[3];
+            expect(makerFillAmount).to.be.bignumber.equal(makerTokenAmount);
+        });
+        it('should correctly round the makerFeeAmount', async () => {
+            const makerFee = new BigNumber(2);
+            const takerFee = new BigNumber(4);
+            const signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync(
+                makerTokenAddress, takerTokenAddress, makerFee, takerFee, makerAddress, takerAddress,
+                fillableAmount, ZeroEx.NULL_ADDRESS,
+            );
+            const fillTakerTokenAmount = fillableAmount.div(2).round(0);
+            await orderValidationUtils.validateFillOrderBalancesAllowancesThrowIfInvalidAsync(
+                exchangeTransferSimulator, signedOrder, fillTakerTokenAmount, takerAddress, zrxTokenAddress,
+            );
+            const makerPartialFee = makerFee.div(2);
+            const takerPartialFee = takerFee.div(2);
+            expect(transferFromAsync.callCount).to.be.equal(4);
+            const partialMakerFee = transferFromAsync.getCall(2).args[3];
+            expect(partialMakerFee).to.be.bignumber.equal(makerPartialFee);
+            const partialTakerFee = transferFromAsync.getCall(3).args[3];
+            expect(partialTakerFee).to.be.bignumber.equal(takerPartialFee);
+        });
     });
 });