-
Notifications
You must be signed in to change notification settings - Fork 113
Add LSPS5 webhook notification support #729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Integrates LSPS5 (BLIP-0055) from lightning-liquidity to enable webhook-based push notifications for clients. - Add event system integration for LSPS5 events - Expose public API for webhook management This allows client developers to register webhook endpoints with their LSP to receive notifications when their app is offline.
|
👋 Hi! I see this is a draft PR. |
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| /// | ||
| /// [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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we need to replicate this struct?
Integrates LSPS5 (BLIP-0055) from lightning-liquidity to enable webhook-based push notifications for clients.
This allows client developers to register webhook endpoints with their LSP to receive notifications when their app is offline.