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

Allow passing of description_hash to the add invoice call #317

Open
bumi opened this issue Oct 27, 2021 · 15 comments
Open

Allow passing of description_hash to the add invoice call #317

bumi opened this issue Oct 27, 2021 · 15 comments

Comments

@bumi
Copy link

bumi commented Oct 27, 2021

Is there a reason why the description_hash (and potentially other params) can not be passed on to LND?
https://github.com/bumi/LndHub/blob/master/controllers/api.js#L192

Allowing to pass in a description_hash would allow lndhub to be used as a lightning address backend.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

iirc when passing in description_hash: one lose the memo: and description: fields

i would recommend leave LndHub as is and create a side microsevice for lnurl related extensibility using the popular "BlueWallet Lightning Protocol" used in LndHub and elsewhere ...

@bumi
Copy link
Author

bumi commented Oct 27, 2021

iirc when passing in description_hash: one lose the memo: and description: fields

but that's up to whoever uses the LndHub API. Why should LndHub "protect" against this as this is how LND works. And there are reasons to use a description_hash.

regarding lnurl: afaik the payment request must have the LNURL metadata hash as description_hash - otherwise it is rejected. So a side microservice using lndhub would not work.
But I don't think LNURL is the only reason for support of the description hash.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

i made withdraw and pay with existing LndHub calling /addinvoicethat sets { memo, value } so i can not follow in Your reasoning ...

please expand

@bumi
Copy link
Author

bumi commented Oct 27, 2021

(there was a formatting issue in my previous reply)

/addinvoice does a call to the underlaying LND node. I am wondering why we do not allow passing more parameters like here the description_hash.
Even if that means a user can no longer set a memo, this is just the way LND works. And if somebody is using the LndHub API and wants to set a description_hash that is fine.
So I don't understand why not supporting the description_hash.

the description_hash is specifically needed for LNURL pay requests. The payment request returned by an lnurl pay endpoint must contain the hash of the LNURL metadata as description_hash.
Thus LndHub currently can not be used as a backend for an lnurl/lightning address service.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

correct in as "Thus LndHub currently can not be used as a backend for an lnurl/lightning address service."

that is why one should use a separate microservice for extending LndHub ! ...

i think we are confusing description_hash with payment_hash that i used with satisfactory result.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

also BlueWallet need be able read memo for presentation in its UI ?, whereas with description_hash is not really readable ! ...

@bumi
Copy link
Author

bumi commented Oct 27, 2021

The LNURL spec says:

Then, once the user accepts the terms (and choose an amount, if that is not fixed), the wallet will call the service and get a Lightning invoice specific for that payment, containing a hash of the metadata as its h tag (description_hash) and proceed to pay the invoice if it matches the expected amount and hash.

LN WALLET Verifies that h tag in provided invoice is a hash of metadata string converted to byte array in UTF-8 encoding.

maybe I am confusing something, but if I understand it correctly then a separate service would not work because the payment request must have the correct data (description_hash) otherwise it would be rejected from the LNURL pay wallet.

BUT this discussion is also too specific to LNURL.
I still do not understand why LndHub would not allow the API user to set that data and send it to the underlaying LND.

The description_hash is not really readable, that is correct... but then that is up to whoever uses the LndHub API., isn't it?
BlueWallet created invoices will only set a memo.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

metadata: in tag: "payRequest", can be anything ? as long as lnurl can later verify it itself

make invoice with a hash in metadata: --later verify the metadata: is payed

LndHub has in Redis the payment_hash convenient already from when made invoice and a lnurl-pay implementation can verify "if and when" invoice payed.

this way memo intact at same time lnurl satisfied.

description_hash in lnurl spec need not be same as invoice description_hash

@bumi
Copy link
Author

bumi commented Oct 27, 2021

not sure if I understand what you mean. sorry.

description_hash in lnurl spec need not be same as invoice description_hash

where do you have this from?
the LNURLpay spec describes that the metadata hash needs to be in the description_hash of the invoice, doesn't it?

LndHub has in Redis the payment_hash convenient already from when made invoice and a lnurl-pay implementation can verify "if and when" invoice payed.

we are talking about end-user wallets here. End-user wallets check the payment request.
also lnurl pay does not really check if the invoice is paid, it mainly describes a way how the wallet cat retrieve a payment request.

But back to the question: why would you not want to allow the users of the LndHub API to pass additional params to LND?

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

it is possible use a NEW endpoint in LndHub either running in same memarea or as a microservice in parralell with LndHub where service runs with separate :port, using a lnurlp-router for ex. `/YourQRCodeUrl/enpoint/doWhaterverYouLike.

LndHub is frozen other then acute bugs is my understanding, that do not hinder Us from extending it.

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

a correct service will sometimes want to know if invoice is paid and can do so with metadata: , and thereafter deliver what is paid for ...

this metadata:need not be the description_hash in invoice but can be anything_hash as long as the lnurl service implementation issuing the invoice can associate it.

@bumi
Copy link
Author

bumi commented Oct 27, 2021

this metadata:need not be the description_hash in invoice but can be anything_hash as long as the lnurl service implementation issuing the invoice can associate it.

afaik for LNURLpay this is wrong. the spec clearly says it must be the hash of the metadata string.

A new "microservice in parralell" will also not work because for LNURLpay it must be in the payment request.

LndHub is frozen

does that mean LndHub is not actively improved right now?

I've experimented with this: bumi@ac2061c

@xraid
Copy link
Contributor

xraid commented Oct 27, 2021

it is the same payment request , all of any extension service is using a LndHub account via "BlueWallet Lightning Protocol"

@bumi
Copy link
Author

bumi commented Oct 27, 2021

it is the same payment request , all of any extension service is using a LndHub account via "BlueWallet Lightning Protocol"

a payment request is specific for one payment and not all the "same payment request"

@kristapsk
Copy link

This seems to be a blocker for SatSale to implement lightning address support for LndHub backend.

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

No branches or pull requests

3 participants