-
Notifications
You must be signed in to change notification settings - Fork 116
Account for LiquidityManager
persistence
#650
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
Conversation
👋 I see @jkczyz was un-assigned. |
dad5932
to
53d4f0a
Compare
We decided to make the |
We recently implemented persistence for the `lightning-liquidity` service state. Here we make corresponding changes in LDK Node to have our service state persisted.
53d4f0a
to
a404a94
Compare
Ready for review now that #633 landed. |
liquidity_client_config, | ||
) | ||
.await | ||
.map_err(|_| BuildError::ReadFailed)?, |
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.
Reading data in new
, isn't that an undesirable pattern?
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.
Maybe, yes. We did it over at lightningdevkit/rust-lightning#4059 since the individual service states are persisted under different locations and the LiquidityManager
is the singular interface on which you can decide which services to enable. So allowing an API that allows to enable some services but not reading all the service states seemed complicated and error-prone (i.e., worse than reading them all in new
.
|
||
let liquidity_source = Arc::new(liquidity_source_builder.build()); | ||
let liquidity_source = runtime | ||
.block_on(async move { liquidity_source_builder.build().await.map(Arc::new) })?; |
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.
Assuming this is temporary until async building?
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.
Yes, we'll remove this when we move building to Node::start
and make it async
.
Based on #633.
We recently implemented persistence for the
lightning-liquidity
servicestate. Here we make corresponding changes in LDK Node to have our
service state persisted.