From 114151f5f6f5ddb327da08fbf08ddad415e658d8 Mon Sep 17 00:00:00 2001
From: Tom Kirkpatrick <tkp@kirkdesigns.co.uk>
Date: Thu, 16 Aug 2018 11:31:03 +0200
Subject: [PATCH 1/5] fix(grpc): ensure full disconnect on window close

Do not try to send incoming data from GRPC streams to the main window
after the window has been closed.

Fix #677
---
 app/lib/lnd/lightning.js              |  9 +++++----
 app/lib/lnd/subscribe/channelgraph.js | 23 +++++++++++++++--------
 app/lib/lnd/subscribe/invoices.js     | 17 ++++++++++-------
 app/lib/lnd/subscribe/transactions.js | 17 ++++++++++-------
 4 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/app/lib/lnd/lightning.js b/app/lib/lnd/lightning.js
index 67a2a75b..0453dd84 100644
--- a/app/lib/lnd/lightning.js
+++ b/app/lib/lnd/lightning.js
@@ -89,22 +89,23 @@ class Lightning {
    */
   subscribe(mainWindow) {
     this.mainWindow = mainWindow
-    this.subscriptions.channelGraph = subscribeToChannelGraph(this.mainWindow, this.lnd, mainLog)
-    this.subscriptions.invoices = subscribeToInvoices(this.mainWindow, this.lnd, mainLog)
-    this.subscriptions.transactions = subscribeToTransactions(this.mainWindow, this.lnd, mainLog)
+
+    this.subscriptions.channelGraph = subscribeToChannelGraph.call(this)
+    this.subscriptions.invoices = subscribeToInvoices.call(this)
+    this.subscriptions.transactions = subscribeToTransactions.call(this)
   }
 
   /**
    * Unsubscribe from all bi-directional streams.
    */
   unsubscribe() {
+    this.mainWindow = null
     Object.keys(this.subscriptions).forEach(subscription => {
       if (this.subscriptions[subscription]) {
         this.subscriptions[subscription].cancel()
         this.subscriptions[subscription] = null
       }
     })
-    this.mainWindow = null
   }
 }
 
diff --git a/app/lib/lnd/subscribe/channelgraph.js b/app/lib/lnd/subscribe/channelgraph.js
index 02f30a08..6340bb21 100644
--- a/app/lib/lnd/subscribe/channelgraph.js
+++ b/app/lib/lnd/subscribe/channelgraph.js
@@ -1,14 +1,21 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToChannelGraph(mainWindow, lnd, log) {
-  const call = lnd.subscribeChannelGraph({})
+export default function subscribeToChannelGraph() {
+  const call = this.lnd.subscribeChannelGraph({})
 
-  call.on('data', channelGraphData => mainWindow.send('channelGraphData', { channelGraphData }))
-  call.on('end', () => log.info('end'))
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
-  call.on('status', channelGraphStatus =>
-    mainWindow.send('channelGraphStatus', { channelGraphStatus })
-  )
+  call.on('data', channelGraphData => {
+    if (this.mainWindow) {
+      this.mainWindow.send('channelGraphData', { channelGraphData })
+    }
+  })
+  call.on('end', () => mainLog.info('end'))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
+  call.on('status', channelGraphStatus => {
+    if (this.mainWindow) {
+      this.mainWindow.send('channelGraphStatus', { channelGraphStatus })
+    }
+  })
 
   return call
 }
diff --git a/app/lib/lnd/subscribe/invoices.js b/app/lib/lnd/subscribe/invoices.js
index a127c221..171471dc 100644
--- a/app/lib/lnd/subscribe/invoices.js
+++ b/app/lib/lnd/subscribe/invoices.js
@@ -1,15 +1,18 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToInvoices(mainWindow, lnd, log) {
-  const call = lnd.subscribeInvoices({})
+export default function subscribeToInvoices() {
+  const call = this.lnd.subscribeInvoices({})
 
   call.on('data', invoice => {
-    log.info('INVOICE:', invoice)
-    mainWindow.send('invoiceUpdate', { invoice })
+    mainLog.info('INVOICE:', invoice)
+    if (this.mainWindow) {
+      this.mainWindow.send('invoiceUpdate', { invoice })
+    }
   })
-  call.on('end', () => log.info('end'))
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
-  call.on('status', status => log.info('INVOICE STATUS:', status))
+  call.on('end', () => mainLog.info('end'))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
+  call.on('status', status => mainLog.info('INVOICE STATUS:', status))
 
   return call
 }
diff --git a/app/lib/lnd/subscribe/transactions.js b/app/lib/lnd/subscribe/transactions.js
index 0f143dc5..c328abb2 100644
--- a/app/lib/lnd/subscribe/transactions.js
+++ b/app/lib/lnd/subscribe/transactions.js
@@ -1,15 +1,18 @@
 import { status } from 'grpc'
+import { mainLog } from '../../utils/log'
 
-export default function subscribeToTransactions(mainWindow, lnd, log) {
-  const call = lnd.subscribeTransactions({})
+export default function subscribeToTransactions() {
+  const call = this.lnd.subscribeTransactions({})
 
   call.on('data', transaction => {
-    log.info('TRANSACTION:', transaction)
-    mainWindow.send('newTransaction', { transaction })
+    mainLog.info('TRANSACTION:', transaction)
+    if (this.mainWindow) {
+      this.mainWindow.send('newTransaction', { transaction })
+    }
   })
-  call.on('end', () => log.info('end'))
-  call.on('error', error => error.code !== status.CANCELLED && log.error(error))
-  call.on('status', status => log.info('TRANSACTION STATUS: ', status))
+  call.on('end', () => mainLog.info('end'))
+  call.on('error', error => error.code !== status.CANCELLED && mainLog.error(error))
+  call.on('status', status => mainLog.info('TRANSACTION STATUS: ', status))
 
   return call
 }

From dfa09c70f5049c9b4d6cff21642e82ad6c1342fd Mon Sep 17 00:00:00 2001
From: Tom Kirkpatrick <tkp@kirkdesigns.co.uk>
Date: Thu, 16 Aug 2018 11:33:54 +0200
Subject: [PATCH 2/5] fix(app): simplify app quit process

Remove redundant code relating to the app close/quit process.
---
 app/lib/zap/controller.js |  3 ---
 app/main.dev.js           | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/app/lib/zap/controller.js b/app/lib/zap/controller.js
index 9c1cee80..b5c0162e 100644
--- a/app/lib/zap/controller.js
+++ b/app/lib/zap/controller.js
@@ -199,9 +199,6 @@ class ZapController {
     if (this.neutrino) {
       this.neutrino.stop()
     }
-
-    // Give the grpc connections a chance to be properly closed out.
-    return new Promise(resolve => setTimeout(resolve, 200))
   }
 
   onTerminate() {
diff --git a/app/main.dev.js b/app/main.dev.js
index 1765d7de..090a9a98 100644
--- a/app/main.dev.js
+++ b/app/main.dev.js
@@ -99,18 +99,6 @@ app.on('ready', () => {
     }
   })
 
-  /**
-   * Add application event listener:
-   *  - quit app when window is closed
-   */
-  app.on('window-all-closed', () => {
-    mainLog.debug('app.window-all-closed')
-    // Respect the OSX convention of having the application in memory even after all windows have been closed
-    if (process.platform !== 'darwin') {
-      app.quit()
-    }
-  })
-
   /**
    * Add application event listener:
    *  - Stop gRPC and kill lnd process before the app windows are closed and the app quits.

From 0ecb169e5c173f384030d3cf81dec8942ec6c922 Mon Sep 17 00:00:00 2001
From: Tom Kirkpatrick <tkp@kirkdesigns.co.uk>
Date: Mon, 20 Aug 2018 08:22:21 +0200
Subject: [PATCH 3/5] chore(flow): add flowtype to Lightning class

---
 app/lib/lnd/lightning.js | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/lib/lnd/lightning.js b/app/lib/lnd/lightning.js
index 0453dd84..25471812 100644
--- a/app/lib/lnd/lightning.js
+++ b/app/lib/lnd/lightning.js
@@ -1,5 +1,9 @@
+// @flow
+
 import grpc from 'grpc'
 import { loadSync } from '@grpc/proto-loader'
+import { BrowserWindow } from 'electron'
+import LndConfig from './config'
 import { getDeadline, validateHost, createSslCreds, createMacaroonCreds } from './util'
 import methods from './methods'
 import { mainLog } from '../utils/log'
@@ -7,11 +11,22 @@ import subscribeToTransactions from './subscribe/transactions'
 import subscribeToInvoices from './subscribe/invoices'
 import subscribeToChannelGraph from './subscribe/channelgraph'
 
+// Type definition for subscriptions property.
+type LightningSubscriptionsType = {
+  channelGraph: any,
+  invoices: any,
+  transactions: any
+}
+
 /**
  * Creates an LND grpc client lightning service.
  * @returns {Lightning}
  */
 class Lightning {
+  mainWindow: BrowserWindow
+  lnd: any
+  subscriptions: LightningSubscriptionsType
+
   constructor() {
     this.mainWindow = null
     this.lnd = null
@@ -26,7 +41,7 @@ class Lightning {
    * Connect to the gRPC interface and verify it is functional.
    * @return {Promise<rpc.lnrpc.Lightning>}
    */
-  async connect(lndConfig) {
+  async connect(lndConfig: LndConfig) {
     const { rpcProtoPath, host, cert, macaroon } = lndConfig
 
     // Verify that the host is valid before creating a gRPC client that is connected to it.
@@ -80,14 +95,14 @@ class Lightning {
   /**
    * Hook up lnd restful methods.
    */
-  lndMethods(event, msg, data) {
+  lndMethods(event: Event, msg: string, data: any) {
     return methods(this.lnd, mainLog, event, msg, data)
   }
 
   /**
    * Subscribe to all bi-directional streams.
    */
-  subscribe(mainWindow) {
+  subscribe(mainWindow: BrowserWindow) {
     this.mainWindow = mainWindow
 
     this.subscriptions.channelGraph = subscribeToChannelGraph.call(this)

From 602d03411ae01f2b9c8cc46e8b6134163e23ad0f Mon Sep 17 00:00:00 2001
From: Tom Kirkpatrick <tkp@kirkdesigns.co.uk>
Date: Mon, 20 Aug 2018 08:24:57 +0200
Subject: [PATCH 4/5] test(unit): add some basic tests for Lightning class

---
 app/lib/lnd/lightning.js        |  4 ++-
 test/unit/lnd/lightning.spec.js | 53 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 test/unit/lnd/lightning.spec.js

diff --git a/app/lib/lnd/lightning.js b/app/lib/lnd/lightning.js
index 25471812..81e466e3 100644
--- a/app/lib/lnd/lightning.js
+++ b/app/lib/lnd/lightning.js
@@ -89,7 +89,9 @@ class Lightning {
    */
   disconnect() {
     this.unsubscribe()
-    this.lnd.close()
+    if (this.lnd) {
+      this.lnd.close()
+    }
   }
 
   /**
diff --git a/test/unit/lnd/lightning.spec.js b/test/unit/lnd/lightning.spec.js
new file mode 100644
index 00000000..6a06e3ce
--- /dev/null
+++ b/test/unit/lnd/lightning.spec.js
@@ -0,0 +1,53 @@
+import { BrowserWindow } from 'electron'
+import Lightning from 'lib/lnd/lightning'
+
+jest.mock('electron-store')
+jest.mock('lib/lnd/subscribe/transactions')
+jest.mock('lib/lnd/subscribe/invoices')
+jest.mock('lib/lnd/subscribe/channelgraph')
+
+describe('Lightning', function() {
+  describe('Constructor', () => {
+    beforeAll(() => (this.lightning = new Lightning()))
+
+    describe('initial values', () => {
+      it('should set the "mainWindow" property to null', () => {
+        expect(this.lightning.mainWindow).toBeNull()
+      })
+      it('should set the "lnd" property to null', () => {
+        expect(this.lightning.lnd).toBeNull()
+      })
+      it('should initialise the "subscriptions" object with null values', () => {
+        expect(this.lightning.subscriptions).toMatchObject({
+          channelGraph: null,
+          invoices: null,
+          transactions: null
+        })
+      })
+    })
+  })
+
+  describe('subscribe()', () => {
+    beforeAll(() => {
+      this.window = new BrowserWindow({})
+      this.lightning = new Lightning()
+      this.lightning.subscribe(this.window)
+    })
+
+    it('should assign the window to the "mainWindow" property', () => {
+      expect(this.lightning.mainWindow).toBe(this.window)
+    })
+  })
+
+  describe('unsubscribe()', () => {
+    beforeAll(() => {
+      this.lightning = new Lightning()
+      this.lightning.mainWindow = new BrowserWindow({})
+      this.lightning.unsubscribe()
+    })
+
+    it('should unassign the "mainWindow" property', () => {
+      expect(this.lightning.mainWindow).toBeNull()
+    })
+  })
+})

From e786958b556916c18abbac9f240bf23aec7ba505 Mon Sep 17 00:00:00 2001
From: Tom Kirkpatrick <tkp@kirkdesigns.co.uk>
Date: Mon, 20 Aug 2018 09:01:14 +0200
Subject: [PATCH 5/5] test(electron): simplify electron mocks

---
 app/lib/lnd/config.js            |  8 +++++---
 app/lib/lnd/util.js              | 13 ++++++++-----
 test/e2e/e2e.spec.js             |  2 ++
 test/unit/__mocks__/electron.js  | 13 +++++++++++++
 test/unit/lnd/lnd-config.spec.js | 29 ++++-------------------------
 test/unit/lnd/neutrino.spec.js   | 11 +----------
 6 files changed, 33 insertions(+), 43 deletions(-)
 create mode 100644 test/unit/__mocks__/electron.js

diff --git a/app/lib/lnd/config.js b/app/lib/lnd/config.js
index bfd0b4e5..e61329fb 100644
--- a/app/lib/lnd/config.js
+++ b/app/lib/lnd/config.js
@@ -141,7 +141,9 @@ class LndConfig {
       },
       binaryPath: {
         enumerable: true,
-        value: binaryPath
+        get() {
+          return binaryPath()
+        }
       },
       dataDir: {
         enumerable: true,
@@ -152,13 +154,13 @@ class LndConfig {
       configPath: {
         enumerable: true,
         get() {
-          return join(appRootPath, 'resources', 'lnd.conf')
+          return join(appRootPath(), 'resources', 'lnd.conf')
         }
       },
       rpcProtoPath: {
         enumerable: true,
         get() {
-          return join(appRootPath, 'resources', 'rpc.proto')
+          return join(appRootPath(), 'resources', 'rpc.proto')
         }
       },
 
diff --git a/app/lib/lnd/util.js b/app/lib/lnd/util.js
index 0656abdd..08cc2244 100644
--- a/app/lib/lnd/util.js
+++ b/app/lib/lnd/util.js
@@ -33,8 +33,9 @@ const dnsLookup = promisify(dns.lookup)
  * And no, we can't just set our working directory to somewhere inside the asar. The OS can't handle that.
  * @return {String} Path to the lnd binary.
  */
-export const appRootPath =
-  app.getAppPath().indexOf('default_app.asar') < 0 ? normalize(`${app.getAppPath()}/..`) : ''
+export const appRootPath = () => {
+  return app.getAppPath().indexOf('default_app.asar') < 0 ? normalize(`${app.getAppPath()}/..`) : ''
+}
 
 /**
  * Get the OS specific lnd binary name.
@@ -46,9 +47,11 @@ export const binaryName = platform() === 'win32' ? 'lnd.exe' : 'lnd'
  * Get the OS specific path to the lnd binary.
  * @return {String} Path to the lnd binary.
  */
-export const binaryPath = isDev
-  ? join(dirname(require.resolve('lnd-binary/package.json')), 'vendor', binaryName)
-  : join(appRootPath, 'bin', binaryName)
+export const binaryPath = () => {
+  return isDev
+    ? join(dirname(require.resolve('lnd-binary/package.json')), 'vendor', binaryName)
+    : join(appRootPath(), 'bin', binaryName)
+}
 
 // ------------------------------------
 // Helpers
diff --git a/test/e2e/e2e.spec.js b/test/e2e/e2e.spec.js
index 0760032c..e8769781 100644
--- a/test/e2e/e2e.spec.js
+++ b/test/e2e/e2e.spec.js
@@ -2,6 +2,8 @@ import { Application } from 'spectron'
 import electronPath from 'electron'
 import path from 'path'
 
+jest.unmock('electron')
+
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 15000
 
 const delay = time => new Promise(resolve => setTimeout(resolve, time))
diff --git a/test/unit/__mocks__/electron.js b/test/unit/__mocks__/electron.js
new file mode 100644
index 00000000..e4cb4e68
--- /dev/null
+++ b/test/unit/__mocks__/electron.js
@@ -0,0 +1,13 @@
+const { normalize } = require('path')
+
+module.exports = {
+  require: jest.fn(),
+  match: jest.fn(),
+  app: {
+    getPath: name => normalize(`/tmp/zap-test/${name}`),
+    getAppPath: () => normalize('/tmp/zap-test')
+  },
+  remote: jest.fn(),
+  dialog: jest.fn(),
+  BrowserWindow: jest.fn()
+}
diff --git a/test/unit/lnd/lnd-config.spec.js b/test/unit/lnd/lnd-config.spec.js
index 80ca746a..4767f20b 100644
--- a/test/unit/lnd/lnd-config.spec.js
+++ b/test/unit/lnd/lnd-config.spec.js
@@ -4,46 +4,25 @@ import { join, normalize } from 'path'
 import Store from 'electron-store'
 import LndConfig from 'lib/lnd/config'
 
-jest.mock('grpc')
-
-jest.mock('electron', () => {
-  const { normalize } = require('path')
-
-  return {
-    app: {
-      getPath: name => normalize(`/tmp/zap-test/${name}`),
-      getAppPath: () => normalize('/tmp/zap-test')
-    }
-  }
-})
-
+jest.mock('electron-store')
 jest.mock('lib/lnd/util', () => {
-  const { normalize } = require('path')
-
   return {
     ...jest.requireActual('lib/lnd/util'),
-    appRootPath: normalize('/tmp/zap-test/app/root'),
     binaryName: 'binaryName',
-    binaryPath: 'binaryPath'
+    binaryPath: () => 'binaryPath'
   }
 })
 
-Store.prototype.set = jest.fn()
-
 describe('LndConfig', function() {
   const checkForStaticProperties = () => {
     it('should have "binaryPath" set to the value returned by lib/lnd/util', () => {
       expect(this.lndConfig.binaryPath).toEqual('binaryPath')
     })
     it('should have "configPath" set to "resources/lnd.conf" relative to app root from lib/lnd/util"', () => {
-      expect(this.lndConfig.configPath).toEqual(
-        normalize('/tmp/zap-test/app/root/resources/lnd.conf')
-      )
+      expect(this.lndConfig.configPath).toEqual(normalize('/tmp/resources/lnd.conf'))
     })
     it('should have "rpcProtoPath" set to "resources/rcp.proto" relative to app root from lib/lnd/util"', () => {
-      expect(this.lndConfig.rpcProtoPath).toEqual(
-        normalize('/tmp/zap-test/app/root/resources/rpc.proto')
-      )
+      expect(this.lndConfig.rpcProtoPath).toEqual(normalize('/tmp/resources/rpc.proto'))
     })
   }
 
diff --git a/test/unit/lnd/neutrino.spec.js b/test/unit/lnd/neutrino.spec.js
index c89c3489..012e1f13 100644
--- a/test/unit/lnd/neutrino.spec.js
+++ b/test/unit/lnd/neutrino.spec.js
@@ -1,15 +1,6 @@
 import Neutrino from 'lib/lnd/neutrino'
 
-jest.mock('electron', () => {
-  const { normalize } = require('path')
-
-  return {
-    app: {
-      getPath: name => normalize(`/tmp/zap-test/${name}`),
-      getAppPath: () => normalize('/tmp/zap-test')
-    }
-  }
-})
+jest.mock('electron-store')
 
 describe('Neutrino', function() {
   describe('Constructor', () => {