ACINQ / eclair

A scala implementation of the Lightning Network.
Apache License 2.0
1.24k stars 266 forks source link

RPC: parse BOLT12 invoices and offers #2843

Closed rorp closed 1 month ago

rorp commented 7 months ago

This PR adds the ability to parse BOLT12 invoices to parseinvoice RPC call, and introduces parseoffer RPC call, that parses BOLT12 offers.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.00%. Comparing base (c8184b3) to head (6d01eee).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2843 +/- ## ========================================== + Coverage 85.96% 86.00% +0.03% ========================================== Files 219 219 Lines 18441 18442 +1 Branches 762 731 -31 ========================================== + Hits 15853 15861 +8 + Misses 2588 2581 -7 ``` | [Files](https://app.codecov.io/gh/ACINQ/eclair/pull/2843?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ACINQ) | Coverage Δ | | |---|---|---| | [...n/scala/fr/acinq/eclair/json/JsonSerializers.scala](https://app.codecov.io/gh/ACINQ/eclair/pull/2843?src=pr&el=tree&filepath=eclair-core%2Fsrc%2Fmain%2Fscala%2Ffr%2Facinq%2Feclair%2Fjson%2FJsonSerializers.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ACINQ#diff-ZWNsYWlyLWNvcmUvc3JjL21haW4vc2NhbGEvZnIvYWNpbnEvZWNsYWlyL2pzb24vSnNvblNlcmlhbGl6ZXJzLnNjYWxh) | `96.42% <ø> (ø)` | | | [...la/fr/acinq/eclair/payment/send/OfferPayment.scala](https://app.codecov.io/gh/ACINQ/eclair/pull/2843?src=pr&el=tree&filepath=eclair-core%2Fsrc%2Fmain%2Fscala%2Ffr%2Facinq%2Feclair%2Fpayment%2Fsend%2FOfferPayment.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ACINQ#diff-ZWNsYWlyLWNvcmUvc3JjL21haW4vc2NhbGEvZnIvYWNpbnEvZWNsYWlyL3BheW1lbnQvc2VuZC9PZmZlclBheW1lbnQuc2NhbGE=) | `72.91% <100.00%> (+0.57%)` | :arrow_up: | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/ACINQ/eclair/pull/2843/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ACINQ)
rorp commented 7 months ago

You seem to be doing more than just adding a parseoffer API in this PR, you're also adding support for Bolt 12 invoices which is arguably not a good idea...

Not at all. Only parseinvoice was changed to support Bolt 12 invoices. The changes in other RPCs were made just to prevent MatchError exceptions and avoid compiler errors.

t-bast commented 7 months ago

Only parseinvoice was changed to support Bolt 12 invoices.

This is what threw me off, I don't think we should do this. But I may be wrong, @thomash-acinq do you think this would be useful?

thomash-acinq commented 7 months ago

If there was an explicit decision to hide Bolt 12 invoices from the user (I didn't realize it was removed from the spec), then we shouldn't expose them.

If it's only for debugging purposes, what I usually do is run this test:

test("show invoice"){
  val encodedInvoice = "lni1qqgds4gw..."
  val invoice = Bolt12Invoice.fromString(encodedInvoice).get
  invoice.records.records.foreach(println)
}
t-bast commented 1 month ago

Shelving this for now, as Bolt 12 invoices aren't supposed to be exposed yet. We can reopen this in the future when concrete use-cases need it.