From 984e06460466f82b661ccc9b41e7221be56625c9 Mon Sep 17 00:00:00 2001
From: Daniel McNally <mcnallydp@gmail.com>
Date: Thu, 27 Aug 2020 10:53:02 -0400
Subject: [PATCH] fix(orderbook): prevent stuck replace order holds (#1842)

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.
---
 lib/orderbook/OrderBook.ts | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/orderbook/OrderBook.ts b/lib/orderbook/OrderBook.ts
index adf5019a9..66d6be793 100644
--- a/lib/orderbook/OrderBook.ts
+++ b/lib/orderbook/OrderBook.ts
@@ -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) {
@@ -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);