From 918315e89f3408124d2e78bbd1acb58ed42d1766 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 12:40:58 +0200 Subject: Implement fillOrKill & tests --- src/contract_wrappers/exchange_wrapper.ts | 105 +++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 17 deletions(-) (limited to 'src/contract_wrappers/exchange_wrapper.ts') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d3a53a9f7..55ff9068e 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -7,6 +7,7 @@ import { ExchangeContract, ExchangeContractErrCodes, ExchangeContractErrs, + Order, OrderValues, OrderAddresses, SignedOrder, @@ -126,21 +127,9 @@ export class ExchangeWrapper extends ContractWrapper { const exchangeInstance = await this.getExchangeContractAsync(); await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); - const orderAddresses: OrderAddresses = [ - signedOrder.maker, - signedOrder.taker, - signedOrder.makerTokenAddress, - signedOrder.takerTokenAddress, - signedOrder.feeRecipient, - ]; - const orderValues: OrderValues = [ - signedOrder.makerTokenAmount, - signedOrder.takerTokenAmount, - signedOrder.makerFee, - signedOrder.takerFee, - signedOrder.expirationUnixTimestampSec, - signedOrder.salt, - ]; + const orderAddresses = this.getOrderAddresses(signedOrder); + const orderValues = this.getOrderValues(signedOrder); + const gas = await exchangeInstance.fill.estimateGas( orderAddresses, orderValues, @@ -168,6 +157,67 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } + /** + * Attempts to fill a specific amount of an order. If the entire amount specified cannot be filled, + * the fill order is abandoned. + */ + public async fillOrKillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, + shouldCheckTransfer: boolean, takerAddress: string) { + assert.doesConformToSchema('signedOrder', + SchemaValidator.convertToJSONSchemaCompatibleObject(signedOrder as object), + signedOrderSchema); + assert.isBigNumber('fillTakerAmount', fillTakerAmount); + assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); + await assert.isSenderAddressAsync('takerAddress', takerAddress, this.web3Wrapper); + + const exchangeInstance = await this.getExchangeContractAsync(); + await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); + + // Check that fillValue available >= fillTakerAmount + const orderHashHex = utils.getOrderHashHex(signedOrder, exchangeInstance.address); + const unavailableTakerAmount = await this.getUnavailableTakerAmountAsync(orderHashHex); + const remainingTakerAmount = signedOrder.takerTokenAmount.minus(unavailableTakerAmount); + if (remainingTakerAmount < fillTakerAmount) { + throw new Error(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); + } + + const orderAddresses = this.getOrderAddresses(signedOrder); + const orderValues = this.getOrderValues(signedOrder); + + const gas = await exchangeInstance.fillOrKill.estimateGas( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + }, + ); + try { + const response: ContractResponse = await exchangeInstance.fillOrKill( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); + } catch (err) { + // There is a potential race condition where when the cancellation is broadcasted, a sufficient + // fillAmount is available, but by the time the transaction gets mined, it no longer is. Instead of + // throwing an invalid jump exception, we would rather give the user a more helpful error message. + if (_.includes(err, constants.INVALID_JUMP_IDENTIFIER)) { + throw new Error(ZeroExError.INSUFFICIENT_REMAINING_FILL_AMOUNT); + } + } + } /** * Subscribe to an event type emitted by the Exchange smart contract */ @@ -232,8 +282,8 @@ export class ExchangeWrapper extends ContractWrapper { * Handling the edge-cases that arise when this happens would require making sure that the user has sufficient * funds to pay both the fees and the transfer amount. We decided to punt on this for now as the contracts * will throw for these edge-cases. - * TODO: Throw errors before calling the smart contract for these edge-cases - * TODO: in order to minimize the callers gas costs. + * TODO: Throw errors before calling the smart contract for these edge-cases in order to minimize + * the callers gas costs. */ private async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, @@ -316,4 +366,25 @@ export class ExchangeWrapper extends ContractWrapper { const exchangeInstance = await this.getExchangeContractAsync(); return exchangeInstance.ZRX.call(); } + private getOrderAddresses(order: Order|SignedOrder) { + const orderAddresses: OrderAddresses = [ + order.maker, + order.taker, + order.makerTokenAddress, + order.takerTokenAddress, + order.feeRecipient, + ]; + return orderAddresses; + } + private getOrderValues(order: Order|SignedOrder) { + const orderValues: OrderValues = [ + order.makerTokenAmount, + order.takerTokenAmount, + order.makerFee, + order.takerFee, + order.expirationUnixTimestampSec, + order.salt, + ]; + return orderValues; + } } -- cgit v1.2.3 From 9ae2e0c1aec5ac989eca652aa8c329114bcffccf Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 13:06:01 +0200 Subject: remove unused arg --- src/contract_wrappers/exchange_wrapper.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/contract_wrappers/exchange_wrapper.ts') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index c24a518a4..f6451a3cc 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -178,12 +178,11 @@ export class ExchangeWrapper extends ContractWrapper { * the fill order is abandoned. */ public async fillOrKillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, - shouldCheckTransfer: boolean, takerAddress: string) { + takerAddress: string) { assert.doesConformToSchema('signedOrder', SchemaValidator.convertToJSONSchemaCompatibleObject(signedOrder as object), signedOrderSchema); assert.isBigNumber('fillTakerAmount', fillTakerAmount); - assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); await assert.isSenderAddressAsync('takerAddress', takerAddress, this.web3Wrapper); const exchangeInstance = await this.getExchangeContractAsync(); -- cgit v1.2.3 From bc441015b672c310bd4b4a67fcf9a98e28793883 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:23:59 +0200 Subject: add `hex` to function and variable name for clarity --- src/contract_wrappers/exchange_wrapper.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/contract_wrappers/exchange_wrapper.ts') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index f6451a3cc..d25b8aa29 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -292,11 +292,11 @@ export class ExchangeWrapper extends ContractWrapper { logEventObj.watch(callback); this.exchangeLogEventObjs.push(logEventObj); } - private async getOrderHashAsync(order: Order|SignedOrder): Promise { + private async getOrderHashHexAsync(order: Order|SignedOrder): Promise { const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order); const exchangeInstance = await this.getExchangeContractAsync(); - const orderHash = utils.getOrderHashHex(order, exchangeInstance.address); - return orderHash; + const orderHashHex = utils.getOrderHashHex(order, exchangeInstance.address); + return orderHashHex; } private async stopWatchingExchangeLogEventsAsync() { const stopWatchingPromises = _.map(this.exchangeLogEventObjs, logEventObj => { @@ -334,7 +334,7 @@ export class ExchangeWrapper extends ContractWrapper { if (takerTokenCancelAmount.eq(0)) { throw new Error(ExchangeContractErrs.ORDER_CANCEL_AMOUNT_ZERO); } - const orderHash = await this.getOrderHashAsync(order); + const orderHash = await this.getOrderHashHexAsync(order); const unavailableAmount = await this.getUnavailableTakerAmountAsync(orderHash); if (order.takerTokenAmount.minus(unavailableAmount).eq(0)) { throw new Error(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED); -- cgit v1.2.3 From 9e855b4c6ad3baef8be3a725ca26b679c2542c00 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:53:52 +0200 Subject: Remove catch of invalid jump throws since there are many reasons the contracts could throw this error --- src/contract_wrappers/exchange_wrapper.ts | 38 ++++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-) (limited to 'src/contract_wrappers/exchange_wrapper.ts') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d25b8aa29..4f132656e 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -189,7 +189,7 @@ export class ExchangeWrapper extends ContractWrapper { await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); // Check that fillValue available >= fillTakerAmount - const orderHashHex = utils.getOrderHashHex(signedOrder, exchangeInstance.address); + const orderHashHex = await this.getOrderHashHexAsync(signedOrder); const unavailableTakerAmount = await this.getUnavailableTakerAmountAsync(orderHashHex); const remainingTakerAmount = signedOrder.takerTokenAmount.minus(unavailableTakerAmount); if (remainingTakerAmount < fillTakerAmount) { @@ -209,28 +209,19 @@ export class ExchangeWrapper extends ContractWrapper { from: takerAddress, }, ); - try { - const response: ContractResponse = await exchangeInstance.fillOrKill( - orderAddresses, - orderValues, - fillTakerAmount, - signedOrder.ecSignature.v, - signedOrder.ecSignature.r, - signedOrder.ecSignature.s, - { - from: takerAddress, - gas, - }, - ); - this.throwErrorLogsAsErrors(response.logs); - } catch (err) { - // There is a potential race condition where when the cancellation is broadcasted, a sufficient - // fillAmount is available, but by the time the transaction gets mined, it no longer is. Instead of - // throwing an invalid jump exception, we would rather give the user a more helpful error message. - if (_.includes(err, constants.INVALID_JUMP_IDENTIFIER)) { - throw new Error(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); - } - } + const response: ContractResponse = await exchangeInstance.fillOrKill( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); } /** * Cancel a given fill amount of an order. Cancellations are cumulative. @@ -293,7 +284,6 @@ export class ExchangeWrapper extends ContractWrapper { this.exchangeLogEventObjs.push(logEventObj); } private async getOrderHashHexAsync(order: Order|SignedOrder): Promise { - const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order); const exchangeInstance = await this.getExchangeContractAsync(); const orderHashHex = utils.getOrderHashHex(order, exchangeInstance.address); return orderHashHex; -- cgit v1.2.3