Introduce Stronger Typing for VerifiedInvoiceRequest and Refactor Invoice Building Flow
#3964
Conversation
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.
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
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. |
|
|
||
| /// An enum representing a verified invoice request, which can either have derived signing keys or | ||
| /// require explicit signing keys. | ||
| pub enum VerifiedInvoiceRequestEnum { |
There was a problem hiding this comment.
Maybe we name this InvoiceRequestVerifiedFromOffer. Would like to avoid the Enum suffix.
There was a problem hiding this comment.
Since this enum contains the verified invoice request used to build the corresponding invoice, and distinguishes between the signing strategies used to sign that invoice, I felt that the name InvoiceSigningInfo could reflect its role clearly.
I’ve introduced this name in pr3964.02 — would love to hear your thoughts! Thanks a lot!
|
🔔 5th Reminder Hey @valentinewallace! This PR has been waiting for your review. |
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.
This commit replaces the legacy `VerifiedInvoiceRequestLegacy` with the new `InvoiceSigningInfo` 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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3964 +/- ##
========================================
Coverage 88.92% 88.92%
========================================
Files 173 174 +1
Lines 123794 124624 +830
Branches 123794 124624 +830
========================================
+ Hits 110081 110820 +739
- Misses 11253 11302 +49
- Partials 2460 2502 +42
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. |
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.