feat: add close_channels_with_peer command to gateway liquidity API #5218
Conversation
CLN supports closing a channel by peer id: https://docs.corelightning.org/reference/lightning-close For LND/LDK, you could lookup the TXID + output index using something like Maybe it makes more sense to have the API accept a |
That works, but if there are multiple channels open with the same peer then CLN refuses to close any channels:
What do you think about adding a |
tvolk131
force-pushed
the
liquidity_api_close_channel
branch
3 times, most recently
from
ef05669
to
e0ab490
Compare
tvolk131
changed the title
|
|
||
| client | ||
| .lightning() | ||
| .close_channel(CloseChannelRequest { |
There was a problem hiding this comment.
We might eventually want a way to query the status of closed channels. This RPC returns a stream and (especially for LDK), the operator will probably want to know what the state of their channel close is.
There was a problem hiding this comment.
I talked with tnull and it sounds like ldk-node v0.3 will provide a much better API for tracking channel lifecycle. It sounds like v0.3 will be released very soon, so I'll probably wait for that and then put more thought into how we want to expose channel state tracking behavior.
|
Overall I think its good enough to merge, I would prefer to fix the String vs bytes issue though. Perhaps we should start adding devimint tests that exercise these endpoints? |
tvolk131
force-pushed
the
liquidity_api_close_channel
branch
2 times, most recently
from
5bd19dd
to
1880735
Compare
Agreed! First I'd like to remove |
There was a problem hiding this comment.
Looks ok enough, feel free to merge. Seconding @m1sterc001guy's comments about testing.
| .into_inner() | ||
| .channels; | ||
|
|
||
| for channel in &channels_with_peer { |
There was a problem hiding this comment.
A failure in this loop would be annoying since the first channels are already closed while later ones stay open. Seems like a potential source of bugs. I'd much prefer the "single channel closing" semantics eventually.
There was a problem hiding this comment.
This function doesn't wait for the channel to close anyway, we'll eventually need a way to track that as well. Rather than failing and returning here (and above in the CLN implementation), I think emitting a warning and trying to close the next channel would be sufficient
gateway/cli/src/main.rs
Outdated
| let num_channels_closed = client() | ||
| .close_channels_with_peer(CloseChannelsWithPeerPayload { pubkey }) | ||
| .await?; | ||
| println!("Closed {num_channels_closed} channel(s)"); |
There was a problem hiding this comment.
Ick. Can we change this to return JSON with num_channels_closed as one of the fields? That way tests can parse it and we can use print_response here instead
tvolk131
force-pushed
the
liquidity_api_close_channel
branch
from
1880735
to
e7706d0
Compare
| @@ -9,6 +9,7 @@ fn main() { | |||
| .build_server(true) | |||
| .build_client(true) | |||
| .protoc_arg("--experimental_allow_proto3_optional") | |||
| .type_attribute(".", "#[derive(serde::Serialize, serde::Deserialize)]") | |||
There was a problem hiding this comment.
Note for reviewer: We're adding this so we can encode CloseChannelsWithPeerResponse as JSON
This will be an important command to have once we ship an LDK-powered gateway since the liquidity API will be the only way to manage channels
Only implementing for LND for now since LND and LDK support closing channels by funding TXID + output index, while CLN instead requires funding tx block height + transaction index + output indexChanged from closing by TXID + output index to closing all channels with a specific peer