Rename most lightning-liquidity types for bindings compatibility
#3583
Conversation
tnull
force-pushed
the
2025-01-rename-all-liquidity-types
branch
2 times, most recently
from
6dd41b7 to
9759af4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
- Coverage 88.55% 88.54% -0.02%
==========================================
Files 149 149
Lines 114944 114973 +29
Branches 114944 114973 +29
==========================================
+ Hits 101794 101799 +5
- Misses 10663 10682 +19
- Partials 2487 2492 +5 ☔ View full report in Codecov by Sentry. |
tnull
force-pushed
the
2025-01-rename-all-liquidity-types
branch
2 times, most recently
from
9db2575 to
2643ee0
Compare
Recently, LSPS0, 1, and 2 were upstreamed as bLIP-50, 51, and 52, respectively. Here, we 1. start linking to the bLIPs instead of the LSP spec repository, which is likely going to be deprecated. 2. start consistently citing the specs as `bLIP-5X / LSPSX` to avoid any confusions and to potentially initiate a process in which the LSP specs will be referred to by their bLIP number rather than their LSPS number (especially given that any upcoming specs by the LSP spec group will directly be drafted as bLIPs going forward).
As we're not testing with `cfg(lsps1_service)`, the builds broke during previous refactoring. This isn't bad as we're looking to completely overhaul / rewrite LSPS1 service soon, but until then we fix this pre-existing breakage to make sure following changes make sense.
In order to avoid naming collisions with LDK's `Event` type, we rename `lightning-liquidity`'s `Event` to `LiquidityEvent`. To minimize furhter churn on the upcoming renaming changes, we also `impl From X for LiquidityEvent` for the protocol-specific event variants, which also allows us to reduce some boilerplate while enqueuing.
since it is
This wrapper is more ergonomic to use in the local context and will be used as a serialization wrapper in following commits.
tnull
force-pushed
the
2025-01-rename-all-liquidity-types
branch
from
2643ee0 to
690fcb1
Compare
|
I have no idea how this is possible, but it seems this didn't have any conflicts with #3510. |
| #[serde(transparent)] | ||
| pub struct LSPSDateTime(chrono::DateTime<chrono::Utc>); | ||
|
|
||
| impl LSPSDateTime { |
There was a problem hiding this comment.
Ah, thanks for doing this.
Yeah, see the PR description: unfortunately I think it doesn't make sense to completely switch to bLIP terminology just yet if the specs themselves are still using LSPS everywhere. And of course the actual protocol methods are prefixed by |
Based on #3510.
Supersedes #3574.
Here, we refactor the
lightning-liquiditycrate to ensure it's easier to work with in bindings generation, i.e., mainly ensure that types won't conflict with other LDK types. To this end, we renameEventtoLiquidityEvent, and prefix all LSPS0/1/2 related types withLSPS0/1/2.Note I didn't go for the
BLIP50/51/52naming as the LSPSX naming is pretty engrained in the specs currently (e.g., actual JSON API calls are namespaced by thelspsXprefixes), so if we'd ever want to change everything to be referred by bLIP numbers, the specs themselves should likely get cleaned up first (not sure if we'd ever be happy to do deal with the parts that break compat though).I also tagged on a commit I had already lying around for upcoming persistence work which introduces a
DateTimewrapper, as I assume that might be easier thanchrono::DateTimeto work with in bindings.