implement LSPS5 #3499
implement LSPS5 #3499
Conversation
johncantrell97
force-pushed
the
lsps5
branch
from
a200ae5 to
7f49f93
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3499 +/- ##
==========================================
- Coverage 88.34% 88.26% -0.09%
==========================================
Files 149 149
Lines 112915 112991 +76
Branches 112915 112991 +76
==========================================
- Hits 99753 99729 -24
- Misses 10685 10770 +85
- Partials 2477 2492 +15 ☔ View full report in Codecov by Sentry. |
johncantrell97
force-pushed
the
lsps5
branch
from
7f49f93 to
c6893d7
Compare
There was a problem hiding this comment.
Ah, cool, thanks for looking into this! I have yet to properly review this, but here are a few first comments.
Generally, this a huge diff. I'd prefer to break this up over multiple PRs, e.g., adding persistence in a follow-up (preferably when we also worked out a common scheme across LSPS1/2/5). And, at the very least, please break this PR up into multiple logical commits rather than a single huge one. This would making reviewing it much easier. Thanks!
| @@ -17,6 +17,7 @@ categories = ["cryptography::cryptocurrencies"] | |||
| default = ["std"] | |||
| std = ["lightning/std"] | |||
| backtrace = ["dep:backtrace"] | |||
| lsps5 = ["minreq", "url"] | |||
There was a problem hiding this comment.
I don't think we'll want to add an HTTP client dependency here, especially since recently rustls bumped MSRV.
As send_webhook doesn't even return any data, we should be fine just creating an event variant for it and letting the service implement it however they want, no?
Feel free to convert the lsps5 feature to an lsps5 cfg-gate while you're here though, that would allow us to land this before we're sure LSPS5 is fully ready to be shipped.
| @@ -0,0 +1 @@ | |||
| //! Utilities for implementing the LSPS5 standard. | |||
There was a problem hiding this comment.
This file can just be dropped, no?
| // You may not use this file except in accordance with one or both of these | ||
| // licenses. | ||
|
|
||
| //! Implementation of LSPS2: JIT Channel Negotiation specification. |
| for app_name in registered_app_names.into_iter() { | ||
| let webhook_bytes = self | ||
| .kv_store | ||
| .read(WEBHOOK_PRIMARY_NAMESPACE, &counterparty_node_id.to_string(), &app_name) |
There was a problem hiding this comment.
Mhh, how much data are we expecting here? Should all of the webhooks live under a single key (and in memory) rather then reading them on a per-peer basis from IO on-demand?
| service_config: Option<LiquidityServiceConfig>, | ||
| _client_config: Option<LiquidityClientConfig>, | ||
| best_block: RwLock<Option<BestBlock>>, | ||
| _chain_source: Option<C>, | ||
| _kv_store: Option<KV>, |
There was a problem hiding this comment.
Mhh, I'm generally not sure if persistence should be added in this PR, especially given that we still have to work out a common persistence scheme across LSPS1/2/5. At the very least this should be broken out to (a number of) different commits.
|
Thanks for the feedback. Will close this and break it up into much smaller, easier-to-review PR's instead. |
Implements LSPS5
I put it behind a feature flag because of the added dependencies of
minreqandurl. Alternatively, could rip these dependencies out and just have the liquidity crate surface an event to the LSP and have them make the http request themselves.Has integration test that registers a webhook, lists them, and then asks to remove it.
TODO: Webhook notification message signing.