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.

added currency check to walletwithdraw

lib/cli/commands/walletwithdraw.ts

@@ -33,6 +33,10 @@ export const builder = (argv: Argv) => argv
   .example('$0 withdraw --all --currency BTC --address 1BitcoinEaterAddressDontSendf59kuE', 'withdraws all BTC');
 
 export const handler = async (argv: Arguments<any>) => {
+  if (!argv.currency) {
+    throw 'currency is not valid';
+  }
+
   const request = new WithdrawRequest();
   request.setCurrency(argv.currency.toUpperCase());
   if (argv.all) {

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);