Sender fallbacks #1642

ekzyis · 2024-11-23T05:22:31Z

Description

Partially solve #1494 for sender side

Based on #1651 #1660

TODO

fix payment check in sendPayment: wallets that are enabled but can only receive will throw SenderError instead of WalletNotConfiguredError
handle aggregated wallet errors in QR code
fix cache not updated with latest invoice on payment success or error
fix p2p zap retries create new invoices from SN, not from receiver
~~fix hmacs lost~~ doesn't matter, anon is not affected
fix zap retry in notifications not updating to paid when paid

Manual Tests

Screenshots

how errors from one or multiple wallets will now be shown:

Additional Context

I think the only reason we only allowed retries for optimistic actions was because retries weren't necessary yet for pessimistic actions.

We also should probably move all code from components/payment.js into wallets/payment.js but I didn't to not create more noisy changes than necessary.

Checklist

Are your changes backwards compatible? Please answer below:

tbd

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

tbd

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

tbd

Did you introduce any new environment variables? If so, call them out explicitly here:

no, not planned

wallets/payment.js

wallets/index.js

ekzyis · 2024-11-26T10:53:21Z

api/paidAction/index.js

-  if (!action.paymentMethods.includes(PAID_ACTION_PAYMENT_METHODS.OPTIMISTIC)) {
-    throw new Error(`retryPaidAction - action does not support optimism ${actionType}`)
-  }
-
-  if (!action.retry) {
-    throw new Error(`retryPaidAction - action does not support retrying ${actionType}`)
-  }
-


So far, I think it's okay to remove these checks to allow retries for pessimistic actions but I will make more sure

I see no problem with removing these. Retries for optimistic actions work like before and pessimistic actions support retries as expected now.

ekzyis · 2024-11-26T10:57:36Z

components/use-paid-mutation.js

+        walletError = err
+        // wallet payment error handling always creates a new invoice to retry
+        // unless no wallet was even able to attempt a payment
+        if (err.newInvoice) walletInvoice = err.newInvoice


this is maybe the weirdest part of this implementation of sender fallbacks: using an error to pass the new invoice to the calling context

This is very strange indeed, the newInvoice is not logically a property of the error.
Can't you extract the part that recreates the invoice or return it from waitForWalletPayment ?

Can't you extract the part that recreates the invoice or return it from waitForWalletPayment ?

I don't think so. Even if I extract it, I still need to pass the latest invoice to the calling context in some way. I can't use return since that would mean the wallet payment succeeded which isn't the case here.

I do return the latest invoice on success though, see the waitForPayment call for pessimistic actions below.

It might seem unimportant in the context this function is used, but these are things that build up technical debt, since now you have a function that leaks invoices, unless you remember that unexpectedly the error contains a new resource you need to cancel if you don't need it, that's why passing around new data in an error field is generally bad practice, errors should report only what and why something failed.

But, unless i'm missing something in the logic, I think you can make a proper error object simply by not recreating the last invoice but just cancelling it and setting a failedInvoice field in the error. Then when you catch it in waitForPayment you can recreate it with await invoiceHelper.retry(error.failedInvoice) only if it is needed.
eg. if skipQr is true, you won't even need to recreate it, right?

Thoughts?

Oh, I had it like this before but it looked awkward to do something different on the last payment attempt error (not create new invoice) so I changed it to this with less code.

But the argument about "new data" is a good one, I'll change it back to only save the failed invoice in the error and the caller can retry if they want so. Thanks!!

Did this in 96f76a9.

I realized that I can filter for only the wallets that are actually able to attempt a payment so the logic if there is another wallet we can try is indeed basically just if (i === wallets.length - 1). For some reason I didn't think the filter would work so I didn't even try and I think that's why I also didn't do it like this before.

This means the checks in sendPayment aren't really required anymore so maybe we should remove them 🤔

riccardobl

this implementation of retryPaidAction doesn't work for p2p actions, it will turn them to pessimistic actions that pay SN.

api/paidAction/index.js

riccardobl · 2024-11-27T09:17:02Z

components/use-paid-mutation.js

+        walletError = err
+        // wallet payment error handling always creates a new invoice to retry
+        // unless no wallet was even able to attempt a payment
+        if (err.newInvoice) walletInvoice = err.newInvoice


This is very strange indeed, the newInvoice is not logically a property of the error.
Can't you extract the part that recreates the invoice or return it from waitForWalletPayment ?

ekzyis · 2024-11-27T10:10:52Z

Oh, I didn't test p2p zaps. Thanks, will check and fix!

ekzyis · 2024-11-27T16:15:15Z

Need to change some more stuff like the flow when no wallet is available for 96f76a9. Will put in draft until this is ready for review again.

ekzyis · 2024-11-27T21:48:40Z

wallets/lnc/client.js

-    try {
-      const lnc = await getLNC(credentials, { logger })
-      const { paymentError, paymentPreimage: preimage } = await lnc.lnd.lightning.sendPaymentSync({ payment_request: bolt11 })
-      if (paymentError) throw new Error(paymentError)
-      if (!preimage) throw new Error('No preimage in response')
-      return preimage
-    } catch (err) {
-      const msg = err.message || err.toString?.()
-      if (msg.includes('invoice expired')) throw new InvoiceExpiredError(hash)
-      if (msg.includes('canceled')) throw new InvoiceCanceledError(hash)
-      throw err
-    }
+    const lnc = await getLNC(credentials, { logger })
+    const { paymentError, paymentPreimage: preimage } = await lnc.lnd.lightning.sendPaymentSync({ payment_request: bolt11 })
+    if (paymentError) throw new Error(paymentError)
+    if (!preimage) throw new Error('No preimage in response')
+    return preimage


I removed the try/catch here since we already have error handling in waitForPayment (which also checks for canceled or expired invoices) and in sendPayment which wrapps wallet.def.sendPayment.

I think this was just old code that we no longer need.

ekzyis · 2024-11-27T22:06:18Z

this implementation of retryPaidAction doesn't work for p2p actions, it will turn them to pessimistic actions that pay SN.

Fixed now. The code now retries the same receiver wallet if the forward didn't fail. If the forward failed, we immediately fallback to the SN node but we will try the next sender wallet.

I can see more clearly now why it makes sense to do sender + receiver fallbacks in one PR. The difference between both becomes blurry when we start to look into good error handling.

However, I think this is still an improvement that we can review and ship on its own. But with the // TODO: receiver fallbacks in the code, receiver fallback should be easy to add in another PR so ideally we can ship sender+receiver fallbacks together.

huumn · 2024-11-27T22:32:40Z

Cool, I'm going to try to refactor out the array of useWalletLogger(), so please don't force push.

huumn · 2024-11-27T23:34:33Z

Main thing I did in b608fb6 was

factor out the wallet-specific code from the generic "helper" logic
turn the wallet-specific code it into a hook that returns a "factory", i.e. factory produces a logger given a wallet

I'm not sure how generically we'll be able to apply this pattern but it works here.

Also (pats back):

Fought the urge to refactor all components/wallet-*.js

wallets/index.js

components/use-paid-mutation.js

huumn · 2024-11-28T00:43:02Z

Is this good to merge? I noticed you added another todo

It might be a consequence of my refactor, but because state is kept in memory, retries don't really work after navigation.

I don't think we'll be able to rely on memory post-auto-retries anyways, so I don't think that's blocking merging this. It's a strict improvement.

Also sorry about the renaming and Error refactors. It helped me understand what was going on to rename some stuff and encapsulate others.

huumn · 2024-11-28T03:02:16Z

I looked into the zap retry with fallback cache issue some. It's not clear why it's no longer working but these retry cache updates were the hardest part of my initial retry work. The item act retry and item retry are quite similar though - odd it's just affecting one of them.

The only thing I can thing of is that the retry mutation in useWalletPayment might be conflicting with the cache update

huumn · 2024-11-28T17:30:04Z

~~fix hmacs lost~~ doesn't matter, anon is not affected

Do we always lose hmacs? This is how we authenticate cancelations.

If that's inconsequential, this is ready to merge afaict.

ekzyis · 2024-11-28T17:33:55Z

Do we always lose hmacs?

Only in local storage and if we use attached wallets which poll invoices and return the latest state (= without hmac). Since anons don't have attached wallets, this doesn't matter for them (for now at least).

This is how we authenticate cancelations.

Afaik, cancels between retries use the original invoice state so they work. Since there was no error regarding cancels, I think they work but maybe good to make sure but I am not at home right now.

If that's inconsequential, this is ready to merge afaict.

This is also my view

huumn

Feel free to merge into master if there aren't any issues remaining.

ekzyis marked this pull request as draft November 23, 2024 05:22

ekzyis added feature new product features that weren't there before wallets labels Nov 23, 2024

ekzyis force-pushed the sender-fallbacks branch from d75da15 to 989ab5a Compare November 23, 2024 05:30

ekzyis mentioned this pull request Nov 23, 2024

refactor: Include logger and sendPayment in useWallets #1643

Closed

ekzyis force-pushed the sender-fallbacks branch 3 times, most recently from 7918299 to 10632fd Compare November 25, 2024 00:53

ekzyis commented Nov 25, 2024

View reviewed changes

wallets/payment.js Outdated Show resolved Hide resolved

ekzyis commented Nov 25, 2024

View reviewed changes

wallets/index.js Show resolved Hide resolved

ekzyis mentioned this pull request Nov 26, 2024

Fix actionArgs of new invoice during retry #1651

Merged

ekzyis force-pushed the sender-fallbacks branch 2 times, most recently from b5bdf36 to 765d9f3 Compare November 26, 2024 09:46

ekzyis commented Nov 26, 2024

View reviewed changes

ekzyis force-pushed the sender-fallbacks branch from 7636a99 to 4a2355a Compare November 26, 2024 22:35

ekzyis marked this pull request as ready for review November 26, 2024 22:46

ekzyis requested a review from riccardobl November 26, 2024 22:47

riccardobl reviewed Nov 27, 2024

View reviewed changes

ekzyis force-pushed the sender-fallbacks branch from 4a2355a to 96f76a9 Compare November 27, 2024 15:57

ekzyis marked this pull request as draft November 27, 2024 16:15

ekzyis force-pushed the sender-fallbacks branch 2 times, most recently from 7ce033e to 2398491 Compare November 27, 2024 21:46

ekzyis marked this pull request as ready for review November 27, 2024 21:46

ekzyis requested a review from riccardobl November 27, 2024 21:46

ekzyis commented Nov 27, 2024

View reviewed changes

ekzyis added 2 commits November 27, 2024 22:54

Remove unused useWallet from QR code component

35159bf

Fix comment

2b47bf5

Only retry same receiver if forward did not fail

7a8db53

ekzyis force-pushed the sender-fallbacks branch from 2398491 to 7a8db53 Compare November 27, 2024 22:00

huumn and others added 2 commits November 27, 2024 17:17

Merge branch 'master' into sender-fallbacks

61395a3

refactor out array of hooks

b608fb6

ekzyis commented Nov 27, 2024

View reviewed changes

wallets/index.js Outdated Show resolved Hide resolved

ekzyis commented Nov 27, 2024

View reviewed changes

components/use-paid-mutation.js Outdated Show resolved Hide resolved

ekzyis commented Nov 27, 2024

View reviewed changes

components/use-paid-mutation.js Outdated Show resolved Hide resolved

ekzyis added 2 commits November 28, 2024 01:16

Fix wallet save

a0d33a2

Fix [undefined] in logs

f9169c6

ekzyis force-pushed the sender-fallbacks branch from df72a13 to f9169c6 Compare November 28, 2024 00:37

readability improvements

67f6c17

ekzyis and others added 3 commits November 28, 2024 01:53

Fix missing item invoice update on failure

6c3301a

make logger use full wallet

f89286f

usesendwallets

6b59e1f

huumn added 2 commits November 28, 2024 10:48

fix zap fallback retries in notifications

cb028d2

make fallback retry cache updates a special case

105f7b0

function for merging data after retry

404cf18

huumn approved these changes Nov 28, 2024

View reviewed changes

fix cache update option name on qr

9caeca0

ekzyis merged commit 8ce2e46 into master Nov 28, 2024
6 checks passed

ekzyis deleted the sender-fallbacks branch November 28, 2024 22:00

This was referenced Dec 11, 2024

receiver and sender fallback #1523

Closed

Add timeouts to all wallet API calls #1722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sender fallbacks #1642

Sender fallbacks #1642

ekzyis commented Nov 23, 2024 •

edited by huumn

Loading

ekzyis Nov 26, 2024

ekzyis Nov 26, 2024

ekzyis Nov 26, 2024 •

edited

Loading

riccardobl Nov 27, 2024

ekzyis Nov 27, 2024

riccardobl Nov 27, 2024 •

edited

Loading

ekzyis Nov 27, 2024

ekzyis Nov 27, 2024 •

edited

Loading

riccardobl left a comment

riccardobl Nov 27, 2024

ekzyis commented Nov 27, 2024

ekzyis commented Nov 27, 2024 •

edited

Loading

ekzyis Nov 27, 2024

ekzyis commented Nov 27, 2024 •

edited

Loading

huumn commented Nov 27, 2024

huumn commented Nov 27, 2024 •

edited

Loading

huumn commented Nov 28, 2024 •

edited

Loading

huumn commented Nov 28, 2024

huumn commented Nov 28, 2024

ekzyis commented Nov 28, 2024 •

edited

Loading

huumn left a comment

Sender fallbacks #1642

Sender fallbacks #1642

Conversation

ekzyis commented Nov 23, 2024 • edited by huumn Loading

Description

TODO

Screenshots

Additional Context

Checklist

ekzyis Nov 26, 2024

Choose a reason for hiding this comment

ekzyis Nov 26, 2024

Choose a reason for hiding this comment

ekzyis Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl Nov 27, 2024

Choose a reason for hiding this comment

ekzyis Nov 27, 2024

Choose a reason for hiding this comment

riccardobl Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ekzyis Nov 27, 2024

Choose a reason for hiding this comment

ekzyis Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl left a comment

Choose a reason for hiding this comment

riccardobl Nov 27, 2024

Choose a reason for hiding this comment

ekzyis commented Nov 27, 2024

ekzyis commented Nov 27, 2024 • edited Loading

ekzyis Nov 27, 2024

Choose a reason for hiding this comment

ekzyis commented Nov 27, 2024 • edited Loading

huumn commented Nov 27, 2024

huumn commented Nov 27, 2024 • edited Loading

huumn commented Nov 28, 2024 • edited Loading

huumn commented Nov 28, 2024

huumn commented Nov 28, 2024

ekzyis commented Nov 28, 2024 • edited Loading

huumn left a comment

Choose a reason for hiding this comment

ekzyis commented Nov 23, 2024 •

edited by huumn

Loading

ekzyis Nov 26, 2024 •

edited

Loading

riccardobl Nov 27, 2024 •

edited

Loading

ekzyis Nov 27, 2024 •

edited

Loading

ekzyis commented Nov 27, 2024 •

edited

Loading

ekzyis commented Nov 27, 2024 •

edited

Loading

huumn commented Nov 27, 2024 •

edited

Loading

huumn commented Nov 28, 2024 •

edited

Loading

ekzyis commented Nov 28, 2024 •

edited

Loading