Allow creation of reusable lnurl-pay endpoints #78
Conversation
- add a new table 'lnurlpay_endpoint' and a column to the 'invoice' table for loosely referecing the lnurl-pay endpoint an invoice is related to when applicable - add /endpoint* routes for creating and managing lnurl-pay endpoints - add /lnurl/:endpoint_id* routes callable by wallets that return invoices
|
Wow, nice! This is very cool and definitely desirable. :) I'll refresh my memory on the lnurl-pay specs and try to give this a review in the next few days, but the overall direction looks good from what I've seen so far. Thanks for the contribution! |
src/lnurl.js
Outdated
|
|
||
| function addBech23Lnurl (req, lnurlpay) { | ||
| const hostname = req.hostname || req.params.hostname | ||
| const url = `https://${hostname}/lnurl/${lnurlpay.id}` |
There was a problem hiding this comment.
This assumes that the protocol is https and that charge is served from the root / path, which may not be true. This also uses the user-provided hostname (sent via the Host header), which may not be reliable.
This should use the URL environment variable, which is already used by the built-in checkout page as the root url of where the charge instance is publicly accessible to payers. This option is not documented in the CLI help text though, would be cool if you could add it :)
If the URL option is not provided, this could fall back to guessing the URL. But I am bit wary about using the Host header, need to give this some more thought and make sure that its not potentially exploitable somehow.
There was a problem hiding this comment.
Right, I will use that, but I thought using req.hostname was fine since you have app.set('trust proxy', process.env.PROXIED || 'loopback') there in place, which means you gave this some thought.
If I remember correctly, req.hostname will use the value from the Host header, but if X-Forwarded-Host is present it will use that instead as long as you have set app.set('trust proxy', true).
So maybe we should use process.env.URL and fallback to https://${req.hostname}? (Usage of https is required according to the LNURL spec so I think it is fine to hardcode that if no process.env.URL is found.)
(That req.params.hostname was arbitrarily inserted there to aid myself and others on a development environment, but it is probably doing more harm than good.)
src/model.js
Outdated
| , desc = props.description ? ''+props.description : defaultDesc | ||
| , lninv = await ln.invoice(msatoshi || 'any', id, desc, expiry) | ||
| , desc = props.descriptionHash || (props.description ? ''+props.description : defaultDesc) | ||
| , method = props.descriptionHash ? 'invoicewithdescriptionhash' : 'invoice' |
There was a problem hiding this comment.
By the way, let me note here that none of this will work on c-lightning unless you have the method invoicewithdescriptionhash, which is provided by this plugin.
Should I check at startup that this method exists by calling help and print something to the console if not? Or disable the lnurl-pay thing entirely?
There was a problem hiding this comment.
I would disable it. The way its implemented makes that pretty easy, the lnurlpay express setup is already self-contained and decoupled from the main app.
While at it, maybe also make it optional via an NO_LNURLPAY option? (on by default)
I wouldn't bother with disabling the new migrations if lnurlpay is not enabled, just let it create an unused table/column.
As an aside, lnurl-pay could theoretically specify that invoices may use the hash as an hex-encoded string descriptor and that wallets should check for both options, which would make compatibility easier.
There was a problem hiding this comment.
Awesome, thanks!
Yes, I agree with that last suggestion, but I don't know if it's too late for that by now.
There was a problem hiding this comment.
What is the rationale for having a NO_LNURLPAY option? Users who don't want lnurlpay won't notice anything, the lnurlpay API routes will never be called and the lnurlpay tables will be forever empty and that's it.
On the other hand, if someone wants to use the lnurlpay feature they will very likely miss the fact that there is a NO_LNURLPAY option that is on by default and have a lot of frustration.
There was a problem hiding this comment.
Ok, I did disable the lnurlpay routes when the invoicewithdescriptionhash method isn't found, but I think that is also confusing. What if a middleware was defined inside src/lnurl.js that returned a more informative error about the missing plugin (and printed something to the console?) only when lnurlpay routes were called?
|
Sorry for taking so long to address your points and pick up this task again. Let's hope I can do it right this time! |
|
Nevermind, some things are still broken and I don't understand why. I'll debug more later. |
lnurlpay_endpointand a column to theinvoicetable for loosely referencing the lnurl-pay endpoint an invoice is related to when applicable/endpoint*routes for creating and managing lnurl-pay endpoints/lnurl/:endpoint_id*routes callable directly by walletsmessagesuccessActionurlsuccessActionaessuccessActionBefore I finished everything and added tests I wanted to get some feedback. Is this thing desirable here? I think it fitted quite nicely with the purpose of lightning-charge and the codebase was specially welcoming 🙂