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.

Sign up for GitHub

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

Jump to bottom

Fix missing invoice retry before showing QR code #1674

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix missing invoice retry before showing QR code #1674

wants to merge 3 commits into from

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Dec 2, 2024 •
edited
Loading

Description

Fix #1672

This adds a new property retry to waitForQrPayment. If this is set (which it is by default), waitForQrPayment will first retry the invoice before showing it to the user.

To keep the code simple, invoices can now also be canceled without a hmac as long as it's your own invoice.

TODO:

  • fix pending still shown after QR invoice was canceled and we no longer are trying the attached wallets
  • fix retry vote fails with Invoice is not in failed state: RETRYING

Checklist

Are your changes backwards compatible? Please answer below:

yes

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:

n/a

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

no

@ekzyis ekzyis added the bug label Dec 2, 2024
@ekzyis ekzyis marked this pull request as draft December 2, 2024 02:31
@ekzyis ekzyis force-pushed the fix-pending-double-payment branch 2 times, most recently from cfb50f5 to 016b0e1 Compare December 2, 2024 02:36
@ekzyis ekzyis force-pushed the fix-pending-double-payment branch from 016b0e1 to 9940588 Compare December 2, 2024 02:39
@ekzyis ekzyis force-pushed the fix-pending-double-payment branch from e3b4bea to e805331 Compare December 2, 2024 14:16
@ekzyis
Copy link
Member Author

ekzyis commented Dec 2, 2024

As discussed in our meeting, I will not continue with this PR since it's not a big problem that we're still trying to pay with attached wallets in the background while we show the QR code. Receiver fallbacks (#1494) are more important currently.

If I remember correctly, it was also mentioned that the UX is not clear that clicking on pending cancels the payment in the background just to show a new invoice in the QR code. If you then close the QR code again, the wallets no longer attempt the payment in the background but only when you click on retry payment again, at which point it goes back to pending.

I personally think these UI state transitions (retry payment -> pending -> QR code -> ...) can be a good UX. But because implementing the necessary cache updates to make this work well is kind of complicated/messy, we should rethink how we do these cache updates before we attempt any such thing.

However, there's an issue with this that already exists and this PR doesn't fix: pending does not necessarily mean the wallets are attempting to pay since if you refresh the page, any payments attempts no longer happen but it still shows pending.

Fixing this would probably be the better first step to improve the user-facing code around retries.

@ekzyis ekzyis mentioned this pull request Dec 2, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug needs specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clicking on 'pending' does not prevent double payments
1 participant
@ekzyis