Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve QR code flow for wrapped invoices #1712

Open
1 task done
ekzyis opened this issue Dec 11, 2024 · 7 comments
Open
1 task done

Improve QR code flow for wrapped invoices #1712

ekzyis opened this issue Dec 11, 2024 · 7 comments
Labels
feature new product features that weren't there before wallets

Comments

@ekzyis
Copy link
Member

ekzyis commented Dec 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When someone has a receiving wallet attached and we are anon or have no wallets attached, we will show a wrapped invoice in a QR code.

When the incoming payment arrives, the UI immediately considers the payment successful even though the forward can still fail.

Screenshots

2024-12-11.17-43-10.mp4

Steps To Reproduce

  1. Set max fee to 0 so forwards fail
  2. Pay someone with receiver wallet as anon
  3. QR code disappears because we use inv.satsReceived > 0 to check for success but payment failed

Expected behavior

Payment fails if forward failed and we immediately fallback to SN?

Logs

No response

Device information

No response

Additional context

This is basically the same bug as #1707 but in <Invoice> instead of in useInvoice.

@ekzyis ekzyis changed the title Failed forwards for anon payments are considered paid Failed forwards for QR payments are considered paid Dec 11, 2024
@huumn
Copy link
Member

huumn commented Dec 11, 2024

This is so that anon doesn't have to wait X seconds to find out it failed ... only to be able to do nothing about it.

@huumn
Copy link
Member

huumn commented Dec 11, 2024

This isn't a bug so much as it's a lack of a feature.

@ekzyis
Copy link
Member Author

ekzyis commented Dec 11, 2024

I knew you were going to say this 👀

I think as it is, this is super confusing UX and doesn't look trustworthy if we tell them their payment was successful while their wallet will tell them it failed. This is worse than just being transparent that the receiver wallet failed. If that's the first experience of someone with SN, they will think we can't even tell if someone paid successfully, lol.

Imo, we don't need to have optimistic UX for QR codes. If someone is going to pull out their phone to scan a QR code, I don't think they mind waiting a few seconds to see if it was actually successful.

The UI is also in a messed up state because we store the sats locally but the backend knows it didn't work. So it's possible to see an item with 0 sats but still have a zap bolt colored in 1000 sats.

only to be able to do nothing about it.

We can give them an option: try again with a SN invoice.

@huumn
Copy link
Member

huumn commented Dec 11, 2024

I don't think they mind waiting a few seconds to see if it was actually successful.

How about waiting 30 seconds to learn that it failed?

We can give them an option: try again with a SN invoice.

Yeah, this is a feature request.


We agree that this is an issue. I changed it to the current behavior because the regression you're advocating for sucked far worse IMO. I'm all for making this better though.

@ekzyis
Copy link
Member Author

ekzyis commented Dec 11, 2024

Okay, I see, we are basically disagreeing on if closing the QR code is better than keeping the QR code open for up to 30 seconds while it already says "sats received". My point is that closing the QR code is worse because that looks like a even more serious bug.

I'm all for making this better though.

Okay, I'll label this as a feature request then. My idea is to have an additional state for settling the invoice so they at least know what is going on and when it fails, we give them an option to retry as SN invoice so it won't take so long again. We discussed this somewhere already iirc.

@ekzyis ekzyis added feature new product features that weren't there before and removed bug labels Dec 11, 2024
@ekzyis ekzyis changed the title Failed forwards for QR payments are considered paid Improve QR code flow for wrapped invoices Dec 11, 2024
@huumn
Copy link
Member

huumn commented Dec 11, 2024

My point is that closing the QR code is worse because that looks like a even more serious bug.

You're only considering the current worst case. The average/success case of your solution is worse imo.

We need a solution that does not degrade the average case. If we are going to waitFor PAID, I want to:

  1. be able to close the QR and not have it cancel
  2. be able to do something with the error I receive

@ekzyis
Copy link
Member Author

ekzyis commented Dec 11, 2024

A cancel by the sender after sats have been sent should not be possible anyway. We had this bug and you fixed that before we deployed forwarding payments iirc. But I agree with everything else you said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before wallets
Projects
None yet
Development

No branches or pull requests

2 participants