From 15628a1206d3d694d6d6852157a4e7f2fa799017 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Wed, 22 Nov 2017 10:43:38 +1100 Subject: Add a test for when the ratio is < 1 --- .../order_watcher/remaining_fillable_calculator.ts | 98 ++++++++++++---------- .../test/remaining_fillable_calculator_test.ts | 58 +++++++++++-- 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); }); }); }); -- cgit