Skip to content

Commit

Permalink
Bookbuilder modify keeps orders (coinbase#174)
Browse files Browse the repository at this point in the history
* BookBuilderTest.ts: add asserts on adding and modifying orders with zero size.

* BookBuilder#modify: remove redundant check for order existence.

Because BookBuilder#remove() first checks if the order exists and
returns null if it doesn't, there is no need for modify() to also
check for its existence.

* BookBuilder#modify: keep an order even if its size goes to zero.

This is to work around a bug where Trader#handleTradeExecutedMessage()
would modify the order size to zero which would cause it to be deleted
from the orderbook. Later when Trader#handleTradeFinalized() was
called it wouldn't find the order and wouldn't emit
Trader.trade-finalized. Now with d3103dd committed it will always emit
Trader.trade-finalized.

However, this still appears to be the correct action. BookBuilder
supports adding orders with 0 size, which is inconsistent with
modifying an error. Also, only deleting orders when the trade is
finalized seems like a good consistency thing.

* Trader#handleTradeFinalized: micro-optimize.
  • Loading branch information
blair authored and fb55 committed Apr 20, 2018
1 parent db2f807 commit c092684
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 11 deletions.
2 changes: 1 addition & 1 deletion src/core/Trader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ export class Trader extends Writable {
private handleTradeFinalized(msg: TradeFinalizedMessage) {
const id: string = msg.orderId;
const order: Level3Order = this.myBook.remove(id);
if (!order && this.unfilledMarketOrders.has(id)) {
if (!order) {
this.unfilledMarketOrders.delete(id);
}
this.emit('Trader.trade-finalized', msg);
Expand Down
15 changes: 5 additions & 10 deletions src/lib/BookBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,26 +258,21 @@ export class BookBuilder extends EventEmitter implements Orderbook {

/**
* Changes the size of an existing order to newSize. If the order doesn't exist, returns false.
* If the newSize is zero, the order is removed.
* If newSize is negative, an error is thrown.
* Even if newSize is zero, the order is kept.
* It is possible for an order to switch sides, in which case the newSide parameter determines the new side.
*/
modify(id: string, newSize: BigJS, newSide?: Side): boolean {
if (newSize.lt(ZERO)) {
throw new Error('Cannot set an order size to a negative number');
}
const order = this.getOrder(id);
const order = this.remove(id);
if (!order) {
return false;
}
if (!this.remove(id)) {
return false;
}
if (newSize.gt(ZERO)) {
order.size = newSize;
order.side = newSide || order.side;
this.add(order);
}
order.size = newSize;
order.side = newSide || order.side;
this.add(order);
return true;
}

Expand Down
24 changes: 24 additions & 0 deletions test/lib/BookBuilderTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,30 @@ describe('BookBuilder:', () => {
assert.ok(book.asksValueTotal.eq(randomBook.totalAsksValue.minus(deconstedOrder.size.times(deconstedOrder.price))));
}
});

it('adds an order when the order size is zero', () => {
const order: Level3Order = {id: 'abcdefg',
price: Big(123.4567),
size: ZERO,
side: 'buy'};
assert.ok(!book.hasOrder(order.id), 'Book does not have order');
assert.ok(book.add(order), 'Book adds the order with zero size');
assert.ok(book.hasOrder(order.id), 'Book has order');
});

it('keeps an order when the order size is modified to zero', () => {
assert.ok(book.hasOrder('00006'),
'Order was not removed');
const order = book.getOrder('00006');
assert.ok(order,
'Order still was not removed');
assert.ok(book.modify('00006', ZERO),
'Order can be modified to zero size');
assert.ok(book.hasOrder('00006'),
'Book has order after modified to zero size');
assert.ok(book.getOrder('00006').size.eq(ZERO),
'The order has zero size');
});
});
});

Expand Down

0 comments on commit c092684

Please sign in to comment.