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

Clicking on 'pending' does not prevent double payments #1672

Open
1 task done
ekzyis opened this issue Dec 1, 2024 · 6 comments · May be fixed by #1674
Open
1 task done

Clicking on 'pending' does not prevent double payments #1672

ekzyis opened this issue Dec 1, 2024 · 6 comments · May be fixed by #1674

Comments

@ekzyis
Copy link
Member

ekzyis commented Dec 1, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When an item is created and the wallet is not immediately able to pay it, we show pending next to it.

When this is clicked, we show a QR code for the same invoice we're currently still attempting to pay in the background via the attached wallets.

This means it is possible that the invoice can get paid twice.

Screenshots

No response

Steps To Reproduce

  1. Attach wallet that doesn't respond
  2. Create item
  3. Click on pending
  4. Check console: we're still polling for the same invoice in the background

Expected behavior

Clicking on pending cancels the previous invoice and shows a new invoice.

Logs

No response

Device information

No response

Additional context

No response

@ekzyis ekzyis self-assigned this Dec 1, 2024
@huumn
Copy link
Member

huumn commented Dec 2, 2024

Clicking on pending cancels the previous invoice and shows a new invoice.

I don't think this is the expected behavior in all cases. Sometimes I click on pending to see how long the attached wallet has been attempting payment.

I agree with the general problem though. The current UI/UX allows double payments and it shouldn't.

@ekzyis ekzyis linked a pull request Dec 2, 2024 that will close this issue
2 tasks
@ekzyis
Copy link
Member Author

ekzyis commented Dec 2, 2024

Copying my comment in #1674 that I should probably have added here:

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.

-- #1674 (comment)

Sometimes I click on pending to see how long the attached wallet has been attempting payment.

Oh, interesting. If it's a fresh page load, don't you get the same information from the relative time? For example, 1m means here that the payment is pending since a minute:

2024-12-02-202359_312x73_scrot

@huumn
Copy link
Member

huumn commented Dec 2, 2024

That number doesn't update if I haven't navigated back to it. I'd probably still open anyway because I'd have to know/trust those numbers were correlated (and certainly the average stacker wouldn't know that).

@ekzyis
Copy link
Member Author

ekzyis commented Dec 4, 2024

The current UI/UX allows double payments and it shouldn't.

Maybe we should only stop attempting the wallets in the background but keep the same invoice. Once the QR code is closed, we can continue to attempt payment with the wallets again.

@huumn
Copy link
Member

huumn commented Dec 4, 2024

There's no way to stop the wallets in the background generically, e.g. after I've send off an NWC pay_invoice request or whatever it's called.

@ekzyis
Copy link
Member Author

ekzyis commented Dec 4, 2024

Ahh, right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants