Refactor liquidity source to support multiple LSP nodes#792
Refactor liquidity source to support multiple LSP nodes#792Camillarhi wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
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()`
|
👋 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
tnull
left a comment
There was a problem hiding this comment.
Thanks! Some initial questions..
| /// This method is useful when the user wants to connect to an LSP but does not want to be concerned with | ||
| /// the specific protocol used for liquidity provision. The node will automatically detect and use the | ||
| /// appropriate protocol supported by the LSP. | ||
| pub fn add_lsp( |
There was a problem hiding this comment.
Hmm, I think there previously were users that required to be able to configure the LSPs at runtime. Not sure if they solved it differently by now, but we could at least explore whether this is possible, e.g., by allowing to set this on a sub-API struct retrievable from Node (similar to what we do for payments), e.g. via node.liquidity().add_lsp().
| 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.
| 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?
3 participants
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