From 9325bd8cb4d3b0f3bbf241a52bd60cda77972236 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 12:39:44 +0200 Subject: Add initial batchCancelAsync implementation --- src/contract_wrappers/exchange_wrapper.ts | 45 +++++++++++++++++++++++++++++++ src/types.ts | 6 +++++ src/utils/assert.ts | 6 +++++ 3 files changed, 57 insertions(+) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 6f62934dc..b0c4295f1 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -205,6 +205,51 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } + /** + * Batch version of cancelOrderAsync. Atomically cancels multiple orders in a single transaction. + */ + public async batchCancelOrderAsync( + orders: Array, takerTokenCancelAmounts: BigNumber.BigNumber[]): Promise { + const makers = _.map(orders, order => order.maker); + assert.isSameLength('orders', orders, 'takerTokenCancelAmounts', takerTokenCancelAmounts); + assert.assert(_.isEmpty(orders), 'Can not cancel an empty batch'); + assert.assert(_.uniq(makers).length === 1, 'Can not cancel orders from multiple makers in a single batch'); + const maker = makers[0]; + // _.zip doesn't type check if values have different types :'( + const ordersAndTakerTokenCancelAmounts = _.zip(orders, takerTokenCancelAmounts); + _.forEach(ordersAndTakerTokenCancelAmounts, + async ([order, takerTokenCancelAmount]: [Order|SignedOrder, BigNumber.BigNumber]) => { + assert.doesConformToSchema('order', + SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), orderSchema); + assert.isBigNumber('takerTokenCancelAmount', takerTokenCancelAmount); + await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'order.maker', order.maker); + await this.validateCancelOrderAndThrowIfInvalidAsync(order, takerTokenCancelAmount); + }); + const exchangeInstance = await this.getExchangeContractAsync(); + const orderAddressesAndValues = _.map(orders, order => { + return ExchangeWrapper.getOrderAddressesAndValues(order); + }); + // _.unzip doesn't type check if values have different types :'( + const [orderAddresses, orderValues] = _.unzip(orderAddressesAndValues); + const gas = await exchangeInstance.batchCancel.estimateGas( + orderAddresses, + orderValues, + takerTokenCancelAmounts, + { + from: maker, + }, + ); + const response: ContractResponse = await exchangeInstance.batchCancel( + orderAddresses, + orderValues, + takerTokenCancelAmounts, + { + from: maker, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); + } /** * Subscribe to an event type emitted by the Exchange smart contract */ diff --git a/src/types.ts b/src/types.ts index 5407b0121..6c217fe51 100644 --- a/src/types.ts +++ b/src/types.ts @@ -74,6 +74,12 @@ export interface ExchangeContract extends ContractInstance { estimateGas: (orderAddresses: OrderAddresses, orderValues: OrderValues, cancelAmount: BigNumber.BigNumber, txOpts?: TxOpts) => number; }; + batchCancel: { + (orderAddresses: OrderAddresses[], orderValues: OrderValues[], cancelAmount: BigNumber.BigNumber[], + txOpts?: TxOpts): ContractResponse; + estimateGas: (orderAddresses: OrderAddresses[], orderValues: OrderValues[], cancelAmount: BigNumber.BigNumber[], + txOpts?: TxOpts) => number; + }; filled: { call: (orderHash: string) => BigNumber.BigNumber; }; diff --git a/src/utils/assert.ts b/src/utils/assert.ts index 4dc6945a2..3842185bb 100644 --- a/src/utils/assert.ts +++ b/src/utils/assert.ts @@ -42,6 +42,12 @@ export const assert = { const availableAddresses = await web3Wrapper.getAvailableAddressesAsync(); this.assert(!_.isEmpty(availableAddresses), 'No addresses were available on the provided web3 instance'); }, + isSameLength(variableName1: string, value1: any[], variableName2: string, value2: any[]) { + const length1 = value1.length; + const length2 = value2.length; + this.assert(length1 === length2, `${variableName1} and ${variableName2} length mismatch. + ${length1} != ${length2}`); + }, isNumber(variableName: string, value: number): void { this.assert(_.isFinite(value), this.typeAssertionMessage(variableName, 'number', value)); }, -- cgit From 91101eb8ec4057617e644df4439c626093d4b3cc Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 13:20:02 +0200 Subject: Add tests for batchCancelAsync --- src/contract_wrappers/exchange_wrapper.ts | 2 +- src/utils/assert.ts | 4 +- test/exchange_wrapper_test.ts | 88 +++++++++++++++++++++++-------- 3 files changed, 68 insertions(+), 26 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index b0c4295f1..fec75e6df 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -212,7 +212,7 @@ export class ExchangeWrapper extends ContractWrapper { orders: Array, takerTokenCancelAmounts: BigNumber.BigNumber[]): Promise { const makers = _.map(orders, order => order.maker); assert.isSameLength('orders', orders, 'takerTokenCancelAmounts', takerTokenCancelAmounts); - assert.assert(_.isEmpty(orders), 'Can not cancel an empty batch'); + assert.assert(!_.isEmpty(orders), 'Can not cancel an empty batch'); assert.assert(_.uniq(makers).length === 1, 'Can not cancel orders from multiple makers in a single batch'); const maker = makers[0]; // _.zip doesn't type check if values have different types :'( diff --git a/src/utils/assert.ts b/src/utils/assert.ts index 3842185bb..61b7527e6 100644 --- a/src/utils/assert.ts +++ b/src/utils/assert.ts @@ -45,8 +45,8 @@ export const assert = { isSameLength(variableName1: string, value1: any[], variableName2: string, value2: any[]) { const length1 = value1.length; const length2 = value2.length; - this.assert(length1 === length2, `${variableName1} and ${variableName2} length mismatch. - ${length1} != ${length2}`); + this.assert(length1 === length2, `${variableName1} and ${variableName2} length mismatch. \ +${length1} != ${length2}`); }, isNumber(variableName: string, value: number): void { this.assert(_.isFinite(value), this.typeAssertionMessage(variableName, 'number', value)); diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index c0068c58a..6487ba98c 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -323,7 +323,7 @@ describe('ExchangeWrapper', () => { }); }); }); - describe('#cancelOrderAsync', () => { + describe('cancel order(s)', () => { let makerTokenAddress: string; let takerTokenAddress: string; let coinbase: string; @@ -343,32 +343,74 @@ describe('ExchangeWrapper', () => { ); orderHashHex = await zeroEx.getOrderHashHexAsync(signedOrder); }); - describe('failed cancels', () => { - it('should throw when cancel amount is zero', async () => { - const zeroCancelAmount = new BigNumber(0); - return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, zeroCancelAmount)) - .to.be.rejectedWith(ExchangeContractErrs.ORDER_CANCEL_AMOUNT_ZERO); + describe('#cancelOrderAsync', () => { + describe('failed cancels', () => { + it('should throw when cancel amount is zero', async () => { + const zeroCancelAmount = new BigNumber(0); + return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, zeroCancelAmount)) + .to.be.rejectedWith(ExchangeContractErrs.ORDER_CANCEL_AMOUNT_ZERO); + }); + it('should throw when order is expired', async () => { + const expirationInPast = new BigNumber(1496826058); // 7th Jun 2017 + const expiredSignedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, + fillableAmount, expirationInPast, + ); + orderHashHex = await zeroEx.getOrderHashHexAsync(expiredSignedOrder); + return expect(zeroEx.exchange.cancelOrderAsync(expiredSignedOrder, cancelAmount)) + .to.be.rejectedWith(ExchangeContractErrs.ORDER_CANCEL_EXPIRED); + }); + it('should throw when order is already cancelled or filled', async () => { + await zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount); + return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount)) + .to.be.rejectedWith(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED); + }); }); - it('should throw when order is expired', async () => { - const expirationInPast = new BigNumber(1496826058); // 7th Jun 2017 - const expiredSignedOrder = await fillScenarios.createFillableSignedOrderAsync( - makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, expirationInPast, + describe('successful cancels', () => { + it('should cancel an order', async () => { + await zeroEx.exchange.cancelOrderAsync(signedOrder, cancelAmount); + const cancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync(orderHashHex); + expect(cancelledAmount).to.be.bignumber.equal(cancelAmount); + }); + }); + }); + describe('#batchCancelOrderAsync', () => { + let anotherSignedOrder: SignedOrder; + let anotherOrderHashHex: string; + beforeEach(async () => { + anotherSignedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, ); - orderHashHex = await zeroEx.getOrderHashHexAsync(expiredSignedOrder); - return expect(zeroEx.exchange.cancelOrderAsync(expiredSignedOrder, cancelAmount)) - .to.be.rejectedWith(ExchangeContractErrs.ORDER_CANCEL_EXPIRED); + anotherOrderHashHex = await zeroEx.getOrderHashHexAsync(anotherSignedOrder); }); - it('should throw when order is already cancelled or filled', async () => { - await zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount); - return expect(zeroEx.exchange.cancelOrderAsync(signedOrder, fillableAmount)) - .to.be.rejectedWith(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED); + describe('failed batch cancels', () => { + it('should throw when length of orders and cancelAmounts mismatch', async () => { + return expect(zeroEx.exchange.batchCancelOrderAsync([signedOrder], [])) + .to.be.rejectedWith('orders and takerTokenCancelAmounts length mismatch. 1 != 0'); + }); + it('should throw when orders are empty', async () => { + return expect(zeroEx.exchange.batchCancelOrderAsync([], [])) + .to.be.rejectedWith('Can not cancel an empty batch'); + }); + it.only('should throw when orders have different makers', async () => { + const signedOrderWithADifferentMaker = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, takerAddress, takerAddress, fillableAmount, + ); + return expect(zeroEx.exchange.batchCancelOrderAsync( + [signedOrder, signedOrderWithADifferentMaker], [cancelAmount, cancelAmount])) + .to.be.rejectedWith('Can not cancel orders from multiple makers in a single batch'); + }); }); - }); - describe('successful cancels', () => { - it('should cancel an order', async () => { - await zeroEx.exchange.cancelOrderAsync(signedOrder, cancelAmount); - const cancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync(orderHashHex); - expect(cancelledAmount).to.be.bignumber.equal(cancelAmount); + describe('successful batch cancels', () => { + it('should cancel a batch of orders', async () => { + await zeroEx.exchange.batchCancelOrderAsync( + [signedOrder, anotherSignedOrder], [cancelAmount, cancelAmount]); + const cancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync(orderHashHex); + const anotherCancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync( + anotherOrderHashHex); + expect(cancelledAmount).to.be.bignumber.equal(cancelAmount); + expect(anotherCancelledAmount).to.be.bignumber.equal(cancelAmount); + }); }); }); }); -- cgit From c3cd5812e61359b18c469f70c9f0240db777e67a Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 14:40:28 +0200 Subject: Refactor to use OrderCancellationRequest --- src/contract_wrappers/exchange_wrapper.ts | 36 ++++++++++++++++--------------- src/types.ts | 5 +++++ src/utils/assert.ts | 6 ------ test/exchange_wrapper_test.ts | 32 +++++++++++++++++---------- 4 files changed, 45 insertions(+), 34 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index fec75e6df..f67a125aa 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -18,7 +18,7 @@ import { CreateContractEvent, ContractEventObj, EventCallback, - ContractResponse, + ContractResponse, OrderCancellationRequest, } from '../types'; import {assert} from '../utils/assert'; import {utils} from '../utils/utils'; @@ -208,29 +208,31 @@ export class ExchangeWrapper extends ContractWrapper { /** * Batch version of cancelOrderAsync. Atomically cancels multiple orders in a single transaction. */ - public async batchCancelOrderAsync( - orders: Array, takerTokenCancelAmounts: BigNumber.BigNumber[]): Promise { - const makers = _.map(orders, order => order.maker); - assert.isSameLength('orders', orders, 'takerTokenCancelAmounts', takerTokenCancelAmounts); - assert.assert(!_.isEmpty(orders), 'Can not cancel an empty batch'); + 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'); const maker = makers[0]; - // _.zip doesn't type check if values have different types :'( - const ordersAndTakerTokenCancelAmounts = _.zip(orders, takerTokenCancelAmounts); - _.forEach(ordersAndTakerTokenCancelAmounts, - async ([order, takerTokenCancelAmount]: [Order|SignedOrder, BigNumber.BigNumber]) => { + _.forEach(cancellationRequestsBatch, + async (cancellationRequest: OrderCancellationRequest) => { assert.doesConformToSchema('order', - SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), orderSchema); - assert.isBigNumber('takerTokenCancelAmount', takerTokenCancelAmount); - await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'order.maker', order.maker); - await this.validateCancelOrderAndThrowIfInvalidAsync(order, takerTokenCancelAmount); + SchemaValidator.convertToJSONSchemaCompatibleObject(cancellationRequest.order as object), orderSchema); + assert.isBigNumber('takerTokenCancelAmount', cancellationRequest.takerTokenCancelAmount); + await assert.isSenderAddressAvailableAsync(this.web3Wrapper, 'order.maker', + cancellationRequest.order.maker); + await this.validateCancelOrderAndThrowIfInvalidAsync( + cancellationRequest.order, cancellationRequest.takerTokenCancelAmount); }); const exchangeInstance = await this.getExchangeContractAsync(); - const orderAddressesAndValues = _.map(orders, order => { - return ExchangeWrapper.getOrderAddressesAndValues(order); + const orderAddressesValuesAndTakerTokenCancelAmounts = _.map(cancellationRequestsBatch, cancellationRequest => { + return [ + ...ExchangeWrapper.getOrderAddressesAndValues(cancellationRequest.order), + cancellationRequest.takerTokenCancelAmount, + ]; }); // _.unzip doesn't type check if values have different types :'( - const [orderAddresses, orderValues] = _.unzip(orderAddressesAndValues); + const [orderAddresses, orderValues, takerTokenCancelAmounts] = + _.unzip(orderAddressesValuesAndTakerTokenCancelAmounts); const gas = await exchangeInstance.batchCancel.estimateGas( orderAddresses, orderValues, diff --git a/src/types.ts b/src/types.ts index 6c217fe51..1034893a4 100644 --- a/src/types.ts +++ b/src/types.ts @@ -222,3 +222,8 @@ export interface SubscriptionOpts { } export type DoneCallback = (err?: Error) => void; + +export interface OrderCancellationRequest { + order: Order|SignedOrder; + takerTokenCancelAmount: BigNumber.BigNumber; +} diff --git a/src/utils/assert.ts b/src/utils/assert.ts index 61b7527e6..4dc6945a2 100644 --- a/src/utils/assert.ts +++ b/src/utils/assert.ts @@ -42,12 +42,6 @@ export const assert = { const availableAddresses = await web3Wrapper.getAvailableAddressesAsync(); this.assert(!_.isEmpty(availableAddresses), 'No addresses were available on the provided web3 instance'); }, - isSameLength(variableName1: string, value1: any[], variableName2: string, value2: any[]) { - const length1 = value1.length; - const length2 = value2.length; - this.assert(length1 === length2, `${variableName1} and ${variableName2} length mismatch. \ -${length1} != ${length2}`); - }, isNumber(variableName: string, value: number): void { this.assert(_.isFinite(value), this.typeAssertionMessage(variableName, 'number', value)); }, diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index 6487ba98c..68224a68c 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -16,7 +16,7 @@ import { ExchangeEvents, ContractEvent, DoneCallback, - ExchangeContractErrs, + ExchangeContractErrs, OrderCancellationRequest, } from '../src/types'; import {FillScenarios} from './utils/fill_scenarios'; import {TokenUtils} from './utils/token_utils'; @@ -377,34 +377,44 @@ describe('ExchangeWrapper', () => { describe('#batchCancelOrderAsync', () => { let anotherSignedOrder: SignedOrder; let anotherOrderHashHex: string; + let cancelBatch: OrderCancellationRequest[]; beforeEach(async () => { anotherSignedOrder = await fillScenarios.createFillableSignedOrderAsync( makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, ); anotherOrderHashHex = await zeroEx.getOrderHashHexAsync(anotherSignedOrder); + cancelBatch = [ + { + order: signedOrder, + takerTokenCancelAmount: cancelAmount, + }, + { + order: anotherSignedOrder, + takerTokenCancelAmount: cancelAmount, + }, + ]; }); describe('failed batch cancels', () => { - it('should throw when length of orders and cancelAmounts mismatch', async () => { - return expect(zeroEx.exchange.batchCancelOrderAsync([signedOrder], [])) - .to.be.rejectedWith('orders and takerTokenCancelAmounts length mismatch. 1 != 0'); - }); it('should throw when orders are empty', async () => { - return expect(zeroEx.exchange.batchCancelOrderAsync([], [])) + return expect(zeroEx.exchange.batchCancelOrderAsync([])) .to.be.rejectedWith('Can not cancel an empty batch'); }); it.only('should throw when orders have different makers', async () => { const signedOrderWithADifferentMaker = await fillScenarios.createFillableSignedOrderAsync( makerTokenAddress, takerTokenAddress, takerAddress, takerAddress, fillableAmount, ); - return expect(zeroEx.exchange.batchCancelOrderAsync( - [signedOrder, signedOrderWithADifferentMaker], [cancelAmount, cancelAmount])) - .to.be.rejectedWith('Can not cancel orders from multiple makers in a single batch'); + return expect(zeroEx.exchange.batchCancelOrderAsync([ + cancelBatch[0], + { + order: signedOrderWithADifferentMaker, + takerTokenCancelAmount: cancelAmount, + }, + ])).to.be.rejectedWith('Can not cancel orders from multiple makers in a single batch'); }); }); describe('successful batch cancels', () => { it('should cancel a batch of orders', async () => { - await zeroEx.exchange.batchCancelOrderAsync( - [signedOrder, anotherSignedOrder], [cancelAmount, cancelAmount]); + await zeroEx.exchange.batchCancelOrderAsync(cancelBatch); const cancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync(orderHashHex); const anotherCancelledAmount = await zeroEx.exchange.getCanceledTakerAmountAsync( anotherOrderHashHex); -- cgit From 3126279de715d74a73af270a730cf5b0c069634f Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 16:55:25 +0200 Subject: Use batchCancel for normal cancel --- src/contract_wrappers/exchange_wrapper.ts | 33 +++++-------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index f67a125aa..e4d360690 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -175,35 +175,12 @@ export class ExchangeWrapper extends ContractWrapper { /** * Cancel a given fill amount of an order. Cancellations are cumulative. */ - public async cancelOrderAsync(order: Order|SignedOrder, takerTokenCancelAmount: BigNumber.BigNumber): Promise { - 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, + public async cancelOrderAsync( + order: Order|SignedOrder, takerTokenCancelAmount: BigNumber.BigNumber): Promise { + await this.batchCancelOrderAsync([{ + order, 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 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 +++++++++++-------- src/types.ts | 1 + test/exchange_wrapper_test.ts | 12 ++++-------- 3 files changed, 16 insertions(+), 16 deletions(-) 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); }); diff --git a/src/types.ts b/src/types.ts index 1034893a4..c58e5fd02 100644 --- a/src/types.ts +++ b/src/types.ts @@ -145,6 +145,7 @@ export const ExchangeContractErrs = strEnum([ 'INSUFFICIENT_MAKER_FEE_BALANCE', 'INSUFFICIENT_MAKER_FEE_ALLOWANCE', 'TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER', + 'MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH' ]); export type ExchangeContractErrs = keyof typeof ExchangeContractErrs; diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index 68224a68c..88a2e2277 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -395,21 +395,17 @@ describe('ExchangeWrapper', () => { ]; }); describe('failed batch cancels', () => { - it('should throw when orders are empty', async () => { - return expect(zeroEx.exchange.batchCancelOrderAsync([])) - .to.be.rejectedWith('Can not cancel an empty batch'); - }); - it.only('should throw when orders have different makers', async () => { - const signedOrderWithADifferentMaker = await fillScenarios.createFillableSignedOrderAsync( + it('should throw when orders have different makers', async () => { + const signedOrderWithDifferentMaker = await fillScenarios.createFillableSignedOrderAsync( makerTokenAddress, takerTokenAddress, takerAddress, takerAddress, fillableAmount, ); return expect(zeroEx.exchange.batchCancelOrderAsync([ cancelBatch[0], { - order: signedOrderWithADifferentMaker, + order: signedOrderWithDifferentMaker, takerTokenCancelAmount: cancelAmount, }, - ])).to.be.rejectedWith('Can not cancel orders from multiple makers in a single batch'); + ])).to.be.rejectedWith(ExchangeContractErrs.MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH); }); }); describe('successful batch cancels', () => { -- cgit From 7460acbc0b01df35b21971405d582621150443d4 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Wed, 7 Jun 2017 17:53:45 +0200 Subject: Fix linter error --- src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types.ts b/src/types.ts index c58e5fd02..ebd427ae9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -145,7 +145,7 @@ export const ExchangeContractErrs = strEnum([ 'INSUFFICIENT_MAKER_FEE_BALANCE', 'INSUFFICIENT_MAKER_FEE_ALLOWANCE', 'TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER', - 'MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH' + 'MULTIPLE_MAKERS_IN_SINGLE_CANCEL_BATCH', ]); export type ExchangeContractErrs = keyof typeof ExchangeContractErrs; -- cgit 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(-) 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 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(-) 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 From 80294873c16aed756d2d6c321396d6e97b2e9b03 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 8 Jun 2017 11:09:55 +0200 Subject: Format inputs --- test/exchange_wrapper_test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index a9a93fc71..08936f1d2 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -16,7 +16,8 @@ import { ExchangeEvents, ContractEvent, DoneCallback, - ExchangeContractErrs, OrderCancellationRequest, + ExchangeContractErrs, + OrderCancellationRequest, } from '../src/types'; import {FillScenarios} from './utils/fill_scenarios'; import {TokenUtils} from './utils/token_utils'; -- cgit 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(-) 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