fix: swap client initialization

This fixes a bug where the swap clients would fail on their first
attempt to initialize because they would try to update `pool.nodeState`
which is undefined. `pool.nodeState` is not assigned a value until
`pool.init()` is called, which would not happen until after the first
attempt to initialize swap clients.

This changes the initialization flow so that the node state is assigned
a value in the `Pool` constructor. The properties of `Pool.nodeState`
are then updated as the other components (swap clients, order book)
initialize.

lib/Xud.ts

@@ -73,7 +73,7 @@ class Xud extends EventEmitter {
 
       this.db = new DB(loggers.db, this.config.dbpath);
       await this.db.init(this.config.network, this.config.initdb);
-      this.pool = new Pool(this.config.p2p, this.config.network, loggers.p2p, this.db.models);
+      this.pool = new Pool(this.config.p2p, this.config.network, loggers.p2p, this.db.models, version);
       this.swapClientManager = new SwapClientManager(this.config, loggers, this.pool);
       await this.swapClientManager.init(this.db.models);
 
@@ -89,13 +89,7 @@ class Xud extends EventEmitter {
       this.logger.info(`Local nodePubKey is ${this.nodeKey.nodePubKey}`);
 
       // initialize pool and start listening/connecting only once other components are initialized
-      await this.pool.init({
-        version,
-        lndPubKeys: this.swapClientManager.getLndPubKeys(),
-        pairs: this.orderBook.pairIds,
-        nodePubKey: this.nodeKey.nodePubKey,
-        raidenAddress: this.swapClientManager.raidenClient.address,
-      }, this.nodeKey);
+      await this.pool.init(this.nodeKey);
 
       this.service = new Service({
         version,

lib/orderbook/OrderBook.ts

@@ -160,6 +160,8 @@ class OrderBook extends EventEmitter {
       this.pairInstances.set(pair.id, pair);
       this.tradingPairs.set(pair.id, new TradingPair(this.logger, pair.id, this.nomatching));
     });
+
+    this.pool.updatePairs(this.pairIds);
   }
 
   public getCurrencyAttributes(symbol: string) {

lib/p2p/Pool.ts

@@ -69,7 +69,7 @@ interface NodeConnectionIterator {
  */
 class Pool extends EventEmitter {
   /** The local handshake data to be sent to newly connected peers. */
-  public nodeState!: NodeState;
+  public nodeState: NodeState;
   /** The local node key. */
   private nodeKey!: NodeKey;
   /** A map of pub keys to nodes for which we have pending outgoing connections. */
@@ -84,26 +84,33 @@ class Pool extends EventEmitter {
   private connected = false;
   /** The port on which to listen for peer connections, undefined if this node is not listening. */
   private listenPort?: number;
-  /** This node's listening external socket addresses to advertise to peers. */
-  private addresses: Address[] = [];
   /** Points to config comes during construction. */
   private config: PoolConfig;
   private repository: P2PRepository;
   private network: Network;
 
-  constructor(config: PoolConfig, xuNetwork: XuNetwork, private logger: Logger, models: Models) {
+  constructor(config: PoolConfig, xuNetwork: XuNetwork, private logger: Logger, models: Models, version: string) {
     super();
     this.config = config;
     this.network = new Network(xuNetwork);
     this.repository = new P2PRepository(models);
     this.nodes = new NodeList(this.repository);
 
+    this.nodeState = {
+      version,
+      nodePubKey: '',
+      addresses: [],
+      pairs: [],
+      raidenAddress: '',
+      lndPubKeys: {},
+    };
+
     if (config.listen) {
       this.listenPort = config.port;
       this.server = net.createServer();
       config.addresses.forEach((addressString) => {
         const address = addressUtils.fromString(addressString, config.port);
-        this.addresses.push(address);
+        this.nodeState.addresses.push(address);
       });
     }
   }
@@ -115,7 +122,7 @@ class Pool extends EventEmitter {
   /**
    * Initialize the Pool by connecting to known nodes and listening to incoming peer connections, if configured to do so.
    */
-  public init = async (ownNodeState: Pick<NodeState, Exclude<keyof NodeState, 'addresses'>>, nodeKey: NodeKey): Promise<void> => {
+  public init = async (nodeKey: NodeKey): Promise<void> => {
     if (this.connected) {
       return;
     }
@@ -130,7 +137,7 @@ class Pool extends EventEmitter {
       }
     }
 
-    this.nodeState = { ...ownNodeState, addresses: this.addresses };
+    this.nodeState.nodePubKey = nodeKey.nodePubKey;
     this.nodeKey = nodeKey;
 
     this.bindNodeList();
@@ -158,9 +165,9 @@ class Pool extends EventEmitter {
       externalIp = await getExternalIp();
       this.logger.info(`retrieved external IP: ${externalIp}`);
 
-      const externalIpExists = this.addresses.some((address) =>  { return address.host === externalIp; });
+      const externalIpExists = this.nodeState.addresses.some((address) =>  { return address.host === externalIp; });
       if (!externalIpExists) {
-        this.addresses.push({
+        this.nodeState.addresses.push({
           host: externalIp,
           port: this.listenPort!,
         });
@@ -174,8 +181,8 @@ class Pool extends EventEmitter {
    * Updates our active trading pairs and sends a node state update packet to currently connected
    * peers to notify them of the change.
    */
-  public updatePairs = (pairs: string[]) => {
-    this.nodeState.pairs = pairs;
+  public updatePairs = (pairIds: string[]) => {
+    this.nodeState.pairs = pairIds;
     this.sendNodeStateUpdate();
   }
 
@@ -194,6 +201,7 @@ class Pool extends EventEmitter {
    */
   public updateLndPubKey = (currency: string, pubKey: string) => {
     this.nodeState.lndPubKeys[currency] = pubKey;
+    this.logger.info(`${currency} ${pubKey} ${JSON.stringify(this.nodeState.lndPubKeys)}`);
     this.sendNodeStateUpdate();
   }
 

lib/p2p/types.ts

@@ -15,6 +15,7 @@ export type NodeConnectionInfo = {
 export type NodeState = {
   version: string;
   nodePubKey: string;
+  /** This node's listening external socket addresses to advertise to peers. */
   addresses: Address[];
   pairs: string[];
   raidenAddress: string;

lib/raidenclient/RaidenClient.ts

@@ -80,6 +80,7 @@ class RaidenClient extends SwapClient {
     } catch (err) {
       this.logger.error(
         `could not verify connection to raiden at ${this.host}:${this.port}, retrying in ${RaidenClient.RECONNECT_TIMER} ms`,
+        err,
       );
       await this.setStatus(ClientStatus.Disconnected);
     }

lib/swaps/SwapClientManager.ts

@@ -7,8 +7,16 @@ import Logger, { Loggers } from '../Logger';
 import Pool from '../p2p/Pool';
 import { errors } from './errors';
 import { Currency } from '../orderbook/types';
-import { isLndClient, isRaidenClient } from './types';
 import { Models } from '../db/DB';
+import { SwapClientType } from '../constants/enums';
+
+function isRaidenClient(swapClient: SwapClient): swapClient is RaidenClient {
+  return (swapClient.type === SwapClientType.Raiden);
+}
+
+function isLndClient(swapClient: SwapClient): swapClient is LndClient {
+  return (swapClient.type === SwapClientType.Lnd);
+}
 
 class SwapClientManager {
   /** A map between currencies and all swap clients */
@@ -60,12 +68,19 @@ class SwapClientManager {
     }
     // setup Raiden
     initPromises.push(this.raidenClient.init());
-    // TODO: it might make sense to remove swap clients that turned out
-    // to be disabled after the initPromises resolve, since we
-    // currently treat disabled clients as if they don't exist.
-    await Promise.all(initPromises);
-    // bind event listeners after all swap clients have initialized
+
+    // bind event listeners before all swap clients have initialized
     this.bind();
+
+    await Promise.all(initPromises);
+
+    // delete any swap clients that were disabled during initialization
+    this.swapClients.forEach((swapClient, currency) => {
+      if (swapClient.isDisabled()) {
+        this.swapClients.delete(currency);
+      }
+    });
+
     // associate swap clients with currencies managed by raiden client
     if (!this.raidenClient.isDisabled()) {
       const currencyInstances = await models.Currency.findAll();
@@ -93,10 +108,10 @@ class SwapClientManager {
    * @returns Nothing upon success, throws otherwise.
    */
   public add = (currency: Currency): void => {
-    if (isRaidenClient(currency.swapClient) && currency.tokenAddress) {
+    if (currency.swapClient === SwapClientType.Raiden && currency.tokenAddress) {
       this.swapClients.set(currency.id, this.raidenClient);
       this.raidenClient.tokenAddresses.set(currency.id, currency.tokenAddress);
-    } else if (isLndClient(currency.swapClient)) {
+    } else if (currency.swapClient === SwapClientType.Lnd) {
       // in case of lnd we check if the configuration includes swap client
       // for the specified currency
       let hasCurrency = false;
@@ -120,7 +135,7 @@ class SwapClientManager {
   public remove = (currency: string): void => {
     const swapClient = this.get(currency);
     this.swapClients.delete(currency);
-    if (swapClient && isRaidenClient(swapClient.type)) {
+    if (swapClient && isRaidenClient(swapClient)) {
       this.raidenClient.tokenAddresses.delete(currency);
     }
   }
@@ -132,8 +147,8 @@ class SwapClientManager {
   public getLndPubKeys = (): LndPubKeys => {
     const lndPubKeys: LndPubKeys = {};
     for (const [currency, swapClient] of this.swapClients.entries()) {
-      if (isLndClient(swapClient.type)) {
-        lndPubKeys[currency] = (swapClient as LndClient).pubKey;
+      if (isLndClient(swapClient)) {
+        lndPubKeys[currency] = swapClient.pubKey;
       }
     }
     return lndPubKeys;
@@ -151,10 +166,10 @@ class SwapClientManager {
     // rather than determining it dynamically when needed. The benefits
     // would be slightly improved performance.
     for (const [currency, swapClient] of this.swapClients.entries()) {
-      if (isLndClient(swapClient.type)) {
+      if (isLndClient(swapClient)) {
         lndInfos[currency] = swapClient.isDisabled()
           ? undefined
-          : await (swapClient as LndClient).getLndInfo();
+          : await swapClient.getLndInfo();
       }
     }
     return lndInfos;
@@ -168,7 +183,7 @@ class SwapClientManager {
     let raidenClosed = false;
     for (const swapClient of this.swapClients.values()) {
       swapClient.close();
-      if (isRaidenClient(swapClient.type)) {
+      if (isRaidenClient(swapClient)) {
         raidenClosed = true;
       }
     }
@@ -181,7 +196,7 @@ class SwapClientManager {
 
   private bind = () => {
     for (const [currency, swapClient] of this.swapClients.entries()) {
-      if (isLndClient(swapClient.type)) {
+      if (isLndClient(swapClient)) {
         swapClient.on('connectionVerified', (newPubKey) => {
           if (newPubKey) {
             this.pool.updateLndPubKey(currency, newPubKey);

lib/swaps/Swaps.ts

@@ -449,7 +449,6 @@ class Swaps extends EventEmitter {
       localId: orderToAccept.localId,
       phase: SwapPhase.SwapCreated,
       state: SwapState.Active,
-      rHash: requestBody.rHash,
       role: SwapRole.Maker,
       createTime: Date.now(),
     };

lib/swaps/types.ts

@@ -3,7 +3,6 @@ import {
   SwapPhase,
   SwapState,
   SwapFailureReason,
-  SwapClientType,
 } from '../constants/enums';
 
 export type SwapDeal = {
@@ -97,11 +96,3 @@ export type ResolveRequest = {
   amount: number,
   rHash: string,
 };
-
-export function isLndClient(swapClientType: SwapClientType): boolean {
-  return (swapClientType === SwapClientType.Lnd);
-}
-
-export function isRaidenClient(swapClientType: SwapClientType): boolean {
-  return (swapClientType === SwapClientType.Raiden);
-}

test/integration/OrderBook.spec.ts

@@ -18,6 +18,14 @@ const PAIR_ID = 'LTC/BTC';
 const currencies = PAIR_ID.split('/');
 const loggers = Logger.createLoggers(Level.Warn);
 
+const getMockPool = (sandbox: sinon.SinonSandbox) => {
+  const pool = sandbox.createStubInstance(Pool) as any;
+  pool.broadcastOrder = () => {};
+  pool.broadcastOrderInvalidation = () => {};
+  pool.updatePairs = () => {};
+  return pool;
+};
+
 const initValues = async (db: DB) => {
   const orderBookRepository = new OrderBookRepository(db.models);
 
@@ -30,24 +38,22 @@ const initValues = async (db: DB) => {
   ]);
 };
 
-const sandbox = sinon.createSandbox();
-const pool = sandbox.createStubInstance(Pool) as any;
-pool.broadcastOrder = () => {};
-pool.broadcastOrderInvalidation = () => {};
-pool.updatePairs = () => {};
-
 describe('OrderBook', () => {
+  let sandbox: sinon.SinonSandbox;
   let db: DB;
   let swaps: Swaps;
   let orderBook: OrderBook;
 
   before(async () => {
+    sandbox = sinon.createSandbox();
     db = new DB(loggers.db);
     await db.init();
 
+    sandbox = sinon.createSandbox();
+    const pool = getMockPool(sandbox);
     await initValues(db);
 
-    swaps = sandbox.createStubInstance(Swaps) as any;;
+    swaps = sandbox.createStubInstance(Swaps) as any;
     swaps.isPairSupported = () => true;
     const lndBTC = sandbox.createStubInstance(LndClient) as any;
     const lndLTC = sandbox.createStubInstance(LndClient) as any;
@@ -66,10 +72,6 @@ describe('OrderBook', () => {
     await orderBook.init();
   });
 
-  after(async () => {
-    sandbox.restore();
-  });
-
   const getOwnOrder = (order: orders.OwnOrder): orders.OwnOrder | undefined => {
     const ownOrders = orderBook.getOwnOrders(order.pairId);
     const arr = order.isBuy ? ownOrders.buyArray : ownOrders.sellArray;
@@ -148,16 +150,21 @@ describe('OrderBook', () => {
 
   after(async () => {
     await db.close();
+    sandbox.restore();
   });
 });
 
 describe('nomatching OrderBook', () => {
   let db: DB;
+  let sandbox: sinon.SinonSandbox;
+  let pool: Pool;
   let orderBook: OrderBook;
 
   before(async () => {
     db = new DB(loggers.db);
     await db.init();
+    sandbox = sinon.createSandbox();
+    pool = getMockPool(sandbox);
     await initValues(db);
   });
 
@@ -255,10 +262,10 @@ describe('nomatching OrderBook', () => {
       expect(() => orderBook['stampOwnOrder'](ownOrderWithLocalId))
         .to.throw(`order with local id ${ownOrderWithLocalId.localId} already exists`);
     });
-
   });
 
   after(async () => {
     await db.close();
+    sandbox.restore();
   });
 });

test/integration/Pool.spec.ts

@@ -44,22 +44,17 @@ describe('P2P Pool Tests', async () => {
 
   before(async () => {
     nodeKeyOne = await NodeKey['generate']();
+    const nodeKeyTwo = await NodeKey['generate']();
 
     const config = new Config();
     config.p2p.listen = false;
     config.p2p.discover = false;
     db = new DB(loggers.db);
     await db.init();
 
-    pool = new Pool(config.p2p, XuNetwork.SimNet, loggers.p2p, db.models);
+    pool = new Pool(config.p2p, XuNetwork.SimNet, loggers.p2p, db.models, '1.0.0');
 
-    await pool.init({
-      nodePubKey: 'test',
-      version: 'test',
-      pairs: [],
-      lndPubKeys: {},
-      raidenAddress: '',
-    }, nodeKeyOne);
+    await pool.init(nodeKeyTwo);
   });
 
   it('should open a connection with a peer', async () => {