VerifiedInvoiceRequest<S: SigningPubkeyStrategy> Review Branch #4
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.
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 `VerifiedInvoiceRequestEnum` 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.
WalkthroughThis set of changes refactors and extends the BOLT 12 invoice and refund handling across the Lightning Network codebase. It introduces a generic strategy for signing keys in verified invoice requests, adds a new enum to distinguish between derived and explicit signing key types, and updates invoice creation, processing, and test logic to use these new abstractions. The control flow for handling invoice requests is modularized, and error handling is improved, with explicit handling of different verification outcomes throughout the code and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChannelManager
participant OffersFlow
participant InvoiceBuilder
User->>ChannelManager: Send InvoiceRequest
ChannelManager->>OffersFlow: Verify InvoiceRequest
OffersFlow-->>ChannelManager: VerifiedInvoiceRequestEnum (WithKeys/WithoutKeys)
ChannelManager->>OffersFlow: Create InvoiceBuilder (variant-specific)
OffersFlow->>InvoiceBuilder: Build Invoice (with payment info closure)
InvoiceBuilder-->>OffersFlow: Unsigned Invoice
OffersFlow-->>ChannelManager: (InvoiceBuilder, Context) or Error
ChannelManager->>User: Send Invoice or InvoiceError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lightning/src/offers/flow.rs (1)
991-1055: Consider reducing code duplication between the two invoice builder methods.While the implementation is correct, there's significant duplication between
create_invoice_builder_from_invoice_request_with_keysandcreate_invoice_builder_from_invoice_request_without_keys. The only differences are:
- The type parameter constraint
- The response method called (
respond_using_derived_keysvsrespond_with)Consider extracting the common logic into a shared helper method.
Here's a suggested refactor to reduce duplication:
fn create_invoice_builder_common<'a, ES: Deref, R: Deref, F>( &'a self, router: &R, entropy_source: ES, amount_msats: u64, offer_id: OfferId, invoice_request_fields: &InvoiceRequestFields, usable_channels: Vec<ChannelDetails>, get_payment_info: F, ) -> Result<(Vec<BlindedPaymentPath>, PaymentHash, MessageContext), Bolt12SemanticError> where ES::Target: EntropySource, R::Target: Router, F: Fn(u64, u32) -> Result<(PaymentHash, PaymentSecret), Bolt12SemanticError>, { let entropy = &*entropy_source; let relative_expiry = DEFAULT_RELATIVE_EXPIRY.as_secs() as u32; let (payment_hash, payment_secret) = get_payment_info(amount_msats, relative_expiry)?; let context = PaymentContext::Bolt12Offer(Bolt12OfferContext { offer_id, invoice_request: invoice_request_fields.clone(), }); let payment_paths = self .create_blinded_payment_paths( router, entropy, usable_channels, Some(amount_msats), payment_secret, context, relative_expiry, ) .map_err(|_| Bolt12SemanticError::MissingPaths)?; let message_context = MessageContext::Offers(OffersContext::InboundPayment { payment_hash }); Ok((payment_paths, payment_hash, message_context)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lightning/src/ln/channelmanager.rs(4 hunks)lightning/src/ln/offers_tests.rs(2 hunks)lightning/src/offers/flow.rs(8 hunks)lightning/src/offers/invoice.rs(3 hunks)lightning/src/offers/invoice_request.rs(10 hunks)lightning/src/offers/offer.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
lightning/src/offers/offer.rs (3)
lightning/src/offers/invoice_request.rs (1)
offer_id(638-643)lightning/src/offers/invoice.rs (1)
offer_id(982-984)lightning/src/offers/static_invoice.rs (1)
offer_id(415-417)
lightning/src/offers/invoice.rs (2)
lightning/src/offers/static_invoice.rs (2)
invoice(801-827)payment_paths(534-536)lightning/src/offers/test_utils.rs (3)
payment_paths(75-110)payment_hash(112-114)now(116-120)
lightning/src/ln/channelmanager.rs (3)
lightning/src/offers/refund.rs (8)
refund(1488-1488)refund(1510-1510)refund(1532-1532)refund(1567-1567)refund(1621-1621)refund(1641-1641)amount_msats(533-535)amount_msats(750-752)lightning/src/offers/invoice_request.rs (2)
amount_msats(1121-1132)amount_msats(1175-1177)lightning/src/offers/invoice.rs (2)
amount_msats(1255-1257)relative_expiry(1238-1240)
lightning/src/offers/flow.rs (3)
lightning/src/offers/invoice_request.rs (4)
sign(473-475)amount_msats(1121-1132)amount_msats(1175-1177)from(428-437)lightning/src/offers/invoice.rs (7)
sign(619-621)payment_hash(1251-1253)amount_msats(1255-1257)relative_expiry(1238-1240)payment_paths(1230-1232)from(560-568)from(575-583)lightning/src/sign/mod.rs (1)
from(1084-1086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: semver-checks
- GitHub Check: linting
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: incremental-mutants
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build (windows-latest, beta)
- GitHub Check: build (macos-latest, beta)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
🔇 Additional comments (27)
lightning/src/offers/offer.rs (2)
1485-1485: Accessor usage aligns with new encapsulationReplacing direct field access with the
offer_id()accessor keeps the test in sync with the refactor that hides the field behind the enum. Looks good.
1566-1566: Consistent accessor adoptionSame update here ensures consistency across the test suite after the
VerifiedInvoiceRequestEnumchange. No issues spotted.lightning/src/ln/offers_tests.rs (2)
60-60: LGTM: Import addition aligns with refactoring.The import of
VerifiedInvoiceRequestEnumis necessary to support the updated verification logic in the test.
2282-2294: LGTM: Test correctly adapted to new verification enum.The test has been properly updated to handle the new
VerifiedInvoiceRequestEnumthat distinguishes between invoice requests verified with derived keys vs explicit keys. The logic correctly:
- Matches on the enum variants to handle different verification outcomes
- Processes the
WithKeysvariant usingrespond_using_derived_keys_no_std- Panics appropriately for
WithoutKeyssince this test expects derived key verificationThis change aligns with the broader refactoring mentioned in the PR summary while maintaining the test's original intent of verifying payment failure with unknown required features.
lightning/src/offers/invoice.rs (3)
1822-1822: LGTM: Clean import additionThe import of
VerifiedInvoiceRequestEnumis properly added and necessary for the updated test cases that follow.
1239-1258: LGTM: Proper pattern matching implementationThe explicit pattern matching on
VerifiedInvoiceRequestEnumvariants correctly handles the refactored verification flow:
WithKeysvariant proceeds with derived key response as expectedWithoutKeysvariant appropriately panics since this test expects keys- Error handling for invoice building is properly implemented
This change aligns well with the broader refactor to distinguish between requests with derived signing keys vs. those requiring explicit keys.
1277-1285: LGTM: Correct handling of opposite test scenarioThe pattern matching correctly handles the complementary test case:
WithKeysvariant appropriately panics since this test expects no keysWithoutKeysvariant continues silently as expected- The logic properly validates the inverse scenario from the previous test
This ensures comprehensive coverage of both verification outcomes in the new enum-based flow.
lightning/src/ln/channelmanager.rs (4)
91-99: Import changes look goodThe new imports for
UnsignedBolt12InvoiceandVerifiedInvoiceRequestEnumare properly added and align with the refactoring described in the PR objectives.
7197-7202: Proper use of newoffer_id()accessor methodThe change correctly uses the new
offer_id()accessor method on the verified invoice request, which aligns with the refactoring pattern introduced across the codebase.
11997-12018: Well-structured refactoring with improved payment handlingThe closure-based approach for inbound payment creation is cleaner and more flexible. Good addition of
.allow_mpp()to enable multi-path payments, which improves payment reliability.
14089-14168: Comprehensive handling of verified invoice request variantsThe implementation correctly handles both
WithKeysandWithoutKeysvariants with appropriate building and signing strategies. The error handling is consistent and properly returnsInvoiceErrormessages.Note the conditional compilation at line 14141-14142:
#[cfg(c_bindings)] let mut invoice = invoice;Please verify that this mutation pattern for C bindings is intentional and documented, as it creates a mutable binding only when compiling for C bindings.
lightning/src/offers/invoice_request.rs (10)
74-74: Import additions align with the new signing strategy pattern.The import of
DerivedSigningPubkey,ExplicitSigningPubkey, andSigningPubkeyStrategyis necessary for the refactoring that introduces generic signing key strategies.
596-617: Well-designed generic refactoring ofVerifiedInvoiceRequest.The introduction of the generic parameter
S: SigningPubkeyStrategyand the updated documentation clearly explain the different response methods based on the signing key type. This provides better type safety and clearer API semantics.
619-644: Clean enum design for handling different signing key scenarios.The
VerifiedInvoiceRequestEnumeffectively encapsulates the two possible states of a verified invoice request. The helper methodsinner()andoffer_id()provide ergonomic access to common fields across both variants.
776-777: Documentation correctly updated to reference the new enum.The documentation now appropriately references
VerifiedInvoiceRequestEnummethods, reflecting the new API design.
832-856: Verification methods correctly adapted to return the new enum.The
verify_using_metadatamethod now properly returnsVerifiedInvoiceRequestEnumwith the appropriate variant based on whether signing keys could be derived. The pattern matching logic is clear and correct.
875-901: Consistent implementation of recipient data verification.The
verify_using_recipient_datamethod follows the same pattern asverify_using_metadata, maintaining consistency in the API design.
1016-1045: Well-designed macro for unified field access.The
fields_accessormacro effectively reduces code duplication by providing a consistent way to extractInvoiceRequestFieldsacross different types. The payer note truncation is handled correctly using thestring_truncate_safefunction.
1047-1089: Consistent implementations across signing key strategies.The implementations for
VerifiedInvoiceRequest<DerivedSigningPubkey>,VerifiedInvoiceRequest<ExplicitSigningPubkey>, andVerifiedInvoiceRequestEnumare well-structured and consistent. Each provides the appropriate response methods for its signing key strategy.
1003-1003: Correct access to derived signing keys.The code correctly accesses the inner
Keypairfrom theDerivedSigningPubkeytuple struct.
3083-3083: Test correctly updated to use the new API.The test now uses the
offer_id()method from the verified invoice request enum, validating the refactored code paths.lightning/src/offers/flow.rs (6)
39-44: LGTM! Import changes align with the new architecture.The addition of
DEFAULT_RELATIVE_EXPIRYandVerifiedInvoiceRequestEnumimports properly supports the refactored invoice request handling logic.
405-408: Good architectural improvement using the new enum.The change from a generic
VerifiedInvoiceRequesttoVerifiedInvoiceRequestEnumimproves type safety by explicitly distinguishing between invoice requests with derived vs explicit signing keys.
869-889: Excellent refactoring to use callback pattern for payment info retrieval.The change to use a callback
get_payment_infoinstead of direct parameters improves flexibility and allows callers to defer payment creation until the exact moment it's needed. The error propagation is properly handled.
925-989: Well-implemented method for handling invoice requests with derived keys.The implementation correctly:
- Extracts amount from the invoice request
- Uses the callback pattern consistently
- Creates appropriate payment context with offer_id and invoice_request fields
- Handles both std and no_std environments
- Returns both the builder and message context for flexible response handling
1081-1082: Documentation formatting improvement.Good practice using the full path for cross-references in documentation.
1127-1128: Consistent documentation formatting.Maintains consistency with the earlier documentation change by using the full path.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests