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') 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') 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') 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 badbaa39fc000e6febced9d8d0118565af7648dc Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 17:46:55 +0200 Subject: Address feedback --- src/contract_wrappers/exchange_wrapper.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index e4d360690..af8b8db24 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -18,7 +18,8 @@ import { CreateContractEvent, ContractEventObj, EventCallback, - ContractResponse, OrderCancellationRequest, + ContractResponse, + OrderCancellationRequest, } from '../types'; import {assert} from '../utils/assert'; import {utils} from '../utils/utils'; @@ -187,16 +188,18 @@ export class ExchangeWrapper extends ContractWrapper { */ public async batchCancelOrderAsync(cancellationRequestsBatch: OrderCancellationRequest[]): Promise { const makers = _.map(cancellationRequestsBatch, cancellationRequest => cancellationRequest.order.maker); - assert.assert(!_.isEmpty(cancellationRequestsBatch), 'Can not cancel an empty batch'); - assert.assert(_.uniq(makers).length === 1, 'Can not cancel orders from multiple makers in a single batch'); + if (_.isEmpty(cancellationRequestsBatch)) { + return; + } + assert.assert(_.uniq(makers).length === 1, ExchangeContractErrs.MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH); const maker = makers[0]; + await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'maker', maker); _.forEach(cancellationRequestsBatch, - async (cancellationRequest: OrderCancellationRequest) => { - assert.doesConformToSchema('order', + async (cancellationRequest: OrderCancellationRequest, i: number) => { + assert.doesConformToSchema(`orderCancellationRequests[${i}].order`, SchemaValidator.convertToJSONSchemaCompatibleObject(cancellationRequest.order as object), orderSchema); - assert.isBigNumber('takerTokenCancelAmount', cancellationRequest.takerTokenCancelAmount); - await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'order.maker', - cancellationRequest.order.maker); + assert.isBigNumber(`orderCancellationRequests[${i}].takerTokenCancelAmount`, + cancellationRequest.takerTokenCancelAmount); await this.validateCancelOrderAndThrowIfInvalidAsync( cancellationRequest.order, cancellationRequest.takerTokenCancelAmount); }); -- 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') 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 From 007102f2de38b59d54debd849719c525972ef2b4 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 8 Jun 2017 10:51:13 +0200 Subject: Don't use batch function for normal one --- src/contract_wrappers/exchange_wrapper.ts | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 47c49fad6..7ffcd824c 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -229,10 +229,34 @@ export class ExchangeWrapper extends ContractWrapper { */ public async cancelOrderAsync( order: Order|SignedOrder, takerTokenCancelAmount: BigNumber.BigNumber): Promise { - await this.batchCancelOrderAsync([{ - order, + assert.doesConformToSchema('order', + SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), + orderSchema); + assert.isBigNumber('takerTokenCancelAmount', takerTokenCancelAmount); + await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'order.maker', order.maker); + + const exchangeInstance = await this.getExchangeContractAsync(); + await this.validateCancelOrderAndThrowIfInvalidAsync(order, takerTokenCancelAmount); + + const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order); + const gas = await exchangeInstance.cancel.estimateGas( + orderAddresses, + orderValues, + takerTokenCancelAmount, + { + from: order.maker, + }, + ); + const response: ContractResponse = await exchangeInstance.cancel( + orderAddresses, + orderValues, takerTokenCancelAmount, - }]); + { + from: order.maker, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); } /** * Batch version of cancelOrderAsync. Atomically cancels multiple orders in a single transaction. -- cgit v1.2.3 From 5ba4cd22e3a14f60666f4092ab00a012ff7786a4 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 8 Jun 2017 10:58:14 +0200 Subject: Address feedback --- src/contract_wrappers/exchange_wrapper.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 7ffcd824c..0e0fb2f6f 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -260,16 +260,17 @@ export class ExchangeWrapper extends ContractWrapper { } /** * Batch version of cancelOrderAsync. Atomically cancels multiple orders in a single transaction. + * All orders must be from the same maker. */ - public async batchCancelOrderAsync(cancellationRequestsBatch: OrderCancellationRequest[]): Promise { - const makers = _.map(cancellationRequestsBatch, cancellationRequest => cancellationRequest.order.maker); - if (_.isEmpty(cancellationRequestsBatch)) { + public async batchCancelOrderAsync(orderCancellationRequests: OrderCancellationRequest[]): Promise { + const makers = _.map(orderCancellationRequests, cancellationRequest => cancellationRequest.order.maker); + if (_.isEmpty(orderCancellationRequests)) { return; } assert.assert(_.uniq(makers).length === 1, ExchangeContractErrs.MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH); const maker = makers[0]; await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'maker', maker); - _.forEach(cancellationRequestsBatch, + _.forEach(orderCancellationRequests, async (cancellationRequest: OrderCancellationRequest, i: number) => { assert.doesConformToSchema(`orderCancellationRequests[${i}].order`, SchemaValidator.convertToJSONSchemaCompatibleObject(cancellationRequest.order as object), orderSchema); @@ -279,7 +280,7 @@ export class ExchangeWrapper extends ContractWrapper { cancellationRequest.order, cancellationRequest.takerTokenCancelAmount); }); const exchangeInstance = await this.getExchangeContractAsync(); - const orderAddressesValuesAndTakerTokenCancelAmounts = _.map(cancellationRequestsBatch, cancellationRequest => { + const orderAddressesValuesAndTakerTokenCancelAmounts = _.map(orderCancellationRequests, cancellationRequest => { return [ ...ExchangeWrapper.getOrderAddressesAndValues(cancellationRequest.order), cancellationRequest.takerTokenCancelAmount, -- cgit v1.2.3 From 32ba65ee265d6acc3fe8ff87a5a36df36cd9a3d5 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 8 Jun 2017 11:16:06 +0200 Subject: Address feedback --- src/contract_wrappers/exchange_wrapper.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 0e0fb2f6f..d144d8aad 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -263,21 +263,24 @@ export class ExchangeWrapper extends ContractWrapper { * All orders must be from the same maker. */ public async batchCancelOrderAsync(orderCancellationRequests: OrderCancellationRequest[]): Promise { - const makers = _.map(orderCancellationRequests, cancellationRequest => cancellationRequest.order.maker); if (_.isEmpty(orderCancellationRequests)) { - return; + return; // no-op } + const makers = _.map(orderCancellationRequests, cancellationRequest => cancellationRequest.order.maker); assert.assert(_.uniq(makers).length === 1, ExchangeContractErrs.MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH); const maker = makers[0]; await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'maker', maker); _.forEach(orderCancellationRequests, async (cancellationRequest: OrderCancellationRequest, i: number) => { assert.doesConformToSchema(`orderCancellationRequests[${i}].order`, - SchemaValidator.convertToJSONSchemaCompatibleObject(cancellationRequest.order as object), orderSchema); + SchemaValidator.convertToJSONSchemaCompatibleObject(cancellationRequest.order as object), orderSchema, + ); assert.isBigNumber(`orderCancellationRequests[${i}].takerTokenCancelAmount`, - cancellationRequest.takerTokenCancelAmount); + cancellationRequest.takerTokenCancelAmount, + ); await this.validateCancelOrderAndThrowIfInvalidAsync( - cancellationRequest.order, cancellationRequest.takerTokenCancelAmount); + cancellationRequest.order, cancellationRequest.takerTokenCancelAmount, + ); }); const exchangeInstance = await this.getExchangeContractAsync(); const orderAddressesValuesAndTakerTokenCancelAmounts = _.map(orderCancellationRequests, cancellationRequest => { @@ -286,7 +289,7 @@ export class ExchangeWrapper extends ContractWrapper { cancellationRequest.takerTokenCancelAmount, ]; }); - // _.unzip doesn't type check if values have different types :'( + // We use _.unzip because _.unzip doesn't type check if values have different types :'( const [orderAddresses, orderValues, takerTokenCancelAmounts] = _.unzip(orderAddressesValuesAndTakerTokenCancelAmounts); const gas = await exchangeInstance.batchCancel.estimateGas( -- cgit v1.2.3