RPC: parse BOLT12 invoices and offers #2843

rorp · 2024-03-28T16:39:51Z

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

codecov-commenter · 2024-03-29T09:20:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files

@@            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	Coverage Δ
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala	`96.42% <ø> (ø)`
...la/fr/acinq/eclair/payment/send/OfferPayment.scala	`72.91% <100.00%> (+0.57%)`	⬆️

... and 5 files with indirect coverage changes

t-bast

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...

t-bast · 2024-03-29T14:01:02Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/send/OfferPayment.scala

@@ -138,6 +138,7 @@ private class OfferPayment(replyTo: ActorRef,
            Behaviors.stopped
          case None =>
            context.spawnAnonymous(CompactBlindedPathsResolver(router)) ! Resolve(context.messageAdapter[Seq[ResolvedPath]](WrappedResolvedPaths), payload.invoice.blindedPaths)
+            context.log.info(s"BOLT12 invoice: ${payload.invoice}")


nit: remove?

Not really. It's really useful for debugging. How else could we know what kind of invoices our counterparties send to us?

I don't understand...? Why is it useful to log the Bolt 12 invoice we receive? If the payment completes it will be stored in our payments DB. If the payment fails, do you really need the invoice to investigate why it failed? It will generally be easy to figure out what fails without it?

eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala

t-bast · 2024-03-29T14:05:30Z

eclair-node/src/main/scala/fr/acinq/eclair/api/serde/FormParamExtractors.scala

+  val invoiceUnmarshaller: Unmarshaller[String, Invoice] = Unmarshaller.strict { str =>
+    Bolt11Invoice.fromString(str).orElse(Bolt12Invoice.fromString(str)).get
+  }


Why are you introducing this? It creates unnecessary complexity in API handlers since we don't support directly paying a Bolt 12 invoice (we should always start with an offer and go through the invoice_request flow)?

Please advise how it is supposed to be done in the right way? All I need is to have an ability to parse Bolt 12 invoices and offers.

To parse an invoice when you don't know which kind of invoice to expect, you should use Invoice.fromString.

What I meant is that Bolt 12 invoices shouldn't be exposed, see #2843 (comment)

There was a decision to not even specify a string encoding for them, we have one in eclair for consistency (and because we added it before the spec removed it), but it shouldn't be relied on.

t-bast · 2024-03-29T14:06:57Z

eclair-node/src/main/scala/fr/acinq/eclair/api/directives/ExtraDirectives.scala

@@ -47,7 +47,7 @@ trait ExtraDirectives extends Directives {
  val outPointsFormParam: NameUnmarshallerReceptacle[List[OutPoint]] = "outpoints".as[List[OutPoint]](outPointListUnmarshaller)
  val paymentHashFormParam: NameUnmarshallerReceptacle[ByteVector32] = "paymentHash".as[ByteVector32](bytes32Unmarshaller)
  val amountMsatFormParam: NameReceptacle[MilliSatoshi] = "amountMsat".as[MilliSatoshi]
-  val invoiceFormParam: NameReceptacle[Bolt11Invoice] = "invoice".as[Bolt11Invoice]
+  val invoiceFormParam: NameUnmarshallerReceptacle[Invoice] = "invoice".as[Invoice](invoiceUnmarshaller)


We should keep this for Bolt 11 only, Bolt 12 invoices should be obtained with the invoice_request flow and never accepted as a parameter?

I don't really understand the question, sorry.

It's actually not a question but a statement. I guess t-bast added a question mark to ask if you object to this statement.
Your reason to use it is parseinvoice and I agree that it would be nice to have, but it would be cleaner to keep this only for Bolt11Invoice (renaming it to bolt11invoiceFormParam) and create a new invoiceFormParam that you use only for parseinvoice.

From the spec point of view, Bolt 12 invoices should not be exposed to end users, only the offer should be exposed. You shouldn't be able to explicitly pay a Bolt 12 invoice, APIs should only allow you to pay a Bolt 12 offer.

So I'm not sure why you'd want to be able to parse them, since they're meant to be internal objects that aren't exposed to end users. On top of that, they're just a TLV stream, so it won't look nice in JSON at all...

rorp · 2024-03-29T14:47:03Z

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 · 2024-03-29T16:19:35Z

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 · 2024-03-29T16:57:58Z

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)
}

rorp added 2 commits March 28, 2024 17:35

RPC: parse BOLT12 invoices and offers

eb0fc25

Update the unit tests

6d01eee

t-bast reviewed Mar 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: parse BOLT12 invoices and offers #2843

RPC: parse BOLT12 invoices and offers #2843

rorp commented Mar 28, 2024 •

edited

codecov-commenter commented Mar 29, 2024

t-bast left a comment

t-bast Mar 29, 2024

rorp Mar 29, 2024

t-bast Mar 29, 2024

t-bast Mar 29, 2024

rorp Mar 29, 2024

thomash-acinq Mar 29, 2024

t-bast Mar 29, 2024

t-bast Mar 29, 2024

rorp Mar 29, 2024

thomash-acinq Mar 29, 2024

t-bast Mar 29, 2024

rorp commented Mar 29, 2024 •

edited

t-bast commented Mar 29, 2024

thomash-acinq commented Mar 29, 2024

RPC: parse BOLT12 invoices and offers #2843

Are you sure you want to change the base?

RPC: parse BOLT12 invoices and offers #2843

Conversation

rorp commented Mar 28, 2024 • edited

codecov-commenter commented Mar 29, 2024

Codecov Report

t-bast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rorp commented Mar 29, 2024 • edited

t-bast commented Mar 29, 2024

thomash-acinq commented Mar 29, 2024

rorp commented Mar 28, 2024 •

edited

rorp commented Mar 29, 2024 •

edited