No Blinded Path Offer (Review Branch) #1
Conversation
Previously, the `compact_paths` flag was only used to determine whether to use a compact introduction node when creating compact blinded paths. With the upcoming change to accept `MessageForwardNode` in `create_blinded_paths`, there's a risk of SCIDs being passed (and used) even when the user intends to create a full-length blinded path. This patch updates the logic in `create_blinded_paths_from_iter` to ignore SCIDs unless `compact_paths` is explicitly true—preserving correct behavior for full-length blinded paths.
To prepare for supporting both standard and compact blinded paths, this commit updates the `create_blinded_paths` function to take a `Vec<MessageForwardNode>` as input. This change ensures the function has all the information it needs to handle both types of blinded path creation. This refactor that sets the stage for upcoming enhancements.
To make the purpose of each `MessageRouter` implementation unambiguous, this commit sets a direction where the type of `MessageRouter` used deterministically defines the kind of blinded paths created. As a step toward this goal, two new default routers are introduced: - `NodeIdMessageRouter` – creates full-length blinded paths using the peer's node ID. - `NullMessageRouter` – intentionally creates no blinded paths.
To allow choosing different message router types for testing nodes, convert `TestMessageRouter` to an enum with variants `DefaultMessageRouter` and `NodeIdMessageRouter`. This provides better flexibility when testing various scenarios.
WalkthroughThe changes unify and refactor blinded path creation and message routing across the codebase. All relevant methods now use Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChannelManager
participant OffersMessageFlow
participant MessageRouter
Caller->>ChannelManager: create_offer_builder_using_router(router)
ChannelManager->>OffersMessageFlow: create_offer_builder_using_router(router)
OffersMessageFlow->>MessageRouter: create_blinded_paths(recipient, context, peers, secp_ctx)
MessageRouter-->>OffersMessageFlow: Vec<BlindedMessagePath>
OffersMessageFlow-->>ChannelManager: OfferBuilder
ChannelManager-->>Caller: OfferBuilder
sequenceDiagram
participant Test
participant TestUtils
participant TestMessageRouter
Test->>TestUtils: create_node_cfgs_with_node_id_message_router()
TestUtils->>TestMessageRouter: new_node_id_router(...)
TestMessageRouter-->>TestUtils: NodeId variant
TestUtils-->>Test: Vec<NodeCfg>
Poem
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the onion‐message routing and offer/refund builder APIs to always use full-length blinded paths, removes the concept of compact (short-lived) paths, and updates the test and functional infrastructure to support both default and node-id–based routers.
- Replace
TestMessageRouterstruct with an enum exposing both default and node-id router variants. - Remove
create_compact_blinded_pathsin favor of a singlecreate_blinded_pathsmethod and unifyMessageRouterimplementations (DefaultMessageRouter,NodeIdMessageRouter,NullMessageRouter). - Drop the
absolute_expiryparameter fromcreate_offer_builderand introduce internal helper methods for offer/refund builder logic. - Update tests, functional utilities, and
ChannelManagermacros to use the new router constructors and builder signatures.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lightning/src/util/test_utils.rs | Convert TestMessageRouter to an enum with Default/NodeId variants; add new_default and new_node_id_router. |
| lightning/src/onion_message/messenger.rs | Remove compact-path default impl, add MessageForwardNode everywhere, implement NodeIdMessageRouter and NullMessageRouter. |
| lightning/src/offers/flow.rs | Remove expiry-based path selection, add create_offer_builder_intern/create_refund_builder_intern, simplify public builder methods. |
| lightning/src/ln/offers_tests.rs | Update tests to call create_offer_builder()/create_refund_builder_using_router() and adapt path assertions via helper. |
| lightning/src/ln/functional_test_utils.rs | Introduce create_node_cfgs_internal, new helpers create_node_cfgs, create_node_cfgs_with_node_id_message_router. |
| lightning/src/ln/channelmanager.rs | Update create_offer_builder macro to drop the absolute_expiry argument; add router-based builder macros. |
Comments suppressed due to low confidence (3)
lightning/src/ln/channelmanager.rs:10870
- Removing the
absolute_expiryparameter fromcreate_offer_builderis a breaking change to the public API. Consider deprecating the old signature first or providing a clear migration path to avoid unexpected breakage for downstream consumers.
pub fn create_offer_builder(&$self) -> Result<$builder, Bolt12SemanticError> {
lightning/src/util/test_utils.rs:314
- [nitpick] Having an enum variant named
Defaultcan be confusing due to its overlap with theDefaulttrait. Consider renaming the variants to more descriptive names (e.g.,FullPathandNodeIdBased) for clarity.
pub enum TestMessageRouter<'a> {
lightning/src/offers/flow.rs:413
- [nitpick] The
_internsuffix for this helper method is unconventional and may not clearly communicate its purpose. Consider renaming it to_internalor another more descriptive name to adhere to common naming conventions.
fn create_offer_builder_intern<ES: Deref, PF>(
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lightning/src/offers/flow.rs (1)
443-481: Breaking change:absolute_expiryparameter removedThe removal of the
absolute_expiryparameter fromcreate_offer_builderis a breaking API change. The documentation still mentions "The offer will expire atabsolute_expiryifSome, or will not expire ifNone" but this parameter no longer exists.Update the documentation to reflect the current behavior:
- /// The offer will expire at `absolute_expiry` if `Some`, - /// or will not expire if `None`. + /// The created offer will not have an explicit expiration time. + /// Use the builder's methods to set expiration if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
fuzz/src/chanmon_consistency.rs(2 hunks)fuzz/src/full_stack.rs(2 hunks)fuzz/src/onion_message.rs(2 hunks)lightning-dns-resolver/src/lib.rs(4 hunks)lightning/src/ln/channelmanager.rs(5 hunks)lightning/src/ln/functional_test_utils.rs(4 hunks)lightning/src/ln/functional_tests.rs(1 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(47 hunks)lightning/src/offers/flow.rs(6 hunks)lightning/src/onion_message/messenger.rs(6 hunks)lightning/src/util/test_utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: semver-checks
- GitHub Check: fuzz
- GitHub Check: check_commits
- GitHub Check: benchmark
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (ubuntu-latest, 1.63.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (ubuntu-latest, beta)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, beta)
- GitHub Check: incremental-mutants
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: coverage
- GitHub Check: semver-checks
🔇 Additional comments (39)
lightning/src/ln/max_payment_path_len_tests.rs (1)
520-520: LGTM! Clean API update aligns with method signature simplification.The removal of the
Noneparameter fromcreate_offer_builder()correctly reflects the broader refactoring to simplify the method signature by removing theabsolute_expiryparameter. The test functionality remains unchanged.lightning/src/ln/functional_tests.rs (1)
5177-5178: Use updatednew_defaultconstructor forTestMessageRouter
This change correctly aligns with the new enum‐based API forTestMessageRouter.To verify there are no remaining deprecated constructor calls, you can run:
rg -n "TestMessageRouter::new"fuzz/src/onion_message.rs (1)
9-9: LGTM! Consistent trait implementation update.The changes properly update the fuzz test implementation to match the new
MessageRoutertrait signature, replacingVec<PublicKey>withVec<MessageForwardNode>for richer peer representation in blinded path creation.Also applies to: 106-106
fuzz/src/full_stack.rs (1)
33-33: LGTM! Consistent trait implementation update.The changes correctly update the fuzz test's
MessageRouterimplementation to match the new trait signature, usingMessageForwardNodeinstead of rawPublicKeyfor enhanced peer representation in blinded path creation.Also applies to: 176-176
fuzz/src/chanmon_consistency.rs (1)
36-36: LGTM! Consistent refactor to use MessageForwardNode.The import addition and method signature update align perfectly with the broader refactor to standardize blinded path creation around
MessageForwardNode. Since this is a fuzz implementation that usesunreachable!(), the signature change is safe and maintains consistency with otherMessageRouterimplementations.Also applies to: 145-145
lightning-dns-resolver/src/lib.rs (3)
162-164: LGTM! Consistent with the MessageForwardNode refactor.The import addition and method signature update properly align with the broader refactor. Since
DirectlyConnectedRouterignores the_peersparameter anyway, this change is safe and maintains consistency with the updatedMessageRoutertrait.Also applies to: 235-235
459-459: LGTM! Updated to use the new test utility function.The change from
create_node_cfgstocreate_node_cfgs_with_node_id_message_routeraligns with the introduction of multipleTestMessageRoutervariants and ensures consistent test setup with the refactored message routing infrastructure.
485-485: LGTM! Simplified offer builder call.The removal of the
Noneparameter aligns with the refactoring ofcreate_offer_builderthat removed theabsolute_expiryparameter, making the API cleaner and more consistent.lightning/src/ln/channelmanager.rs (6)
2216-2216: Documentation correctly updated to reflect API change.The documentation example properly removes the
absolute_expiryparameter to match the simplifiedcreate_offer_buildersignature.
3024-3024: Improved documentation with valuable context.The updated documentation provides clear guidance on how BlindedMessagePath creation works and the privacy trade-offs, which will help users make informed decisions.
10870-10876: Breaking change:absolute_expiryparameter removed.The removal of the
absolute_expiryparameter fromcreate_offer_builderis a breaking API change. Ensure this is documented in release notes and migration guides.
10878-10912: Excellent addition of flexible router support.The new
create_offer_builder_using_routermethod provides valuable flexibility for custom blinded path creation. The implementation correctly:
- Accepts a generic MessageRouter via trait bounds
- Includes comprehensive documentation covering privacy, limitations, and error conditions
- Properly handles error propagation from the internal flow methods
10980-11048: Comprehensive refund builder with robust error handling.The new
create_refund_builder_using_routermethod provides excellent functionality:Strengths:
- Comprehensive documentation covering all aspects (payment, privacy, limitations, errors)
- Proper duplicate payment ID prevention with
Bolt12SemanticError::DuplicatePaymentId- Correct use of
PersistenceNotifierGuardfor state persistence- Well-structured parameter validation and error propagation
Implementation Details:
- Payment tracking setup is correctly implemented with expiration handling
- Generic MessageRouter support provides flexibility for custom routing
- Error handling follows established patterns in the codebase
17181-17181: Test utility correctly updated for new API.The update to use
TestMessageRouter::new_defaultproperly adapts the test code to the refactored TestMessageRouter API, maintaining test functionality.lightning/src/util/test_utils.rs (5)
37-37: LGTM!The import addition for
NodeIdMessageRouteris necessary for the new enum variant and follows proper import conventions.
314-330: Well-designed enum refactor.The conversion from struct to enum enables flexible router selection while maintaining type safety. The variant names clearly indicate their purpose, and the generic parameters are consistently applied.
333-344: Excellent constructor design.The constructor methods provide a clean API for instantiating different router variants. The naming is clear, parameters are consistent, and the implementation properly encapsulates router creation complexity.
350-354: Clean pattern matching dispatch.The
find_pathimplementation correctly uses pattern matching to delegate to the appropriate inner router. All enum variants are handled consistently.
360-368: Consistent dispatch implementation with updated signature.The
create_blinded_pathsmethod correctly uses pattern matching dispatch and the updated signature withVec<MessageForwardNode>aligns with the broader codebase refactoring for unified message routing.lightning/src/ln/offers_tests.rs (9)
63-63: LGTM: Import addition supports new test functionality.The
NullMessageRouterimport is needed for the new tests that create offers/refunds without blinded paths.
152-160: Excellent helper function for improving test accuracy.This helper function correctly combines two important checks:
- Verifies the introduction node matches the expected public key
- Ensures the path uses compact representation (
DirectedShortChannelId)This is a significant improvement over the previous direct equality assertions, as it validates both the node identity and the specific compact path format.
268-288: LGTM: Well-structured test for no-blinded-path offers.The test correctly verifies that:
- Offers can be created with
NullMessageRouter- The issuer signing pubkey is properly set when no blinded paths are used
- The paths collection is empty as expected
The test structure follows the established pattern and covers the essential assertions.
290-315: LGTM: Comprehensive test for no-blinded-path refunds.The test thoroughly validates refund creation without blinded paths:
- Uses
NullMessageRouterappropriately- Verifies all expected refund properties (amount, expiry, payer key)
- Confirms empty paths collection
- Follows consistent test patterns
359-359: LGTM: API simplification through parameter removal.The removal of parameters from
create_offer_builder()calls aligns with the refactoring mentioned in the AI summary where the expiry parameter was removed and offer builder creation was simplified.Also applies to: 375-375, 426-426, 452-452, 475-475, 485-485
466-466: Appropriate router configuration for long-lived offers/refunds.The change to
create_node_cfgs_with_node_id_message_routerfor tests involving long-lived offers and refunds ensures these tests use the correct message routing behavior consistent with the new router abstractions.Also applies to: 527-527
596-596: Excellent adoption of the improved helper function.The widespread replacement of direct path introduction node assertions with
check_compact_path_introduction_nodeimproves test accuracy by:
- Ensuring consistent validation across all tests
- Checking both node identity and compact path format
- Making the test intent clearer and more explicit
This change enhances the reliability of path validation throughout the test suite.
Also applies to: 626-626, 645-647, 705-705, 729-729, 762-762, 784-784, 796-796, 833-833, 852-852
1029-1029: Consistent test improvements with the helper function.These changes continue the pattern of replacing direct assertions with the more robust
check_compact_path_introduction_nodehelper, ensuring comprehensive validation of both introduction node identity and compact path representation across all relevant test scenarios.Also applies to: 1039-1039, 1094-1094, 1110-1110, 1119-1119, 1145-1145, 1173-1173, 1445-1445, 1454-1454, 1475-1475, 1505-1505, 1556-1556, 1607-1607, 1664-1664, 1698-1698
745-745: Consistent API usage throughout test suite.The systematic removal of parameters from
create_offer_builder()calls demonstrates good consistency in applying the API changes across the entire test suite.Also applies to: 756-756, 879-879, 920-920, 1003-1003, 1139-1139, 1215-1215, 1307-1307, 1437-1437, 1449-1449, 1549-1549, 1861-1861, 2040-2040, 2249-2249, 2328-2328, 2408-2408, 2745-2745, 2756-2756, 2861-2861, 2954-2954
lightning/src/ln/functional_test_utils.rs (3)
348-388: LGTM! Well-designed generic refactoring.The introduction of
create_node_cfgs_internalwith a generic message router constructor is an excellent refactoring approach. It eliminates code duplication while providing flexibility for different router types. The generic constraints and lifetime parameters are properly defined.
390-415: LGTM! Clean wrapper functions maintain backwards compatibility.The refactored wrapper functions (
create_node_cfgs,create_node_cfgs_with_persisters, and the newcreate_node_cfgs_with_node_id_message_router) are well-implemented. They maintain backwards compatibility while providing the flexibility to use different message router types as mentioned in the AI summary.
731-731: LGTM! Method call updated to match new TestMessageRouter API.The change from
TestMessageRouter::newtoTestMessageRouter::new_defaultcorrectly aligns with the TestMessageRouter transformation from a struct to an enum with multiple constructors.lightning/src/onion_message/messenger.rs (3)
530-536: Method visibility changed topub(crate)with newcompact_pathsparameterThe
create_blinded_paths_from_itermethod is now exposed aspub(crate)and includes a newcompact_pathsparameter. This allows bothDefaultMessageRouterandNodeIdMessageRouterto share the implementation while controlling whether short channel IDs are preserved.Good refactoring to avoid code duplication between the two router implementations.
674-731: Well-documented alternative router implementationThe
NodeIdMessageRouterprovides a clear alternative toDefaultMessageRouterby always creating full-length blinded paths using node IDs. The implementation correctly reuses the existing logic by callingcreate_blinded_paths_from_iterwithcompact_paths=false.The documentation clearly explains the privacy implications and differences from the default implementation.
1444-1447: Correct adaptation to new MessageForwardNode APIThe mapping of peers to
MessageForwardNodewithshort_channel_idset toNoneis appropriate since these are direct peer connections without specific channel associations.lightning/src/offers/flow.rs (5)
413-441: Well-designed internal helper using closure patternThe
create_offer_builder_internmethod effectively abstracts the common logic for offer builder creation while allowing flexibility through themake_pathsclosure. This enables both the default implementation and custom router implementations to share code.Good use of the closure pattern for dependency injection.
482-518: Excellent addition for custom routing flexibilityThe
create_offer_builder_using_routermethod provides valuable flexibility by allowing users to specify a customMessageRouter. The documentation clearly explains the use case and limitations.This is a good addition that maintains backward compatibility while enabling advanced use cases.
540-581: Consistent implementation pattern for refund builderThe
create_refund_builder_internfollows the same pattern as the offer builder, providing good consistency and code reuse. The method properly handles all the refund-specific requirements while abstracting the path creation logic.
637-702: Consistent API addition for refund builderThe
create_refund_builder_using_routermethod maintains API consistency with the offer builder methods, providing the same flexibility for custom routing implementations. The documentation is comprehensive and clearly explains the use case.
587-591: Documentation references potentially removed constantThe documentation mentions
MAX_SHORT_LIVED_RELATIVE_EXPIRYwhich appears to have been removed according to the AI summary. This reference should be updated or removed to avoid confusion.#!/bin/bash # Check if MAX_SHORT_LIVED_RELATIVE_EXPIRY is still defined rg "MAX_SHORT_LIVED_RELATIVE_EXPIRY" -A 2 -B 2
Reasoning: This change aligns `DefaultMessageRouter`'s default behavior with the most common and practical usage: generating compact blinded paths for BOLT12 offers. While configurability is important, most users won't read all documentation and will rely on defaults that "just work." In line with this PR's principle— "One `MessageRouter`, one type of `BlindedPath`"—the previous default would silently produce full blinded paths, even when a compact one would have been more appropriate. In typical setups (e.g., a user connected to a single LSP), the SCID alias remains stable for the channel's lifetime. Compact paths are not only sufficient in this case, but also result in smaller, more efficient offers. And if the alias becomes invalid, a pubkey-based path wouldn't help either—so compact remains the better default. In brief: This commit makes the default behavior match what users actually want. Thanks to [@TheBlueMatt](https://github.com/TheBlueMatt) for the original reasoning. **Discussion link:** [lightningdevkit#3246 (pull request review)](lightningdevkit#3246 (review))
To simplify blinded path creation and uphold the principle of "One `MessageRouter`, one `BlindedPath` type," this commit updates `create_offer_builder` to use the `create_blinded_paths` method of the `MessageRouter`. Now, when `create_offer_builder` is called, the offer will be created using the `MessageRouter` implementation that the `ChannelManager` or `OffersMessageFlow` is parameterized with. If a user wishes to create an offer with a different type of blinded path, they can explicitly use `create_offer_builder_using_router`, which allows passing a custom `MessageRouter`. The reasoning behind this change is to give users clearer, more deterministic control over the type of blinded path used in the offer. It also improves user awareness, ensuring that creating a non-default blinded path becomes an *intentional choice*.
This change mirrors the previous update to `create_offer_builder`, applying the **“One `MessageRouter`, one `BlindedPath` type”** principle to refund creation. Now, `create_refund_builder` uses the `create_blinded_paths` method of the `MessageRouter` associated with the `ChannelManager` or `OffersMessageFlow`. For non-default path behavior, users can call `create_refund_builder_using_router` and pass a custom `MessageRouter`. See previous commit for detailed reasoning.
This commit completes the series implementing the principle: **“One `MessageRouter`, one `BlindedPath` type.”** As the final step, it removes now-redundant variations of the blinded path creation functions, streamlining the API and simplifying the blinded path creation process.
Introduced tests to validate the behavior of Offers and Refunds created without blinded paths, using `NullMessageRouter`.
shaavan
force-pushed
the
review-blinded_api
branch
from
b4cc6ce to
62d056b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lightning-dns-resolver/src/lib.rs(4 hunks)lightning/src/ln/channelmanager.rs(7 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(47 hunks)lightning/src/offers/flow.rs(5 hunks)lightning/src/onion_message/messenger.rs(6 hunks)lightning/src/util/test_utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lightning/src/ln/max_payment_path_len_tests.rs
- lightning-dns-resolver/src/lib.rs
- lightning/src/util/test_utils.rs
- lightning/src/ln/offers_tests.rs
- lightning/src/onion_message/messenger.rs
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-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: build (macos-latest, 1.63.0)
- GitHub Check: fuzz
- GitHub Check: benchmark
- GitHub Check: check_commits
- GitHub Check: linting
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (ubuntu-latest, beta)
- GitHub Check: build (windows-latest, beta)
- GitHub Check: build (macos-latest, beta)
- GitHub Check: build (ubuntu-latest, 1.63.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: coverage
- GitHub Check: ext-test
- GitHub Check: semver-checks
🔇 Additional comments (15)
lightning/src/ln/channelmanager.rs (6)
2215-2216: Documentation example correctly updated.The example properly reflects the simplified
create_offer_builder()API without theabsolute_expiryparameter.
10849-10857: Privacy documentation looks good.The updated documentation clearly explains how the
MessageRouteris used and the privacy implications.
10866-10912: Well-implemented offer builder methods.The refactored
create_offer_builderand newcreate_offer_builder_using_routermethods are properly implemented with:
- Clear separation of concerns delegating to the flow object
- Comprehensive documentation covering privacy, limitations, and error conditions
- Appropriate generic constraints for the custom router variant
10935-10942: Privacy documentation for refund builder is clear.The documentation properly explains the use of
MessageRouterand derived payer ID for privacy.
10974-11047: Excellent implementation of refund builder with custom router.The
create_refund_builder_using_routermethod is well-designed with:
- Comprehensive documentation covering all aspects (payment, privacy, limitations, errors)
- Proper error handling for duplicate payment IDs
- Correct setup of pending outbound payments with expiration tracking
- Appropriate use of persistence guard
17180-17180: Test correctly updated for new TestMessageRouter API.The benchmark test properly uses
TestMessageRouter::new_defaultwhich aligns with the new enum-based design.lightning/src/offers/flow.rs (9)
36-36: Minor formatting improvement in imports.The import formatting change looks clean and follows Rust conventions.
144-153: Well-implemented time handling for async payments.The method properly handles both
stdandno_stdenvironments with appropriate conditional compilation. The fallback tohighest_seen_timestampforno_stdis a sensible approach.
413-441: Excellent refactoring with proper separation of concerns.The
create_offer_builder_internmethod effectively centralizes offer builder creation logic using a closure pattern for path generation. This allows for different path creation strategies while maintaining consistent core logic.
481-514: Good addition of flexible router support.The new
create_offer_builder_using_routermethod provides valuable flexibility by allowing customMessageRouterimplementations. The method follows the established pattern and maintains consistency with the existing API design.
531-534: Clean refactoring using the new helper method.The simplification of
create_async_receive_offer_builderto use the newcreate_offer_builder_internhelper is well-executed and maintains the same functionality.
536-577: Consistent refactoring pattern for refund builders.The
create_refund_builder_internmethod follows the same excellent pattern as the offer version, maintaining consistency across the codebase. The parameter ordering and closure usage are well-designed.
618-629: Proper refactoring maintains existing functionality.The modification to use
create_refund_builder_interncorrectly maintains all existing functionality while benefiting from the centralized logic.
631-688: Comprehensive documentation and flexible API design.The new
create_refund_builder_using_routermethod provides excellent documentation covering privacy, payment tracking, limitations, and error conditions. The method signature and implementation are consistent with the offer builder counterpart.
467-479: Breaking change: Verify all callers have been updated.The method signature has changed by removing the
absolute_expiryparameter. While the refactoring improves the API design, this is a breaking change that requires verification of all callers.#!/bin/bash # Description: Search for all usages of create_offer_builder to verify they've been updated # Expected: All calls should match the new signature without absolute_expiry parameter echo "Searching for create_offer_builder method calls..." rg -A 3 -B 1 "\.create_offer_builder\(" --type rust echo -e "\nSearching for create_offer_builder function definitions..." ast-grep --pattern 'fn create_offer_builder($$$) { $$$ }'
| /// will included a [`BlindedMessagePath`] created using: | ||
| /// - [`MessageRouter::create_compact_blinded_paths`] when short-lived, and | ||
| /// - [`MessageRouter::create_blinded_paths`] when long-lived. | ||
| /// will included a [`BlindedMessagePath`] created using [`MessageRouter::create_blinded_paths`] |
There was a problem hiding this comment.
Fix grammatical error in documentation.
Apply this diff to fix the typo:
-/// will included a [`BlindedMessagePath`] created using [`MessageRouter::create_blinded_paths`]
+/// will include a [`BlindedMessagePath`] created using [`MessageRouter::create_blinded_paths`]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// will included a [`BlindedMessagePath`] created using [`MessageRouter::create_blinded_paths`] | |
| /// will include a [`BlindedMessagePath`] created using [`MessageRouter::create_blinded_paths`] |
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs at line 3024, fix the grammatical error in
the documentation comment by changing "will included" to "will include" to
correct the tense and ensure proper grammar.
Mirrors #3246
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor