Refactor liquidity source to support multiple LSP nodes#792
Refactor liquidity source to support multiple LSP nodes#792Camillarhi wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
@tnull Early draft up for review, would appreciate feedback on the general API direction |
tnull
left a comment
There was a problem hiding this comment.
Thanks! Some initial questions..
src/liquidity.rs
Outdated
| supported_protocols: Mutex::new(None), | ||
| }) | ||
| .collect(), | ||
| pending_lsps1_opening_params_requests: Mutex::new(HashMap::new()), |
There was a problem hiding this comment.
Uh, no, I don't think we should just merge all into one. Especially given that we intend to add more logic on a per-spec basis, this will be will become even more confusing going forward. If anything, we should maybe start the refactoring by first moving the LSPS1/LSPS2 specific parts to src/liquidity/{lsps1,lsps2}.rs, or maybe even to client/service specific sub-modules like src/liquidity/{client,service}/{lsps1,lsps2}.rs.
There was a problem hiding this comment.
Alright, I will move the LSPS1/LSPS2 specific parts
src/liquidity.rs
Outdated
| pub(crate) async fn lsps1_request_channel( | ||
| &self, lsp_balance_sat: u64, client_balance_sat: u64, channel_expiry_blocks: u32, | ||
| announce_channel: bool, refund_address: bitcoin::Address, | ||
| announce_channel: bool, refund_address: bitcoin::Address, node_id: &PublicKey, |
There was a problem hiding this comment.
Hmm, I do wonder if it would make sense to create something like an struct ServiceProvider and move the API methods to there. Then, each registered LSP would have a corresponding ServiceProvider that exposes a bunch of public and internal APIs, which would make the modularization cleaner and would avoid having to give node_id everywhere?
There was a problem hiding this comment.
Makes sense, I will have a look and see
3cda7f7 to
8bbf55a
Compare
|
Hello @tnull, apologies for the delay. This is ready for another round of review |
8bbf55a to
ea93afe
Compare
tnull
left a comment
There was a problem hiding this comment.
Cool, already looks pretty good! Did a first higher-level pass, have yet to look into the details.
| @@ -1,1542 +0,0 @@ | |||
| // This file is Copyright its original authors, visible in version control history. | |||
There was a problem hiding this comment.
Would be good to structure this PR in a way that (as far as possible) makes any code moves dedicated commits that can be picked up by git diff --color-moved --patience, as otherwise reviewing this in detail will be very hard.
src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| discovery_ls.discover_all_lsp_protocols().await; |
There was a problem hiding this comment.
I think it would be good to make this part of the background task above? Also, can we spawn the discovery tasks in parallel rather than doing them sequentially?
src/lib.rs
Outdated
| } | ||
|
|
||
| /// Configures the [`Node`] instance to source inbound liquidity from the given LSP at runtime, | ||
| /// without specifying the exact protocol used (e.g., LSPS1 or LSPS2). |
There was a problem hiding this comment.
I think we can drop the remark regarding 'without specifying the exact protocol' here and elsewhere, as the API already communicates that due to being generic. I do however wonder if we'd want to move this method to an API-extension object similar to what we do for the payment types? I.e., retrieve the API object via Node::liquidity()?
There was a problem hiding this comment.
Thanks. I will remove the remark and move this to a Node::liquidity() API-extension
| /// [`Bolt11Payment::receive_via_jit_channel`]: crate::payment::Bolt11Payment::receive_via_jit_channel | ||
| #[derive(Clone)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Object))] | ||
| pub struct LSPS1Liquidity { |
There was a problem hiding this comment.
It seems this is currently not exposed in the API anymore?
src/builder.rs
Outdated
| /// The given `token` will be used by the LSP to authenticate the user. | ||
| /// | ||
| /// [bLIP-51 / LSPS1]: https://github.com/lightning/blips/blob/master/blip-0051.md | ||
| #[deprecated(note = "Use `add_lsp` instead")] |
There was a problem hiding this comment.
Hmm, I think so far we've been fine with just breaking the APIs without deprecating them first. If we find a better API I'd be fine with just dropping the old ones to clean up.
ea93afe to
6061c6d
Compare
Replace per-protocol single-LSP configuration `LSPS1Client, LSPS2Client` with a unified `Vec<LspNode>` model where users configure LSP nodes via `add_lsp()` and protocol support is discovered at runtime via LSPS0 `list_protocols`. - Replace separate `LSPS1Client/LSPS2Client` with global pending request maps keyed by `LSPSRequestId` - Add LSPS0 protocol discovery `discover_lsp_protocols` with event handling for `ListProtocolsResponse` - Update events to use is_lsps_node() for multi-LSP counterparty checks - Deprecate `set_liquidity_source_lsps1/lsps2` builder methods in favor of `add_lsp()` - LSPS2 JIT channels now query all LSPS2-capable LSPs and automatically select the cheapest fee offer across all of them - Add `request_channel_from_lsp()` for explicit LSPS1 LSP selection - Spawn background discovery task on `Node::start()`
6061c6d to
a560764
Compare
Opening this as an early draft to get feedback on the overall API direction
The current setup ties you to a single LSP per protocol via
set_liquidity_source_lsps1/set_liquidity_source_lsps2. This refactor replaces that with a unifiedVec<LspNode>model where LSP nodes are added viaadd_lsp()and protocol support is discovered at runtime through LSPS0list_protocols. Multi-LSP support has been requested previously in #529.set_liquidity_source_lsps1/set_liquidity_source_lsps2in favor ofadd_lsp()LSPS1Client/LSPS2Clientwith global pending request maps keyed byLSPSRequestIdListProtocolsResponseNode::start()request_channel_from_lsp()for explicit LSPS1 LSP selectionis_lsps_node()for multi-LSP counterparty checksThis sets the foundation for LSPS5 support currently being worked on in #729