search

breez / breez-sdk-greenlight

MIT License
244 stars 43 forks source link

Expand `InputType` with new variant: Bolt12 offer #1121

Closed ok300 closed 3 days ago

ok300 commented 1 week ago

This PR extends the input parsed in sdk-common to recognize Bolt12 offers.

ok300 commented 1 week ago

@hydra-yse your older PR #598 parses the offer to more detailed fields:

https://github.com/breez/breez-sdk-greenlight/blob/ad384b106bfcee3e225625b879b153f398b6ee7f/libs/sdk-bindings/src/breez_sdk.udl#L735-L755

As that PR is still in review, I'm not sure if that structure is final (LNOffer, InputType::Bolt12Offer(LNOffer offer)).

So would you say we need those fields as defined there? If so, I can integrate them in this PR.

dangeross commented 1 week ago

So would you say we need those fields as defined there? If so, I can integrate them in this PR

I think they are needed, we need to parse the offer for the user. I also think our PR struct is out of date and I would suggest bumping the lightning/lightning-invoice dependencies. The current version doesn't parse offers according to the latest spec (e.g. the description field is now optional), which I think is not compatible with Phoenix

JssDWt commented 1 week ago

As that PR is still in review, I'm not sure if that structure is final (LNOffer, InputType::Bolt12Offer(LNOffer offer)).

So would you say we need those fields as defined there? If so, I can integrate them in this PR.

Also keep in mind there are invoice requests and invoices in bolt12, next to offers. We don't have to support them, but let's make the interface in such a way that we don't have to break anything to support them in the future.

ok300 commented 1 week ago

I tried to bump lightning and lightning-invoice as suggested above. Indeed we would need them to support optional descriptions, like in the Bolt12 invoices from Boltz.

However due to inter-dependencies between crates, this means also updating bech32, bip21, bitcoin crates. This results in two problems:

(This is reflected in the last commit f0d19cc3917ce38fe4020c96a99865d5dcffda19 . Almost half the errors are fixed, but I stopped when realizing the node_api issue.)

The only way to avoid the above is to store the unparsed Bolt12 option as String in the InputParser::Bolt12 variant.

Another option is to ask GL to update their lightning, lightning-invoice and bitcoin crates to the newer ones we would use, but I don't know how feasible that is.

Am I missing something? Do you see a simpler way to bump lightning and lightning-invoice?

ok300 commented 1 week ago

As discussed, this is adding a simple Bolt12Offer { offer: String } variant, until gl-client supports the newer lightning and lightning-invoice crates.

In addition, Liquid SDK will attempt to parse the offer locally.

ok300 commented 5 days ago

IMO it's best to add them when they will also be tested and used.