From 8b7caef0db337a5f1b9bf41ee5f8a9157eda4d1d Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 4 Oct 2017 14:21:39 +0300 Subject: Fix an issue when validation failed, but contract call will succeed --- src/utils/order_validation_utils.ts | 6 +++++- test/order_validation_test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts index 6f7522c41..de20cf06d 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); + const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress; + let makerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, signedOrder.maker); + if (isTakerTokenZRX) { + makerZRXBalance = makerZRXBalance.plus(fillTakerAmount); + } const makerZRXAllowance = await this.tokenWrapper.getProxyAllowanceAsync( zrxTokenAddress, signedOrder.maker); diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts index f625433eb..9e6817c8b 100644 --- a/test/order_validation_test.ts +++ b/test/order_validation_test.ts @@ -356,5 +356,37 @@ describe('OrderValidation', () => { )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance); }); }); + describe('should not throw if user doesn\'t have sufficient ZRX to pay the fees, but will after a transfer', + () => { + 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); + }); + }); }); }); -- cgit From 0caab98399544d93891d11a2f25309f92433f3c1 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 4 Oct 2017 14:58:08 +0300 Subject: Fi fees validation is one of the tokens transfered is 0x --- src/utils/order_validation_utils.ts | 9 ++- test/order_validation_test.ts | 142 +++++++++++++++++++++++++----------- 2 files changed, 107 insertions(+), 44 deletions(-) diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts index de20cf06d..160afc43e 100644 --- a/src/utils/order_validation_utils.ts +++ b/src/utils/order_validation_utils.ts @@ -145,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)) { @@ -155,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 9e6817c8b..8f3e6ce0b 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,21 +412,21 @@ describe('OrderValidation', () => { )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerAllowance); }); }); - describe('should not throw if user doesn\'t have sufficient ZRX to pay the fees, but will after a transfer', + describe('should correctly validate fees amounts if maker 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 () => { + 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( - makerTokenAddress, zrxTokenAddress, makerFee, takerFee, + zrxTokenAddress, takerTokenAddress, makerFee, takerFee, makerAddress, takerAddress, fillableAmount, feeRecipient, ); - txHash = await zeroEx.token.transferAsync(zrxTokenAddress, makerAddress, coinbase, makerFee); + txHash = await zeroEx.token.transferAsync(zrxTokenAddress, takerAddress, coinbase, takerFee); await zeroEx.awaitTransactionMinedAsync(txHash); - await (orderValidationUtils as any).validateFillOrderMakerBalancesAllowancesThrowIfInvalidAsync( - signedOrder, fillTakerAmount, zrxTokenAddress, + 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 () => { @@ -380,11 +436,11 @@ describe('OrderValidation', () => { makerTokenAddress, zrxTokenAddress, makerFee, takerFee, makerAddress, takerAddress, fillableAmount, feeRecipient, ); - txHash = await zeroEx.token.transferAsync(zrxTokenAddress, makerAddress, coinbase, makerFee); + txHash = await zeroEx.token.transferAsync(zrxTokenAddress, takerAddress, coinbase, takerFee); await zeroEx.awaitTransactionMinedAsync(txHash); return expect( - (orderValidationUtils as any).validateFillOrderMakerBalancesAllowancesThrowIfInvalidAsync( - signedOrder, fillTakerAmount, zrxTokenAddress, + (orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync( + signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress, )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerFeeBalance); }); }); -- cgit From 074040daf5d30442669426ed45dc08056172c8c1 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 4 Oct 2017 15:01:51 +0300 Subject: Add changes to CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3ce29f82..43377c3e6 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 transfered is ZRX (#181) v0.19.0 - _September 29, 2017_ * Made order validation optional (#172) -- cgit From b4717b85264628babe4b3c463f6bbe715c03e7a0 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 4 Oct 2017 15:06:38 +0300 Subject: Fix tests --- test/order_validation_test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/order_validation_test.ts b/test/order_validation_test.ts index 8f3e6ce0b..c1a0a6c8c 100644 --- a/test/order_validation_test.ts +++ b/test/order_validation_test.ts @@ -433,7 +433,7 @@ describe('OrderValidation', () => { const makerFee = fillableAmount.plus(1); const takerFee = fillableAmount.plus(1); signedOrder = await fillScenarios.createFillableSignedOrderWithFeesAsync( - makerTokenAddress, zrxTokenAddress, makerFee, takerFee, + zrxTokenAddress, takerTokenAddress, makerFee, takerFee, makerAddress, takerAddress, fillableAmount, feeRecipient, ); txHash = await zeroEx.token.transferAsync(zrxTokenAddress, takerAddress, coinbase, takerFee); @@ -441,7 +441,7 @@ describe('OrderValidation', () => { return expect( (orderValidationUtils as any).validateFillOrderTakerBalancesAllowancesThrowIfInvalidAsync( signedOrder, fillTakerAmount, takerAddress, zrxTokenAddress, - )).to.be.rejectedWith(ExchangeContractErrs.InsufficientMakerFeeBalance); + )).to.be.rejectedWith(ExchangeContractErrs.InsufficientTakerFeeBalance); }); }); }); -- cgit From cd16b35814a0f5e7708aa95ff30736c569800cb1 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 5 Oct 2017 09:46:09 +0300 Subject: Fix a typo --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43377c3e6..7c6b9ae62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +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 transfered is ZRX (#181) + * Fixed fees validation when one of the tokens transferred is ZRX (#181) v0.19.0 - _September 29, 2017_ * Made order validation optional (#172) -- cgit From 0594667d36e1551f17ff8b409c47df196f9b1072 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 5 Oct 2017 09:46:54 +0300 Subject: Small reordering --- src/utils/order_validation_utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/order_validation_utils.ts b/src/utils/order_validation_utils.ts index 160afc43e..0450d26a6 100644 --- a/src/utils/order_validation_utils.ts +++ b/src/utils/order_validation_utils.ts @@ -121,8 +121,8 @@ export class OrderValidationUtils { } if (!isMakerTokenZRX) { - const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress; let makerZRXBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, signedOrder.maker); + const isTakerTokenZRX = signedOrder.takerTokenAddress === zrxTokenAddress; if (isTakerTokenZRX) { makerZRXBalance = makerZRXBalance.plus(fillTakerAmount); } -- cgit