receiver and sender fallback #1523

riccardobl · 2024-10-27T13:49:12Z

The PR implements fallbacks on receiver, lnurlp for attached wallets and does some refactoring to the wallet interface.

Closes #1496
Closes #1494
partially solves #1558 by cancelling and creating a new invoice before showing the qr code

Changelog:

made pessimistic actions retryable
implemented receiver fallback
implemented sender fallback

Overview

This PR enhances payment reliability between sender and receiver wallets by adding fallback mechanisms.

The first part ensures that if a receiver’s wallet fails to generate a valid and wrappable invoice, the system will automatically attempt to create the invoice with the next available wallet, repeating this process until a valid invoice is obtained.

The second part addresses situations where a sender might have difficulty paying the invoice. Instead of simply trying a single payment path, the PR introduces a retry system that cycles through different sender-receiver wallet pairs. This way, if the first pair can’t connect, the system will increment an attempt counter and try another receiver wallet to find a working path. This approach helps avoid payment failures caused by both wallets being online but unable to connect directly.

If these attempts still don’t succeed, the system will prioritize fee credits or reward sats. ie. if a p2p payment was prioritized initially by the action but fails, the system will reprioritize internal payment options. This prioritization allows the payment to go through automatically if sufficient funds are available. If these methods do not work, the system will fallback to the next paymentMethod in the action, and generate a new invoice, which the client displays as a QR code for the user to complete the payment manually.

P2P preference flow

When a paid action is tried, the serverside code will go through the payment methods supported by the action, following the priority order. This behavior can be mutated by passing:

forceInternal : to use only the internal payment methods that are supported by the action (currently only FEE_CREDIT)
prioritizeInternal : to move the internal payment methods at the beginning in the priority list, making them the preferable methods while keeping all the others as fallbacks.

The paid action mutation returns a retriable flag that signals the sender it can retry the action using retryPaidMutation without changing anything, but just by increasing the attempt counter. This means the server side code has other ways to make the the transaction happen that were not tried yet. In the scope of this PR it means it has more p2p attachments to try.

Whenever a payment fails, the clientside code queries the paidAction to check if there is an invoiceForward's withdrawl status

if this is present the clientside code can determine that the payment went through on his side but failed to be forwarded on the other side, this will make the clientside code increase the attempt counter and retry if retriable is true, to let the serverside code use the next p2p attachment on the receiving side
if this is not present the clientside code will mark the sender as failed and retry with the next sender without increasing the attempt counter. This in turn will make the serverside code reuse the same receiving attachment.

If all the senders have failed or the retriable flag is false (all the receiver have failed), the clientside code will fallback to retry the payment using prioritizeInternal to hopefully complete it with FEE_CREDITS, if this fails too an invoice will be returned and passed to the qr code payment.

Note: this is not explicit in the code, but it is an effect of the logic: if we previously tried all the receivers and the forward failed for all of them, we will have an attempt counter high enough to exceed the number of receiving wallets, this will skip the p2p method, as it should, since there is no way to pay, even by qr code.

Non P2P flow (not internal priority)

The paid actions is tried, the response is returned with a retriable flag always set to false and the invoice to pay.
The clientside code tries to pay with every sender, once all the senders have failed it fallbacks to use prioritizeInternal, if this fails, an invoice is returned for the qr code

Non P2P flow (internal priority)

The paid actions is tried, the response is returned with a retriable flag always set to false and the invoice to pay.
The clientside code tries to pay with every sender, once all the senders have failed it fallbacks to use prioritizeInternal(FIXME: this is useless, it attempts the same thing twice, it should just skip instead)

if this fails, an invoice is returned for the qr code.

Note on invoice reuse

Every time a new attempt is made, the old invoice is cancelled and a new invoice is regenerated

Tests

setup

stacker1:
- fee credits: 1000
- sending attachments [good, bad]
- receiving attachments []
stacker2:
- fee credits: 0
- sending attachments: [bad, good]
- receiving attachments: []
stacker3:
- fee credits: 1000
- sending attachments: [bad,bad]
- receiving attachments []
stacker4:
- fee credits: 0
- sending attachments: []
- receiving attachments: [good, bad]
stacker5:
- fee credits: 0
- sending attachments: []
- receiving attachments: [bad, good]
stacker6:
- fee credits: 0
- sending attachments: [bad]
- receiving attachments: [bad, bad]
stacker7:
- fee credits: 0
- sending attachments: []
- receiving attachments: []
stacker8:
- fee credits: 1 000 000
- sending attachments: []
- receiving attachments: []

Zap

send receive	stacker1	stacker2	stacker3	stacker4
stacker4	[x] p2p	[x] p2p	[x] cc	-
sacker5	[x] p2p	[x] p2p	[x] cc	[x] qr
stacker6	[x] cc	[x ] cc	[x] cc	[x] qr
stacker7	[x] cc	[x] cc	[x] cc	[x] qr

post result

stacker1 posts spending fee credits
stacker2 posts spending from attached wallet
stacker3 posts spending fee credits
stacker4 shows qr
stacker5 shows qr
stacker6 fails, shows qr on manual retry

LNURLP

stacker3: cc
stacker4: p2p
stacker5: p2p
stacker6: cc

Territory

stacker1 pays from attached wallet
stacker3 pays from qr code (bug: if you cancel the qr code it goes to error 404)
stacker8 pays from fee credits

riccardobl · 2024-11-02T16:46:54Z

on hold until #1507 is merged, i will rebase any change on top of the new code

riccardobl · 2024-11-06T19:54:36Z

components/use-paid-mutation.js

+      }
+      await waitForQrPayment(invoice, null, { persistOnNavigate, waitFor })
+      return { invoice, response }
+    }


anons gets the qr code right away

api/paidAction/boost.js

riccardobl · 2024-11-09T19:38:06Z

api/paidAction/index.js

-    }
+    context.me = context.me ? await context.models.user.findUnique({ where: { id: context.me.id } }) : undefined
+    context.actionAttempt = context.actionAttempt ?? 0
+    context.forceInternal = context.forceInternal ?? false


This replaces forceFeeCredits and it is currently used by things like auto territory renewal to force the code to use only internal payments (fee credits and reward sats).
The difference between forceInternal and the old forceFeeCredits is that forceInternal allows the code to use reward sats too, not only fee credits.

riccardobl · 2024-11-09T19:40:36Z

api/paidAction/index.js

+    context.me = context.me ? await context.models.user.findUnique({ where: { id: context.me.id } }) : undefined
+    context.actionAttempt = context.actionAttempt ?? 0
+    context.forceInternal = context.forceInternal ?? false
+    context.prioritizeInternal = context.prioritizeInternal ?? false


Contrary to forceInternal, this will just prioritize reward sats and fee credits.
This is used by the client side code after every sending wallet failed to pay every receiving wallet, so it will try to do an internal transaction, if this fails too they payment will fallback to the other methods in order, this gives another invoice that is then presented to the user with a qr code.

riccardobl · 2024-11-09T19:58:18Z

api/resolvers/notifications.js

+          SELECT 1
+          FROM "ItemAct"
+          WHERE "ItemAct"."invoiceId" = "Invoice".id
+        )


when we have a failed invoiced action that it is then retried successfully with a fee credit or reward sats payment, we have this situation where there is a failed invoice that used to be bound to this action but it is not anymore.
This check is to exclude it from notifications. Maybe there are other ways, but i am unsure.

api/resolvers/paidAction.js

riccardobl · 2024-11-09T20:01:21Z

components/use-paid-mutation.js

+        setInnerResult(r => addPayError(e, r))
+      })
+
+      if (wait) await p


i've de-duplicated the code, but the logic should be the same

riccardobl · 2024-11-13T13:01:44Z

Rebased to master

Because the sending and receiving routes will succeed and fail independently of each other, I don't think we need to walk through all combinations. e.g. if send1 succeeds at push money to SN, then we only need to retry recv1 through recvN. If recv1 through recvN are all failing, send2 won't succeed to get money to recv1 through recvN, nor will send3.

Does that make sense? Does the code take this into account?

now this is taken into account.

The paid mutation returns a retriable flag and if the payment fails the client queries the paidAction invoiceForward's withdrawl status, at this point it has all the data it needs to determine which side is failing

when the mutation returns a withdrawl status that is set but not CONFIRMED

if retriable is true: the client will call retryPaidMutation passing an increasing 'attempt' counter -> this will walk to the next receiver in the priority sorted list
if retriable is false: it means there are no more receivers, the client will fallback to a fee credit payment

if the mutation doesn't have a withdrawl status

it means the sender failed: the client will mark the sender as failed and try the next one without increasing the attempt counter
when every sender has failed, the client will not retry but instead fall back to a fee credit payment

if the fee credit fails: the client will fallback to the qr code using the last attempt counter value.

Note: this is not explicit in the code, but it is an effect of the logic: if we previously tried all the receivers and the forward failed for all of them, we will have an attempt counter high enough to exceed the number of receiving wallets, this will skip the p2p method, as it should, since there is no way to pay, even by qr code.

huumn · 2024-11-13T15:52:08Z

api/paidAction/index.js

+        console.log(`trying payment method ${paymentMethod}`)
+        switch (paymentMethod) {
+          case PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT: {
+            if (!me || (me.msats ?? 0n) < cost) break // if anon or low balance skip


The original code that looks for violates check was there for a reason. The way you've rewritten this suppresses errors that aren't related to payments.

The switch statement is nice, but be careful causing regressions by changing code that you don't understand - and more importantly, code that isn't directly related to your changes.

Stuff like this is what causes me to do full rewrites.

ekzyis

I didn't run the code yet, but this is my review so far from looking at most of the code

wallets/server.js

api/paidAction/index.js

ekzyis · 2024-11-17T04:45:58Z

api/paidAction/index.js

+    if (forceInternal) {
+      //  we keep only the payment methods that qualify as internal payments
+      if (!me) {
+        throw new Error('user must be logged in to use internal payments')
+      }
+      const forcedPaymentMethods = []
+      // reset the supported payment methods to only include internal methods
+      // that are supported by the action
+      if (context.supportedPaymentMethods.includes(PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT)) {
+        forcedPaymentMethods.push(PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT)
+      }
+      // TODO: add reward sats
+      // ...
+      if (forcedPaymentMethods.length === 0) {
+        throw new Error('action does not support internal payments')
+      }
+      context.supportedPaymentMethods = forcedPaymentMethods
+    } else if (prioritizeInternal) {
+      // prefer internal payment methods over the others (if they are supported)
+      const priority = {
+        [PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT]: -2
+        // add other internal methods here
+      }
+      context.supportedPaymentMethods = context.supportedPaymentMethods.sort((a, b) => {
+        return priority[a] - priority[b]
+      })


Reward sats don't need to be a separate payment method since they can only be used if you pay with fee credits (they get converted into ccs just-in-time).

I think if you add reward sats as a separate payment method, that would mean that they can also be used for p2p zaps which is not what we want.

This means that forcedPaymentMethods doesn't have to be an array, it will always only contain FEE_CREDIT.

This means I also think the distinction between "internal payment method" and "fee credits" isn't needed. Internal always means fee credits.

it seems like they will be implemented as a separated payment method in ex-nov-5 pr, that's why i made the distinction

https://github.com/stackernews/stacker.news/blob/nov-5/api/paidAction/itemUpdate.js#L9-L13

Mhh, I see. @huumn, does it make sense to add REWARD_SATS as a payment method? I understood it as an "implementation detail" of the FEE_CREDIT payment method where they are converted into CCs just-in-time. In that case, isn't it unnecessary as a separate payment method?

api/paidAction/index.js

components/payment.js

api/paidAction/index.js

wallets/server.js

ekzyis

I looked through the sending code in detail now and tested the fallback using LNbits that fails and NWC that succeeds. I noticed that the client cancels the invoice. Is there a specific reason for that?

I still need to test more, especially the receiving part.

ekzyis · 2024-11-18T05:22:05Z

components/qr.js

-      const usableWallets = wallets.filter(w => !w.def.isAvailable || w.def.isAvailable())
-        .filter(w => w.config?.enabled && canSend(w))[0]
-      if (automated && usableWallets.length > 0) {
-        for (const wallet of usableWallets) {
+      if (automated && wallets.length > 0) {
+        for (const wallet of wallets) {


I am sorry, I was wrong here. The code I told you would run in useWallets does actually not run in useWallets but in useWallet. I confused them.

However, this means there is something else wrong: the wallets returned in useWallets do not contain sendPayment so this will fail with TypeError: wallet.sendPayment is not a function. You need to call useWallet to have access to this function. Additionally, usableWallets is not an array because of the [0] at the end.

Since you have similar code in usePaidMutation (where it works since you do call useWallet there), I think it makes sense to create a new hook useEnabledWallets which then returns an array of wallets you can use here and in usePaidMutation.

Again, sorry for the confusion. 🙈

No worries, this part of the code has been rebased so many times that I've lost track of the changes as well

ekzyis · 2024-11-18T05:38:14Z

components/use-paid-mutation.js

+  const senderWallets = walletDefs
+    .map(w => useWallet(w.def.name))
+    .filter(w => !w.def.isAvailable || w.def.isAvailable())
+    .filter(w => w.config?.enabled && canSend(w)).map(w => {
+      return { ...w, failed: false }
+    })


See comment in

https://github.com/stackernews/stacker.news/pull/1523/commits/4e96d5d2a80e1cfba65f1976b857c904722b5a94

about a new useEnabledWallets hook so we don't have to maintain this code at several places

had to use a code block for the URL since else Github automatically converts it into a link to the commit without the context of the PR and the comment ...

ekzyis · 2024-11-18T05:50:48Z

components/use-paid-mutation.js

+      await refreshInvoice()
+      if (!invoice) {
+        setInnerResult(r => addPayError(new Error('You must be logged in'), r))
+        throw new Error('You must be logged in')
+      }


I think this code isn't needed. Afaict, refreshInvoice is always called where invoiceUsed = false and with an invoice so if (!invoice) is also not needed.

ekzyis · 2024-11-18T06:09:21Z

components/poll.js

@@ -103,7 +103,7 @@ function PollResult ({ v, progress }) {
  )
 }

-export function usePollVote ({ query = POLL_VOTE, itemId }) {
+export function usePollVote ({ query = POLL_VOTE, itemId, ...options }) {


Is ...options needed? I don't see usePollVote called with anything else than query or itemId.

ekzyis · 2024-11-18T06:45:01Z

api/paidAction/index.js

-    actionId
+  if (failedInvoice.actionState !== 'FAILED') {
+    // you should cancel the invoice before retrying the action!
+    throw new Error(`actions is not in a retriable state: ${failedInvoice.actionState}`)


Why does the client have to cancel the invoice? Can't we do it here?

Canceling on the client is an additional round-trip and the code might be simpler if retrying implicitly cancels the existing invoice.

What do you think?

Hmm, I did consider implementing it like you've suggested but i've kept it this way because it is what the original code does.

But thinking about this in more depth, i think you are right, and that probably the transaction isolation should be serializable, because there is a race condition here (and in the original code), where two transactions could start with a committed 'failed' invoice state, but then end up with two different results.

What do you think?

riccardobl marked this pull request as draft October 27, 2024 22:54

riccardobl changed the title ~~minor wallet refactoring (server only)~~ fallback + lnurlp and minor refactoring with types (server only) Oct 28, 2024

riccardobl mentioned this pull request Oct 28, 2024

Improve wallet fallbacks #1489

Closed

riccardobl force-pushed the walletref branch 2 times, most recently from 8b22e03 to e647d2e Compare October 30, 2024 00:19

riccardobl added the blocked label Nov 2, 2024

riccardobl force-pushed the walletref branch from 49e4fb9 to 36c8cfb Compare November 4, 2024 13:39

riccardobl changed the title ~~fallback + lnurlp and minor refactoring with types (server only)~~ receiver fallback and p2p lnurlp Nov 4, 2024

riccardobl removed the blocked label Nov 4, 2024

riccardobl marked this pull request as ready for review November 4, 2024 14:05

riccardobl marked this pull request as draft November 4, 2024 21:36

riccardobl changed the title ~~receiver fallback and p2p lnurlp~~ receiver and sender fallback and p2p lnurlp Nov 4, 2024

riccardobl force-pushed the walletref branch 2 times, most recently from c36b04e to 860fab0 Compare November 6, 2024 13:08

riccardobl commented Nov 6, 2024

View reviewed changes

riccardobl mentioned this pull request Nov 8, 2024

Timeout wallet payments #1558

Open

riccardobl changed the base branch from master to nov-5 November 8, 2024 15:58

riccardobl force-pushed the walletref branch from 6bffd20 to 78988a7 Compare November 8, 2024 15:59

riccardobl force-pushed the nov-5 branch from f762165 to 9f9804a Compare November 8, 2024 16:42

riccardobl force-pushed the walletref branch 4 times, most recently from 707826c to 994b69d Compare November 9, 2024 19:30

riccardobl commented Nov 9, 2024

View reviewed changes

api/paidAction/boost.js Show resolved Hide resolved

riccardobl commented Nov 9, 2024

View reviewed changes

api/resolvers/paidAction.js Show resolved Hide resolved

riccardobl commented Nov 9, 2024

View reviewed changes

riccardobl changed the base branch from nov-5 to master November 13, 2024 12:29

riccardobl marked this pull request as ready for review November 13, 2024 12:32

stackernews deleted a comment from huumn Nov 13, 2024

huumn reviewed Nov 13, 2024

View reviewed changes

riccardobl force-pushed the walletref branch 2 times, most recently from 68bf879 to 171dcfd Compare November 14, 2024 13:41

better fallbacks

03ec61f

riccardobl force-pushed the walletref branch from 171dcfd to 03ec61f Compare November 14, 2024 13:54

riccardobl added 2 commits November 14, 2024 15:03

clean payment methods exception handling

0223c9e

fix

a016427

riccardobl force-pushed the walletref branch from 952d705 to a016427 Compare November 14, 2024 14:26

riccardobl added 8 commits November 14, 2024 15:37

comments

bf04342

expose only the withdrawl status

22c0e80

bail early if the invoice was canceled or expired

0fab13d

login check for retryPaidAction

468df05

comments

0e8b8ce

cleanup

a6ffcda

Merge remote-tracking branch 'upstream/master' into walletref

0df1b28

lint fix

8567815

ekzyis reviewed Nov 17, 2024

View reviewed changes

riccardobl added 3 commits November 17, 2024 10:43

fix merging issue and apply review suggestions

4e96d5d

remove unrelated change

f94c92b

remove unused import

03f9a92

ekzyis reviewed Nov 18, 2024

View reviewed changes

apply review suggestions

811aa6c

riccardobl requested a review from ekzyis November 18, 2024 09:49

riccardobl added 3 commits November 18, 2024 18:26

fix bigint conversion lost in merge

f3e80b1

move wallet map to failing states in use-paid-mutation

6a1d184

Merge branch 'master' into walletref

67a17cc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receiver and sender fallback #1523

receiver and sender fallback #1523

riccardobl commented Oct 27, 2024 •

edited

Loading

riccardobl commented Nov 2, 2024

riccardobl Nov 6, 2024

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024

riccardobl Nov 9, 2024

riccardobl commented Nov 13, 2024 •

edited

Loading

huumn Nov 13, 2024

ekzyis left a comment

ekzyis Nov 17, 2024

riccardobl Nov 17, 2024

ekzyis Nov 18, 2024

ekzyis left a comment

ekzyis Nov 18, 2024

riccardobl Nov 18, 2024

ekzyis Nov 18, 2024

ekzyis Nov 18, 2024

ekzyis Nov 18, 2024

ekzyis Nov 18, 2024

riccardobl Nov 18, 2024 •

edited

Loading

receiver and sender fallback #1523

Are you sure you want to change the base?

receiver and sender fallback #1523

Conversation

riccardobl commented Oct 27, 2024 • edited Loading

Overview

P2P preference flow

Non P2P flow (not internal priority)

Non P2P flow (internal priority)

Note on invoice reuse

Tests

setup

Zap

post result

LNURLP

Territory

riccardobl commented Nov 2, 2024

Choose a reason for hiding this comment

riccardobl Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl commented Oct 27, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl commented Nov 13, 2024 •

edited

Loading

riccardobl Nov 18, 2024 •

edited

Loading