Skip to content

Commit

Permalink
fix(orderbook): prevent stuck replace order holds (#1842)
Browse files Browse the repository at this point in the history
This fixes a bug whereby an order could be placed on hold without that
hold ever being removed.

When replacing an order, we place the existing order on hold until the
replacement order has finished matching, and only then do we remove the
original order. However, the order was being placed on hold before the
checks for sufficient balance to fill this order, and in case those
checks failed the remainder of the `placeOrder` routine would be skipped
including the part that removes the original order. In such a case, the
original order would be put on hold indefinitely.

Instead, we place the existing order on hold immediately before we
begin matching and after any checks on the validity of the new order
are passed. If the checks fail, then the new order is rejected and the
order it was intended to replace remains in the order book without a
hold, allowing it to be replaced or removed again by a future call.

Fixes #1835.
  • Loading branch information
sangaman authored Aug 27, 2020
1 parent ed43416 commit 984e064
Showing 1 changed file with 12 additions and 13 deletions.
25 changes: 12 additions & 13 deletions lib/orderbook/OrderBook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,19 +444,6 @@ class OrderBook extends EventEmitter {
}
}

assert(!(replaceOrderId && discardRemaining), 'can not replace order and discard remaining order');

let replacedOrderIdentifier: OrderIdentifier | undefined;
if (replaceOrderId) {
// put the order we are replacing on hold while we place the new order
replacedOrderIdentifier = this.localIdMap.get(replaceOrderId);
if (!replacedOrderIdentifier) {
throw errors.ORDER_NOT_FOUND(replaceOrderId);
}
assert(replacedOrderIdentifier.pairId === order.pairId);
this.addOrderHold(replacedOrderIdentifier.id, replacedOrderIdentifier.pairId);
}

// this method can be called recursively on swap failures retries.
// if max time exceeded, don't try to match
if (maxTime && Date.now() > maxTime) {
Expand Down Expand Up @@ -493,6 +480,18 @@ class OrderBook extends EventEmitter {
}
}

assert(!(replaceOrderId && discardRemaining), 'can not replace order and discard remaining order');
let replacedOrderIdentifier: OrderIdentifier | undefined;
if (replaceOrderId) {
// put the order we are replacing on hold while we place the new order
replacedOrderIdentifier = this.localIdMap.get(replaceOrderId);
if (!replacedOrderIdentifier) {
throw errors.ORDER_NOT_FOUND(replaceOrderId);
}
assert(replacedOrderIdentifier.pairId === order.pairId);
this.addOrderHold(replacedOrderIdentifier.id, replacedOrderIdentifier.pairId);
}

// perform matching routine. maker orders that are matched will be removed from the order book.
const tp = this.getTradingPair(order.pairId);
const matchingResult = tp.match(order);
Expand Down

0 comments on commit 984e064

Please sign in to comment.