Add LSPS5 webhook notification support#729
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for taking this over!
Need to look into this PR closer, here are some early comments that I noted when briefly scrolling through.
| bdk_wallet = { version = "2.2.0", default-features = false, features = ["std", "keys-bip39"]} | ||
|
|
||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls", "blocking"] } |
There was a problem hiding this comment.
We shouldn't ever add the blocking feature for reqwest as it's fundamentally incompatible with the async variant.
| impl std::error::Error for HttpClientError {} | ||
|
|
||
| /// Trait for making HTTP requests. | ||
| pub trait HttpClient: Send + Sync + std::fmt::Debug { |
There was a problem hiding this comment.
All of this code seems like an outdated copy of what was there on the upstream LSPS5 PR at one point in time. Please remove anything related to HttpClient etc. Rather, we should just ... make the call via spawned async task using reqwest when we handle the SendWebhookNotification event.
There was a problem hiding this comment.
Thanks for the feedback! I've updated the PR to use reqwest and removed the HttpClient dependencies. This is still a work in progress, but that should address your concern about the outdated approach.
| /// | ||
| /// [bLIP-55 / LSPS5]: https://github.com/lightning/blips/blob/master/blip-0055.md | ||
| #[derive(Debug, Clone)] | ||
| pub struct LSPS5ServiceConfig { |
There was a problem hiding this comment.
Not sure why we need to replicate this struct?
Camillarhi
force-pushed
the
lsps5
branch
6 times, most recently
from
3fa9f5e to
5f1b187
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for looking into this, before I go into too much details, here are some comments regarding the chosen approach and/or the API design. Let me know what you think!
|
|
||
| /// Error type for HTTP client operations. | ||
| #[derive(Debug)] | ||
| pub enum HttpClientError { |
There was a problem hiding this comment.
Seems this isn't used anywhere. Let's drop it!
| } | ||
|
|
||
| /// Sets a custom HTTP client to be used by the LSPS5 service. | ||
| pub fn set_liquidity_http_client(&mut self, http_client: reqwest::Client) -> &mut Self { |
There was a problem hiding this comment.
No need to set a 'custom' client. Let's just use our usual client and drop this builder method.
There was a problem hiding this comment.
I did this because I wanted to mock the client in the test in order to test the SendWebhookNotification event. I will try another approach
| /// push notifications for Lightning events when the client is offline. | ||
| /// | ||
| /// [bLIP-55 / LSPS5]: https://github.com/lightning/blips/blob/master/blip-0055.md | ||
| pub fn set_liquidity_source_lsps5( |
There was a problem hiding this comment.
I think there is ~no immediate scenario where we'd like to set a different LSP for LSPS5 compared to LSPS1/LSPS2. So rather than having the user configure this explicitly, it should be automatically enabled if we detect that our LSPS1 or LSPS2 counterparty also supports LSPS5 (you can check that via LSPS0ClientHandler::list_protocols). Note that this however poses an API design challenge as suddenly we'll have to support getting notifications from multiple LSPs.
Note that for a while some users have requested multi-LSP support (cf. #529) and for the sake of simplicity we so far have punted on it. But I do wonder if it would now make sense to first make this refactor to a model where users just configure a number of LSP nodes and we lean on list_protocols to discover what the configured nodes are supporting. I think I'll have to think about it a bit more - what are your thoughts on this?
There was a problem hiding this comment.
Yeah, what if Instead of auto-enabling LSPS5 on top of the current model, we do the multi-LSP refactor first. Users would just configure LSP nodes (node_id + address + token), and we use list_protocols to discover what each one supports. LSPS5 then just works for any LSP that advertises it.
|
@Camillarhi Do you still intend to move forward with this PR? |
Yes, I do, apologies for the delayed response |
Implement the bLIP-55 / LSPS5 webhook registration protocol on top of the
multi-LSP liquidity module (src/liquidity/{client,service}).
Client side, exposed via Node::liquidity().lsps5():
- set_webhook / list_webhooks / remove_webhook to manage webhook
registrations with an LSP.
- When no node_id is given, set_webhook and remove_webhook fan out to every
LSPS5-capable LSP so a webhook can be configured once across all configured
LSPs; set_webhook returns one result per LSP that accepted the registration
and remove_webhook returns the LSPs it was removed from.
Service side, enabled via Builder::enable_liquidity_provider_lsps5():
- Deliver outgoing webhook notifications over HTTP in response to
LSPS5ServiceEvent::SendWebhookNotification.
- Automatically send an onion-message-incoming notification when an
intercepted onion message targets a client that is currently offline
(wired from LdkEvent::OnionMessageIntercepted, gated on peer connectivity).
Wires the feature through the UniFFI bindings and adds LSPS5-specific Error
variants.
3 participants
Integrates LSPS5 (bLIP-0055) from lightning-liquidity, enabling webhook-based push notifications so clients can be alerted to events while their app is offline. Built on the refactored multi-LSP liquidity module
(src/liquidity/{client,service}).When
node_idisNone,set_webhookandremove_webhookfan out to every LSPS5-capable LSP, so a webhook can be configured once across all configured LSPs.Lets a node act as an LSPS5 server:
LSPS5ServiceEvent::SendWebhookNotification.onion_message_incomingnotification when an intercepted onion message targets a client that is currently offline (wired fromLdkEvent::OnionMessageIntercepted, gated on peer connectivity via the peer manager).