lightning-liquidity: Refactor LSPS1 service-side#4282
Open
tnull wants to merge 29 commits intolightningdevkit:mainfrom
Open
lightning-liquidity: Refactor LSPS1 service-side#4282tnull wants to merge 29 commits intolightningdevkit:mainfrom
lightning-liquidity: Refactor LSPS1 service-side#4282tnull wants to merge 29 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
4 times, most recently
from
773316a to
fb519ab
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4282 +/- ##
==========================================
+ Coverage 86.01% 86.22% +0.21%
==========================================
Files 156 158 +2
Lines 102857 103776 +919
Branches 102857 103776 +919
==========================================
+ Hits 88474 89484 +1010
+ Misses 11876 11705 -171
- Partials 2507 2587 +80
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:
|
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
2 times, most recently
from
b96685b to
c6eb6b3
Compare
We add the first LSPS1 integration test. This is based on the unfinished work in lightningdevkit#3864, but rebased to account for the new ways we now do integration test setup.
.. for which we got warnings
We previously considered tracking payment confirmations as part of the handler. However, we can considerably simplify our logic if we stick with the current approach of having the LSPs track the payment status and update us when prompted through events.
Now that we don't do on-chain tracking in LSPS1, we can drop quite a few `LiquidityManager` parameters and generics, which were only added in anticipation of tracking on-chain state. Signed-off-by: Elias Rohrer <dev@tnull.de>
We move the `PeerState` related types to a new module. In the following commits we'll bit-by-bit drop the `pub(super)`s introduced here, asserting better separation of state and logic going forward.
.. we will re-add a proper state machine in a later commit, but for now we can just drop all of this half-baked logic that doesn't actually do anything.
.. requiring less access to internals
Previously, we'd directly access the internal `outbound_` map of `PeerState`. Here we refactor the code to avoid this. Note this also highlighted a bug in that we currently don't actually update/persist the order state in `update_order_state`. We don't fix this here, but just improve isolation for now, as all state update behavior will be reworked later.
We introduce two new methods on `PeerState` to avoid direct access to the internal `pending_requests` map.
The `OutboundChannel` construct simply wrapped `ChannelOrder` which we can now simply use directly.
We here remember and update the order state and channel details in `ChannelOrder`
Since we by now have the `TimeProvider` trait, we might as well use it in `LSPS1ServiceHandler` instead of requiring the user to provide a `created_at` manually. Signed-off-by: Elias Rohrer <dev@tnull.de>
In the future we might want to inline the fields in `LSPS1ServiceConfig` (especially once some are added that we'd want to always/never set for the user), but for now we just make the `supported_options` field in `LSPS1ServiceConfig` required, avoiding some dangerous `unwrap`s.
Previously, we'd use an event to have the user check the order status and then call back in. As we already track the order status, we here change that to a model where we respond immediately based on our state and have the user/LSP update that state whenever it detects a change (e.g., a received payment, reorg, etc.). In the next commmit we will add/modify the corresponding API methods to do so.
We add the serializations for all types that will be persisted as part of the `PeerState`.
We follow the model already employed in LSPS2/LSPS5 and implement state pruning and persistence for `LSPS1ServiceHandler` state. Signed-off-by: Elias Rohrer <dev@tnull.de>
.. we read the persisted state in `LiquidityManager::new` Signed-off-by: Elias Rohrer <dev@tnull.de>
Co-authored by Claude AI
As per spec, we check that the user provides at least one payment detail *and* that they don't provide onchain payment details if `refund_onchain_address` is unset.
.. as there's no need to do so.
We add a method that allows the LSP to signal to the client the token they used was invalid. We use the `102` error code as proposed in lightning/blips#68.
We test the just-added API. Co-authored by Claude AI
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
c6eb6b3 to
a2aa7c3
Compare
Contributor
Author
|
Rebased to resolve conflicts. |
This refactors `ChannelOrder` to use an internal state machine enum `ChannelOrderState` that: - Encapsulates state-specific data in variants (e.g., `channel_info` only available in `CompletedAndChannelOpened`) - Provides type-safe state transitions - Replaces the generic `update_order_status` API with specific transition methods: `order_payment_received`, `order_channel_opened`, and `order_failed_and_refunded` The state machine has four states: - `ExpectingPayment`: Initial state, awaiting payment - `OrderPaid`: Payment received, awaiting channel open - `CompletedAndChannelOpened`: Terminal state with channel info - `FailedAndRefunded`: Terminal state for failed/refunded orders Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
Add two new integration tests to cover the new public API methods: - `lsps1_order_state_transitions`: Tests the full flow of `order_payment_received` followed by `order_channel_opened`, verifying that payment states are updated correctly and channel info is returned after the channel is opened. - `lsps1_order_failed_and_refunded`: Tests the `order_failed_and_refunded` method, verifying that payment states are set to Refunded. Co-Authored-By: HAL 9000
Add `lsps1_expired_orders_are_pruned_and_not_persisted` test that verifies: - Orders with expired payment details (expires_at in the past) are accessible before persist() is called - After persist() is called, expired orders in ExpectingPayment state are pruned and no longer accessible - Pruned orders are not recovered after restart, confirming that the pruning also removes the persisted state Co-Authored-By: HAL 9000
The bLIP-51 specification defines a `HOLD` intermediate payment state: - `EXPECT_PAYMENT` -> `HOLD` -> `PAID` (success path) - `EXPECT_PAYMENT` -> `REFUNDED` (failure before payment) - `HOLD` -> `REFUNDED` (failure after payment received) This commit adds the `Hold` variant to `LSPS1PaymentState` and updates the state machine transitions: - `payment_received()` now sets payment state to `Hold` (not `Paid`) - `channel_opened()` transitions payment state from `Hold` to `Paid` - Tests updated to verify the correct state at each transition This allows LSPs to properly communicate when a payment has been received but the channel has not yet been opened (e.g., Lightning HTLC held, or on-chain tx detected but channel funding not published). Co-Authored-By: HAL 9000
Contributor
Author
|
Should be good for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3480.
We 'refactor' (rewrite) the
LSPS1ServiceHandler, move state handling to a dedicatedPeerState, add an STM pattern, add persistence for the service state, add some more critical API paths, add test coverage, and finally remove thecfg(lsps1_service)flag.