-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
fix: WebLN QR fallback for anon users #1858
base: master
Are you sure you want to change the base?
Conversation
89da4b4
to
53f63c6
Compare
components/use-paid-mutation.js
Outdated
try { | ||
walletInvoice = await invoiceHelper.retry(walletInvoice, { update: updateOnFallback }) | ||
} catch (err) { | ||
if (walletError.wallet === 'webln') { // show QR code for WebLN errors | ||
throw err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While thinking about it, the major errors that this would ignore are already handled: Invoice cancel, Invoice expiration. If I'm not mistaken, retryPaidAction checks for Invoice failed and Invoice already paid, in the context of anon payment this shouldn't really happen as every paidAction generates a new invoice
Edit: Tested with alby extension on timeout, on close and on error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, it's a bit hard to understand the implications of these changes. For example, I am worried if the priority of the WebLN wallet makes a difference for these changes here.
I think a better way to fix #1758 is to make useWalletPayment
aware of if we're logged in or not.
If we're not logged in, try to pay with WebLN if available (typeof window.webln !== 'undefined'
) and ignore the rest of the code. That it currently might not ignore it for anons because WebLN might have been enabled (wallets.length > 0
) is the bug we should fix imo. 99% of our code for payments assumes you're logged in and thus that anons will never hit that code.
What anons can do in comparison to stackers is very limited (no sender and receiver fallbacks, no retries etc.) so I suspect if we (continue to) handle anons during payments immediately, it will be much easier to implement, maintain, read etc.
wallets/webln/client.js
Outdated
try { | ||
await window.webln.enable() | ||
} catch (err) { | ||
throw new WalletError('cannot re-enable wallet') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can also fail for other reasons, like if the user closed the prompt etc. so the error message can be misleading.
I think it should just reuse the WebLN error message:
throw new WalletError('cannot re-enable wallet') | |
throw new WalletError(err.message) |
ed59dd3
to
0eaf9d4
Compare
0eaf9d4
to
c819a05
Compare
Thanks a lot for the correct redirection, now we call WebLN's I think it's the correct behavior and gives a choice, if anything we could 'await' for sendPayment to throw an error to show the QR if we ever change idea. |
Description
Fixes #1758
When a WebLN wallet request fails (eg. cancelling a transaction) it can't be re-requested until next reload, throwing an error and prompting an invoice retry with QR that fails if the user is not logged in.
This fix checks for 'me' on
useWalletPayment
, throwing a newAnonWalletError
that can be caught during QR fallback to show both the WebLN client and the QR paymentScreenshots
Screen.Recording.2025-02-10.at.09.45.51.mov
Additional Context
Fix specifically applied for WebLN
Checklist
Are your changes backwards compatible? Please answer below:
Yes, only affects WebLN wallet
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:
7, more testing
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