lndk-org / lndk

MIT License
84 stars 22 forks source link

Feature: use bech32-encoding everywhere instead of hex-encoding some places. #190

Open AndySchroder opened 4 weeks ago

AndySchroder commented 4 weeks ago

decode-invoice Decodes a bech32-encoded invoice string into a BOLT 12 invoice

get-invoice fetches a BOLT 12 invoice and returns as a hex-encoded string.

pay-invoice pays a hex-encoded BOLT12 invoice

Would be nice if all commands used a bech32-encoded invoice string instead of hex-encoded in some places. This is the behavior of CLN (https://docs.corelightning.org/reference/lightning-fetchinvoice , https://docs.corelightning.org/reference/lightning-pay).

supertestnet commented 3 weeks ago

I too want get-invoice to return a bech32-encoded bolt12 invoice

In the meantime, does anyone know a workaround to convert a hex-encoded invoice to a bech32-encoded invoice? I tried to just modify the encoding and prefix "LNI" to it, but then I passed that through a third-party bolt12 parser to see if I did it right and I got an "invalid padding" error.

orbitalturtle commented 3 weeks ago

Sure thing, I think we can change this to bech32 to make the API similar to CLN etc. @mrfelton I can't remember the reason for using hex, would this change break anything on Strike's end or would it be an easy swap?

In the meantime, does anyone know a workaround to convert a hex-encoded invoice to a bech32-encoded invoice? I tried to just modify the encoding and prefix "LNI" to it, but then I passed that through a third-party bolt12 parser to see if I did it right and I got an "invalid padding" error.

This is a common misconception I think - there is actually no lni prefix for invoices. In bolt 12 only lnr for the invoice requests and lno are defined, IIRC directly this is because the invoice is supposed to be hidden from the end user? Because of that, I'd be curious if the parser you're using parses bolt 12 invoices at all - but if it does, does it work with the lni removed?

Sounds like this isn't for your particular use case, but in a lot of cases just using pay-offer (which bundles get-invoice and pay-invoice into one) and not touching the invoice at all is the way to go.

orbitalturtle commented 3 weeks ago

Oh though I just remembered something @supertestnet, if your goal is to inspect the invoice, the decode-invoice cli command does that. I think if you convert the hex to bech32 w/o the lni prefix it should work.

AndySchroder commented 3 weeks ago

This is a common misconception I think - there is actually no lni prefix for invoices. In bolt 12 only lnr for the invoice requests and lno are defined, IIRC directly this is because the invoice is supposed to be hidden from the end user? Because of that, I'd be curious if the parser you're using parses bolt 12 invoices at all - but if it does, does it work with the lni removed?

Yes, the prefix lni is not defined in the spec, but I think it should be. I'd like to use the lni prefix for embedding a serialized refund invoice inside an invoice_request (see discussion here: https://delvingbitcoin.org/t/expanding-on-bolt12/1167#p-3280-automatic-refunds-3 ).

mrfelton commented 3 weeks ago

@mrfelton I can't remember the reason for using hex,

Primarily because it's not defined in the spec, and the recommendation at the time was encode as hex.

I've seen quite a few discussions about this, however. The spec authors argue that there is no need for the bech32 encoding for Invoices and no need for a human readable prefix because Invoices are not intended to be passed around between people.

In our own use case, invoices are passed around, although not between humans. They are stored in databases and passed between systems. hex encoding does work fine for that.

would this change break anything on Strike's end or would it be an easy swap?

Switching to a different encoding format would be a breaking change. I'd suggest adding a switch to allow the encoding scheme to be specified if you want to go in that direction. But I'd be weary of introducing a new human readable prefix that is not part of the spec. If there is broad consensus however that bech32 encoded invoices with a human readable prefix was a missing part of the spec then maybe work towards getting it added to the spec, or better understanding the reasons why it was not included.

AndySchroder commented 3 weeks ago

Why does decode-invoice use bech32-encoded invoice string then? Seems inconsistent. Are you arguing that should actually accept hex instead?

AndySchroder commented 3 weeks ago

Here is some more discussion of exposing the BOLT12 invoice: https://delvingbitcoin.org/t/blip-bolt-11-invoice-blinded-path-tagged-field/991/2 . We have some benefits that the BOLT12 invoice supports blinded paths and can be expanded to include more generic fields than a BOLT11 invoice. Seems like there are a lot of scenarios where skipping the offer/invoice_request steps could improve efficiency/speed and standardizing on BOLT11 instead of BOLT12 could be good.

AndySchroder commented 3 weeks ago

Also, in https://github.com/lndk-org/lndk/issues/170#issuecomment-2381058422 and https://github.com/lndk-org/lndk/issues/181 I requested to return the invoice back over the RPC as an option instead of onion messaging. That's another use case for passing invoices around and I think it would make sense to use bech32 encoding and not hex.

mrfelton commented 3 weeks ago

Why does decode-invoice use bech32-encoded invoice string then?

It doesn't. It accepts a hex encoded invoice. https://github.com/lndk-org/lndk/blob/925ce05096001ecb215c9419862c814de55fa066/src/cli.rs#L153-L169 There may be a mistake in the docs.

AndySchroder commented 3 weeks ago

There may be a mistake in the docs.

Okay, it seems that lndK is a bit more consistent then. My original issue attempted (but probably didn't say it clearly enough) to address (1) the inconsistency and (2) that I think people would prefer a bech32-encoded invoice prefixed with lni. It seems like (1) may not be up for much debate if the only inconsistency is a documentation error, but (2) may be still worth debating.