BlueWallet / LndHub

Wrapper for Lightning Network Daemon. It provides separate accounts for end-users
http://LndHub.io
MIT License
752 stars 185 forks source link

Allow passing of `description_hash` to the add invoice call #317

Open bumi opened 2 years ago

bumi commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

please expand

bumi commented 2 years ago

(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 commented 2 years ago

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 commented 2 years ago

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

bumi commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/bumi/LndHub/commit/ac2061c6edad25ab87a022ca6ec147e28f97ed8b

xraid commented 2 years ago

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

bumi commented 2 years ago

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 commented 1 year ago

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