Use bitcoin-connect for self-custodial zaps #715
Conversation
ekzyis
changed the title
components/header.js
Outdated
| @@ -33,7 +33,8 @@ function WalletSummary ({ me }) { | |||
| if (me.privates?.hideWalletBalance) { | |||
| return <HiddenWalletSummary abbreviate fixedWidth /> | |||
| } | |||
| return `${abbrNum(me.privates?.sats)}` | |||
| const sats = me.privates?.sats + me.weblnSats | |||
There was a problem hiding this comment.
if either of these are undefined, I think it will cause a NaN value. Also, does it even make sense to combine the two values? you can only spend from one or the other
There was a problem hiding this comment.
if either of these are undefined, I think it will cause a NaN value.
You're right, it would cause a NaN value in that case. But I made sure in components/me.js:15 that me.weblnSats is never undefined. It's set to 0 in that case.
However, the optional chaining in me.privates?.sats makes this look like it might be undefined, yes. I'll double-check that this can actually never be the case, thanks! I think we're just using optional chaining in some cases where it's not really needed. (We'll probably keep the optional chaining since showing a NaN value somewhere is better than completely crashing and burning.)
Also, does it even make sense to combine the two values? you can only spend from one or the other
Good point, I also wasn't sure initially if I should combine them but now I think it does make sense. Currently, you can only spend from one or the other. If the user does not have enough sats in their custodial wallet, we show a QR invoice which then gets paid in full from this "WebLN balance" (not really happy with the name, btw).
But with some clever logic, I think we'll be able to spend from both wallets at the same time. For example, imagine someone only has 50 sats in their custodial wallet (me.privates.sats) and wants to pay a 100 sat invoice.
We should be able to use 50 sats from their custodial wallet and the remaining via WebLN (assuming they have enough sats there).
So I think it makes sense to keep these balances separate in the frontend code so we can implement such a thing by splitting invoices, for example. I only didn't get to this part yet.
There was a problem hiding this comment.
how would the combined balance work in practice? you'd need to send some sats to the custodial wallet and then pay the original invoice from the custodial wallet, right? I think this is probably too complicated and also might not even be needed (ideally users either go full self-custodial or full custodial)
There was a problem hiding this comment.
I think this is probably too complicated and also might not even be needed (ideally users either go full self-custodial or full custodial)
Users can get zapped on SN. So until we also have implemented receiving self-custodial (which is going to be nontrivial since we need to be able to take a fee for ranking and sybil resistance - see discussion in #524), it's going to be natural that users will use both wallets at the same time in the mean time.
Also, we don't intend to drop the custodial wallet anytime soon. It's good for onboarding users.
So in the case users want to start with self-custody, it's highly likely they already have some sats on SN. What to do with these sats if they want to connect a self-custodial wallet? Force them to withdraw first? I think that would be bad UX.
how would the combined balance work in practice? you'd need to send some sats to the custodial wallet and then pay the original invoice from the custodial wallet, right?
In my mind (fwiw), it's going to be simple enough. We'll just create two invoices. One invoice is not a real LN invoice since it's just a simple HTTP request, telling our backend that user X just zapped user Y. User balances on SN are just multiplexed over a single LN node with a SQL database.
The other one is a real LN invoice which can then be paid via WebLN.
We only have to make sure this happens atomically. So this should only work if both steps were successful.
/cc @huumn might have more to say to this.
There was a problem hiding this comment.
Until we have self-custodial receives (or people stop wanting to receive money hehe), we'll need to custody receives.
One alternative to combining balances is to only display custodial balances on SN and surfacing the self-custodial balance elsewhere.
There was a problem hiding this comment.
Since Mutiny will probably not support get_balance due to valid privacy concerns (see this comment in MutinyWallet/mutiny-node#895) and since we already have a UI/UX problem with mixing sats from two wallets, I think it makes sense to not show the self-custodial balance at all on SN. Users can always look up their self-custodial balance themselves in the connected wallet.
Also, @rolznz does have a point with this that I didn't really acknowledge in my previous reply. Mixing sats might indeed be more complicated than really necessary:
I think this is probably too complicated and also might not even be needed
So this means we would only show the custodial balance on SN (as before). The self-custodial balance is unknown to us but I believe if we implement zapping as follows, this shouldn't be a problem:
Assume we have X sats on SN and we have a WebLN provider connected.
If we now want to pay for something which costs Y sats with Y > X, we first ask the WebLN provider to pay for Y - X sats with a HODL invoice. If that succeeds, we know we can fully pay the invoice with the remaining known X sats. We then use these custodial sats to pay the remaining amount. If that also succeeds (which should be the case in 99.999% of all cases since it would basically use the same code as fully custodial zaps did all this time), we settle the HODL invoice. Payment done and we didn't need to know the self-custodial balance in advance. Using HODL invoices also shouldn't be a problem since all of this should only take a few seconds to settle. So from the PoV of the WebLN provider, it won't even notice it's a HODL invoice since it gets settled (or canceled) pretty quick.
Any thoughts on this approach @huumn @rolznz ?
update: I was able to immediately tick 3 checkboxes by simply not requiring get_balance. so this feels like the way to go to KISS it and support Mutiny and other WebLN providers which won't support get_balance anyway:
There was a problem hiding this comment.
I think it might be simpler to not doing any sort of combined payments, and just allow users to withdraw sats from custody to their own wallet (that already works)
Is there any downside there?
There was a problem hiding this comment.
(that already works)
With that, you mean that works for some WebLN providers? Or does it work for all of them already? I mentioned in the call yesterday that I wasn't thinking about Bitcoin Connect for receiving sats yet. So I might not be up to speed regarding this and may have missed some important parts about Bitcoin Connect. I only thought of it for sending sats so far.
Is there any downside there?
Yes, there is. Even if they would withdraw their sats to their own wallet, they will still receive sats into their custodial wallet until we also solved the receiving part in a way that makes sense for SN in a self-custodial way (#524). So this problem does not entirely go away with withdrawing sats before connecting.
But we don't even want people to withdraw their sats just to connect a wallet. That would be annoying. Maybe they just want to try out how connecting a wallet works without fully committing to it just yet? Maybe they don't feel comfortable with self-custody yet so they don't feel comfortable with withdrawing their whole balance from SN just yet? etc.
There was a problem hiding this comment.
With that, you mean that works for some WebLN providers? Or does it work for all of them already? I mentioned in the call yesterday that I wasn't thinking about Bitcoin Connect for receiving sats yet. So I might not be up to speed regarding this and may have missed some important parts about Bitcoin Connect. I only thought of it for sending sats so far.
What I mean is stacker news already supports paying an invoice for a withdrawal (and it also supports WebLN.makeInvoice). That invoice can come from any wallet.
So this problem does not entirely go away with withdrawing sats before connecting.
I don't mean before connecting. I just mean if the user wants to withdraw profits then they can withdraw to their own wallet.
I just think that trying to combine self-custodial + custodial to do payments (especially when partially paying from both) is going to be complicated. I think it would be easier if you connect a wallet using Bitcoin Connect, then zaps should come from your connected wallet. Otherwise use the custodial stacker news wallet. If they just want to test out self-custodial zaps and then go back to custodial zaps, they could just disconnect their wallet.
There was a problem hiding this comment.
If we now want to pay for something which costs Y sats with Y > X, we first ask the WebLN provider to pay for Y - X sats with a HODL invoice.
I think this makes sense. If it's any easier, having WebLN pay Y directly is probably sufficient for MVP when Y > X. But your intuition is correct in that paying for Y as Y - X non-custodial + X custodial is the ideal. It's like paying at a store with half cash and half credit.
I just think that trying to combine self-custodial + custodial to do payments (especially when partially paying from both) is going to be complicated.
Our plan is to eventually do that but we aren't ready to rely on an external wallet 100% yet. When we do that we inherit a user experience that we can't fully control and are inexperienced troubleshooting. Also:
- If a stacker has a balance on SN, paying with that balance has zero fees and paying with a non-custodial wallet has variable fees. Defaulting to the lower fee option seems like better UX.
- Some NWC wallets don't pay in real time. Defaulting to the real time option seems like better UX.
- Our code is designed around this custodial model so supplementing it with an external wallet is going be easier to start. It makes the external wallets and all the bugs and quirks of our implementation optional until we are confident in them.
We plan on doing something similar for non-custodial receives. Stackers will receive to their custodial wallets but we'll push out funds to an external wallet if their custodial wallet breaches a certain threshold.
In both cases, once we've nailed supplementing the custodial experience, we'll move to a model where there is no hybrid wallet.
components/webln.js
Outdated
| const connector = 'lnbits' | ||
| // TODO | ||
| // we're reaching into private parts here. | ||
| // is there a better way to show to user which connection was used? |
There was a problem hiding this comment.
What is exactly needed here? ideally webln.getInfo() returns the necessary info. the website should only interact with the WebLN protocol as much as possible
There was a problem hiding this comment.
webln.getInfo() (I use provider.getInfo() but I think this makes no difference and I think that's what you meant, too) returns this:
{
"node": {
"alias": "ek",
"pubkey": ""
},
"methods": [
"getInfo",
"getBalance",
"sendPayment"
],
"version": "1.0",
"supports": [
"lightning"
]
}
I thought it would be nice to show the user which wallet they connected. The provided alias is not sufficient imo.
ideally webln.getInfo() returns the necessary info. the website should only interact with the WebLN protocol as much as possible
👍 This just demonstrates the use case I was going for.
There was a problem hiding this comment.
If you click the connected button it will show where you are connected (via lnbits connector), and actually the LNbits hostname is stored in the bitcoin connect config. But does it help if you know the exact hostname, or would it be enough to know you're connected via LNbits?
I created an issue here: getAlby/bitcoin-connect#177 - maybe we can figure a way to expose some helpful info in a generic way for all connectors.
There was a problem hiding this comment.
But does it help if you know the exact hostname, or would it be enough to know you're connected via LNbits?
Mhh, I think it might be enough to know that I am connected to LNbits. Not entirely sure yet though. When we released the first iteration on this, user feedback will be more valuable than any feedback I can provide you on this as a developer.
There was a problem hiding this comment.
I removed the code that is reaching into private parts in e06e862 since getAlby/bitcoin-connect#177 should fix the use case I was solving for.
|
@ekzyis for improving the payment UX you could check out the Bitcoin Connect payment component which could potentially replace the existing Stacker News QR component. This will automatically not show the QR if you're already connected. You can try a demo here: https://bitcoin-connect-request-payment-modal--rolznz.repl.co/ (for example - connect a wallet, then close the modal without paying, then re-open the modal and it will not show the QR code). On mobile it also has buttons like "open in wallet" so should handle all ways for a user to pay. |
Ah, right, I didn't check out your payment component yet. However, ideally, there would be no modal at all. I want to keep the SN UX consistent with custodial zapping. I think one of the best selling points of SN is how easy it is to zap people. Anything that comes between the click and the zap is friction. So this is how I imagine it to continue working, even if we use a WebLN provider. There is only a modal if the user wants to configure a specific zap amount: 2023-12-28.14-00-13.mp4However, not sure yet if that's possible. But ideally, we'd get / stay as close to this UX as possible. I'll check out the payment component! |
This is the way. At nearly any cost, we don't want to regress UX or change UX just because we can. Improvements to UX are welcome however. This looks great so far @ekzyis. Nice placement of the connect button. I had a lot of trouble imagining where it could go. |
ekzyis
force-pushed
the
533-bitcoin-connect
branch
from
4bc3615 to
e555802
Compare
* created WebLNContext * added weblnSats local ield with @client directive including local field resolver * sum sats (custodial + webLN)
ekzyis
force-pushed
the
533-bitcoin-connect
branch
from
b95ff17 to
e06e862
Compare
|
|
||
| // if you want to store a function, you need to wrap it with another function because of updater functions | ||
| // see https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state | ||
| setLaunchModal(() => launchModal) |
There was a problem hiding this comment.
These functions are imported and then passed as state, could they just be imported where they are actually used? actually I don't think they are even used yet, are they?
There was a problem hiding this comment.
These functions are imported and then passed as state, could they just be imported where they are actually used?
I am importing all of them here so I can simply use what I need where I need it by calling useWebLN. If I would import them where they are actually used, I would spread useEffect with an async wrapper inside around our code base which is what I wanted to avoid. Simply calling useContext is a nicer syntax:
pages/wallet.js:
const { provider, info, launchModal } = useWebLN()actually I don't think they are even used yet, are they?
You're right, isConnected isn't used outside of this function and launchPaymentModal isn't used because of this but the other functions are. I anticipated that I would use these two functions at some point, too.
| useEffect(() => { | ||
| const unsub = [] | ||
| async function effect () { | ||
| const [isConnected, onConnected, onDisconnected, requestProvider, launchModal, launchPaymentModal, closeModal] = await import('@getalby/bitcoin-connect-react').then( |
There was a problem hiding this comment.
you could import init here too and then call it to set Stacker News as the name of the connecting app (this will work for at least NWC and maybe other connectors in the future)
|
I am using our Bitcoin Connect fork now. There were no changes required in this branch. I was able to use Setup instructions:
You should now be able to change code in the fork and after |
|
Closing in favor of #749 |
Close #533 Close #490 Close #75
With this PR, stackers can connect any wallet that is supported by Bitcoin Connect:
When connected, we can add this self-custodial balance to any sats they might hold in their custodial wallet:
If they now zap people and they don't have enough sats in their custodial wallet (these sats are spent first), they'll get the usual QR code. However, no further action is required by the user. Using the WebLN API provided by Bitcoin Connect, we can now pay the invoice automatically:
2023-12-28.03-03-50.mp4
TODO:
getInvoicewith subscriptions viasubscribeToInvoice(Use LND subscriptions #726)consider channel reserve when showing balance to user (?)deprecated. we don't needget_balance, see this commentallow mixed payments from custodial wallet AND WebLN provider at the same timeupdated. for payments above the custodial balance, use this approachmovedeprecated. we don't needme.weblnSatstome.privates.weblnSatsget_balance, see this commentlocalStorage?should we use a reactive variable fordeprecated. we don't needweblnSatsinstead of this (possibly hacky)createRef+useImperativeHandlecombination?get_balance, see this comment