Introduce Stronger Typing for VerifiedInvoiceRequest and Refactor Invoice Building Flow
#3964
Conversation
|
👋 I see @valentinewallace was un-assigned. |
|
cc @jkczyz |
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3964 +/- ##
==========================================
- Coverage 88.85% 88.84% -0.01%
==========================================
Files 175 175
Lines 127710 127795 +85
Branches 127710 127795 +85
==========================================
+ Hits 113478 113542 +64
- Misses 11675 11686 +11
- Partials 2557 2567 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 6th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
In the upcoming commits, we will be phasing the current style of VerifiedInvoiceRequest, in favour of newer version. To keep the changes modular, and clean we rename the current VerifiedInvoiceRequest to VerifiedInvoiceRequestLegacy.
In the following commits we will introduce `fields` function for other types as well, so to keep code DRY we convert the function to a macro.
This commit reintroduces `VerifiedInvoiceRequest`, now parameterized by `SigningPubkeyStrategy`. The key motivation is to restrict which functions can be called on a `VerifiedInvoiceRequest` based on its strategy type. This enables compile-time guarantees — ensuring that an incorrect `InvoiceBuilder` cannot be constructed for a given request, and misuses are caught early.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
This commit replaces the legacy `VerifiedInvoiceRequestLegacy` with the new `InvoiceRequestVerifiedFromOffer` type in the codebase.
This change improves type safety and architectural clarity by introducing dedicated `InvoiceBuilder` methods tied to each variant of `VerifiedInvoiceRequestEnum`. With this change, users are now required to match on the enum variant before calling the corresponding builder method. This pushes the responsibility of selecting the correct builder to the user and ensures that invalid builder usage is caught at compile time, rather than relying on runtime checks. The signing logic has also been moved from the builder to the `ChannelManager`. This shift simplifies the builder's role and aligns it with the rest of the API, where builder methods return a configurable object that can be extended before signing. The result is a more consistent and predictable interface that separates concerns cleanly and makes future maintenance easier.
To ensure correct Bolt12 payment flow behavior, the `amount_msats` used for generating the `payment_hash`, `payment_secret`, and payment path must remain consistent. Previously, these steps could inadvertently diverge due to separate sources of `amount_msats`. This commit refactors the interface to use a `get_payment_info` closure, which captures the required variables and provides a single source of truth for both payment info (payment_hash, payment_secret) and path generation. This ensures consistency and eliminates subtle bugs that could arise from mismatched amounts across the flow.
|
🔔 9th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
I'm unfortunately not going to have time to review this in a timely manner, so unassigning myself and letting the bot take the wheel |
|
✅ Added second reviewer: @joostjager |
There was a problem hiding this comment.
After an initial pass, my feelings about this PR are mixed. Like probably every other rust dev, I also like strong types and compile time checks. But I am not sure if the old situation with the keys option was a big enough problem to justify the code changes proposed in this PR. Especially because the end result may not be considered fully clean either (panic match arms, code duplication).
Other considerations are dev/review resources available, risk that every change introduces and priority. Perhaps it would be good for this PR (and PRs in general) to add a few lines to the description elaborating on that. Could be downstream advantages for example that aren't visible in the PR itself.
| @@ -961,9 +961,43 @@ macro_rules! invoice_request_respond_with_derived_signing_pubkey_methods { ( | |||
| } | |||
| } } | |||
|
|
|||
| macro_rules! fields_accessor { | |||
There was a problem hiding this comment.
My impression is that we want to get rid of macros as much as possible, not introduce new ones?
| .respond_using_derived_keys_no_std(payment_paths, payment_hash, created_at).unwrap() | ||
| .features_unchecked(Bolt12InvoiceFeatures::unknown()) | ||
| .build_and_sign(&secp_ctx).unwrap(); | ||
| let verified_invoice_request = invoice_request |
There was a problem hiding this comment.
For review, I'd prefer all commits up to this one to be squashed. The pattern of 'rename - add new type - replace usages' makes it harder for me to see what the actual change is. This is what I am looking for in a diff:
| }, | ||
| }; | ||
| let payment_paths = self | ||
| .create_blinded_payment_paths( |
There was a problem hiding this comment.
Not ideal that code needs to be duplicated now.
| } | ||
| }, | ||
| InvoiceRequestVerifiedFromOffer::ExplicitKeys(_) => { | ||
| panic!("expected invoice request with keys"); |
There was a problem hiding this comment.
This match arm with a panic comes back a few times. It does feel a bit anti-pattern'ish. If we go for strong types to eliminate runtime checks, I wouldn't expect other runtime checks being added.
There was a problem hiding this comment.
This is really is acting more like a test assertion. The user would need to handle both case.
When reading an invoice request, the invoice signing mechanism is determined by parsing and verifying the underlying offer using its metadata (or more likely from the metadata included in the blinded path that the invoice request was received over). Note that this verification is specific to LDK's construction of offers, but it's not generalizable to offers constructed by other implementations, since they may use the metadata in their own way.
To make it more strongly typed, we'd need to pass in the ExpandedKey used to construct the underlying offer at parse time to determine its type. However, wherever you try to push the type checks, you end up with something that is / has an enum because you can't predict the resulting type before parsing it.
For instance, we can make it such that we have InvoiceRequest<S> that contains OfferContents<S>, but we still need a wrapper enum around InvoiceRequest. And you'd need to fail parsing if you can't authenticate it. We chose to have verification to be a separate step from parsing. That's cleaner and allows the offers module to be used independent of the rest of LDK, if desired.
There was a problem hiding this comment.
Sorry, I missed that this is indeed in tests. Yes, makes sense to have the panic there.
I still keep looking for where in this PR exactly advantage is taken of the strong typing. Is it that you could do something invalid with create_response_for_invoice_request ?
|
Final thought, not fully checked, is whether there is a simpler way to make the api safe by just moving the respond methods from |
Agreed that the PR description should include motivation for the change. Namely, use for custom handling of invoice requests / building custom invoices. |
Can I see this somewhere? So that it becomes more clear where the benefits are of the stronger typing. |
| )?; | ||
|
|
||
| let invoice = builder.allow_mpp().build_and_sign(secp_ctx)?; | ||
| let entropy = &*self.entropy_source; |
There was a problem hiding this comment.
Optional: there is potential to remove some rustfmt::skip in this PR (in a separate commit).
Previously,
VerifiedInvoiceRequestused an optional field to decide between callingusing_derived_keyorusing_explicit_keyinvoice builders. However, this approach relied on runtime checks, and didn't enforce the correct builder usage at compile time.To address this, this PR introduces a stronger type distinction by parameterizing
VerifiedInvoiceRequestwith aSigningPubkeyStrategy. This ensures that only the appropriate builder method (using_derived_keyorusing_explicit_key) is available for the corresponding variant, enforcing correctness at compile time.This change also refactors the downstream
invoice_builderinterface inOffersMessageFlowby taking the following steps:VerifiedInvoiceRequestvariant, ensuring that only the correct method can be called, and shifting the responsibility of matching the variant to the user at compile time.ChannelManager, aligning with the pattern used in other builders. This allows users to further customize theInvoiceBuilderbefore signing.(payment_hash, payment_secret)generation, consolidating theamount_msatsinput into a single source of truth. This ensures consistency between invoice creation and payment path generation.