Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow creation of reusable lnurl-pay endpoints #78
base: master
Are you sure you want to change the base?
Allow creation of reusable lnurl-pay endpoints #78
Changes from 2 commits
e8774fd
443940d
76ff8a5
7f0e9a6
62deba2
a5411e6
a6f7ade
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This does not appear to be in use, was the intention to allow denominating the
min
/max
values in fiat currencies?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.
Yes. I will implement that.
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.
I would read just the
comment
query parameter instead of keeping all of them minus amount/fromnodes/nonce. Also, this needs to enforce the length limit.So maybe something like
if (lnurlpay.comment && req.query.comment) invoiceMetadata.lnurlpay_comment = (''+req.query.comment).substr(0, lnurlpay.comment)
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.
The rationale for keeping the other query parameters is the following:
Imagine a shop like https://bitclouds.sh/. Every VM you buy there gets its own lnurlpay you can use to recharge it (not really true today but it's the main use case I have in mind and one that I expect to adopt this charge-lnurlpay feature once their refactor is ready).
So instead of asking Charge to create a new lnurlpay endpoint every time someone creates a VM, Bitclouds can pregenerate a single lnurlpay endpoint and modify its URL by adding an identifier for each VM.
If the base URL is
https://charge.bitclouds.sh/lnurl/recharge
it can then assignhttps://charge.bitclouds.sh/lnurl/recharge?vm=nusakan58
to the owner of a VM callednusakan58
. Then every time a payment is received at that Bitclouds will get a webhook containing{"vm": "nusakan58"}
and know what to do with it.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.
But your comment about the comment is very
commentivegood, I will integrate it (although I prefer to keep the field named ascomment
instead oflnurlpay_comment
, why not?).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.
What do you see as the use-cases for the endpoint metadata fields? I wonder if its worth keeping a copy of it alongside every invoice, or just a reference to the endpoint id. Duplicating the metadata might be worth it if the endpoint metadata fields are likely to change and its important to have a copy of the original fields at the time of payment.
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.
The use case I had in mind was an app storing stuff on an lnurlpay endpoint that is sent back to it on every webhook for payments made from that lnurlpay endpoint -- so the app can identify where the payment is coming from, basically, and have other data there to make it easier to reason.
I think it is not worth duplicating, it's better to fetch these again at webhook time.
But then I have to change the webhook dispatching logic in weird ways. Should I do it?
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.
Oh, there is also the websocket and the SSE interfaces, so it won't make sense to change the webhook code. Instead I must change the
src/lib/payment-listener.js
code to make it fetch metadata from the lnurlpay table for each received invoice. Do you think that is good?I think it is simpler to duplicate the metadata on each invoice, as ideally this metadata field shouldn't contain many things anyway, and in many cases I have in mind it is likely to be empty.
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.
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 theHost
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 theHost
header, need to give this some more thought and make sure that its not potentially exploitable somehow.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.
Right, I will use that, but I thought using
req.hostname
was fine since you haveapp.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 theHost
header, but ifX-Forwarded-Host
is present it will use that instead as long as you have setapp.set('trust proxy', true)
.So maybe we should use
process.env.URL
and fallback tohttps://${req.hostname}
? (Usage ofhttps
is required according to the LNURL spec so I think it is fine to hardcode that if noprocess.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.)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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?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.
The
metadata
field should always contain a valid JSON string even if no metadata was provided, or it'll fail parsing later when reading it. Simply removing theif metadata
check should do the trick.Also, one easy way to check if metadata is an object with some properties is checking for
Object.keys(props.metadata).length>0
, instead of finding out after serializing it. So you could do something likelnurlpay.metadata = JSON.stringify(props.metadata != null && Object.keys(props.metadata).length ? props.metadata : {})
.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.
Never mind the first part -- I just noticed the default metadata value set via knex.
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.
But
Object.keys([1, 2, 3])
will evaluate to['0', '1', '2']
, so that will falsely identify an array as an object, which we do not want!