From 9d0ccdfdcc2ce962d76c406b83b15e520a971eae Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Mon, 23 Oct 2017 10:46:33 +0300 Subject: Add tests testing that rounding or makerFillAmount is correct and that we only validate partial fees --- test/order_validation_test.ts | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts index 784fa9ec4..07a050fea 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,34 @@ 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); + expect(transferFromAsync.getCall(0).args[3]).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); + expect(transferFromAsync.getCall(2).args[3]).to.be.bignumber.equal(makerPartialFee); + expect(transferFromAsync.getCall(4).args[3]).to.be.bignumber.equal(takerPartialFee); + }); }); }); -- cgit v1.2.3 From fbe34663da84ee798bb39bfd2144ff16a712413e Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Mon, 23 Oct 2017 10:47:03 +0300 Subject: Fix the rounding of makerFillAmount and correctly validate partial fees --- src/utils/order_validation_utils.ts | 43 +++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 9 deletions(-) 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 { - 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; } } -- cgit v1.2.3 From 89103c40fb7d4a1da1f8a3187a2e7732867a99c6 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Mon, 23 Oct 2017 11:09:25 +0300 Subject: Use more meaningful variable names --- test/order_validation_test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts index 07a050fea..dd8b44e1f 100644 --- a/test/order_validation_test.ts +++ b/test/order_validation_test.ts @@ -301,7 +301,8 @@ describe('OrderValidation', () => { exchangeTransferSimulator, signedOrder, takerTokenAmount, takerAddress, zrxTokenAddress, ); expect(transferFromAsync.callCount).to.be.equal(4); - expect(transferFromAsync.getCall(0).args[3]).to.be.bignumber.equal(makerTokenAmount); + 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); @@ -317,8 +318,10 @@ describe('OrderValidation', () => { const makerPartialFee = makerFee.div(2); const takerPartialFee = takerFee.div(2); expect(transferFromAsync.callCount).to.be.equal(4); - expect(transferFromAsync.getCall(2).args[3]).to.be.bignumber.equal(makerPartialFee); - expect(transferFromAsync.getCall(4).args[3]).to.be.bignumber.equal(takerPartialFee); + const partialMakerFee = transferFromAsync.getCall(2).args[3]; + expect(partialMakerFee).to.be.bignumber.equal(makerPartialFee); + const partialTakerFee = transferFromAsync.getCall(4).args[3]; + expect(partialTakerFee).to.be.bignumber.equal(takerPartialFee); }); }); }); -- cgit v1.2.3 From 0c6be942221230fae42a61e1694d7580674d4e8c Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Mon, 23 Oct 2017 11:24:14 +0300 Subject: Fix index --- test/order_validation_test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts index dd8b44e1f..dfcf1d43d 100644 --- a/test/order_validation_test.ts +++ b/test/order_validation_test.ts @@ -320,7 +320,7 @@ describe('OrderValidation', () => { 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(4).args[3]; + const partialTakerFee = transferFromAsync.getCall(3).args[3]; expect(partialTakerFee).to.be.bignumber.equal(takerPartialFee); }); }); -- cgit v1.2.3 From 1fd8c2a6e28150cb7825cf79b0d46c1db9286053 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Tue, 24 Oct 2017 18:47:22 +0300 Subject: Update CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) 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) -- cgit v1.2.3