Fix stale confirmedIndex used on reconnect

[ekzyis]

On reconnects, we used a stale value for confirmedIndex. This means that we would confirm all invoices again since the worker was started.

Found this while debugging #763 but this fixes not #763.

[huumn]

Kind of a nit, but do we need to require subscribeForever receives a promise just because in one case it's convenient to pass one.

If it's easy to make to the promise optional, I think that's what we should do.

[ekzyis]

Kind of a nit, but do we need to require subscribeForever receives a promise just because in one case it's convenient to pass one.

If it's easy to make to the promise optional, I think that's what we should do.

Good point, my initial fix tried to do this and it looked like this:

diff --git a/worker/wallet.js b/worker/wallet.js
index 99bd10bd..a91c8edf 100644
--- a/worker/wallet.js
+++ b/worker/wallet.js
@@ -22,11 +22,16 @@ function subscribeForever (subscribe) {
     let sub
     try {
       return await new Promise((resolve, reject) => {
-        sub = subscribe(resolve, bail)
+        const sub = subscribe(resolve, bail)
         if (!sub) {
-          return bail(new Error('function passed to subscribeForever must return a subscription object'))
+          return bail(new Error('function passed to subscribeForever must return a subscription object or promise'))
+        }
+        if (sub.then) {
+          // sub is promise
+          sub.then(sub => sub.on('error', reject))
+        } else {
+          sub.on('error', reject)
         }
-        sub.on('error', reject)
       })
     } catch (error) {
       console.error(error)
@@ -45,13 +50,12 @@ const logEventError = (name, error) => console.error(`error running ${name}`, er
 async function subscribeToDeposits (args) {
   const { models, lnd } = args

-  const [lastConfirmed] = await models.$queryRaw`
+  subscribeForever(async () => {
+    const [lastConfirmed] = await models.$queryRaw`
     SELECT "confirmedIndex"
     FROM "Invoice"
     ORDER BY "confirmedIndex" DESC NULLS LAST
     LIMIT 1`
-
-  subscribeForever(() => {
     const sub = subscribeToInvoices({ lnd, confirmed_after: lastConfirmed?.confirmedIndex })

     sub.on('invoice_updated', async (inv) => {

So if you prefer that, I can use that. I don't have a strong opinion about either approach.

[huumn]

Seems better to me. I prefer abstractions eat the complexity of using them.

[ekzyis]

Seems better to me. I prefer abstractions eat the complexity of using them.

Done. Tested by starting worker, paying an invoice, restarting LND and paying another invoice (to make sure reconnect worked).

On master (dc8d35f), the invoice is confirmed again after restarting LND which means another push notification is sent.