Add close-channel command to the CLI#65
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
| force_close, | ||
| force_close_reason, |
There was a problem hiding this comment.
Could you add a commit updating handle_close_channel_request to only allow force_close_reason when force_close is Some(true)?
There was a problem hiding this comment.
Change applied as requested, thanks!
There was a problem hiding this comment.
Hmmm... I was thinking we should do this on the server side. But I don't have a problem with doing it here (cc: @tnull any preference?). But we should probably error if force_close_reason is set but force_close isn't rather than just ignoring it. See Commands::Bolt11Receive handling for an example.
There was a problem hiding this comment.
Makes sense! I think the validation should be included in handle_close_channel_request on the ldk-server, since it would also cover requests made through the API and return proper errors there as well.
There was a problem hiding this comment.
Agree, we should probably also validate and error on the server side if the user supplies an arguement that doesn't fit the intended use case. Alternatively, we could split the channel closing API in a mutual close and force close variant, which would ensure we always just set the right fields for the right calls.
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Split channel closing into separate endpoints for mutual and force close
|
| #[arg(short, long)] | ||
| counterparty_node_id: String, | ||
| }, | ||
|
|
| string user_channel_id = 1; | ||
| // The hex-encoded public key of the node to close a channel with. | ||
| string counterparty_node_id = 2; | ||
| // The reason for force-closing, can only be set while force closing a channel. |
There was a problem hiding this comment.
"can only be set while force closing a channel" isn't needed anymore.
| pub fn parse_user_channel_id(id: &str) -> Result<UserChannelId, LdkServerError> { | ||
| Ok(UserChannelId(id.parse::<u128>().map_err(|_| { | ||
| LdkServerError::new(InvalidRequestError, "Invalid UserChannelId.".to_string()) | ||
| })?)) | ||
| } | ||
|
|
||
| pub fn parse_counterparty_node_id(id: &str) -> Result<PublicKey, LdkServerError> { | ||
| PublicKey::from_str(id).map_err(|e| { | ||
| LdkServerError::new( | ||
| InvalidRequestError, | ||
| format!("Invalid counterparty node ID, error: {}", e), | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do these need to be pub? IMO, we should just leave these in close_channel.rs and not make a separate file for handle_force_close_channel_request. There's enough in common that it can go in close_channel.rs.
There was a problem hiding this comment.
Moved handle_force_close_channel_request and the shared parsing functions into close_channel.rs as suggested. Also removed the unnecessary pub modifiers.
Introduce ForceCloseChannelRequest and ForceCloseChannelResponse to the API, implement the handle_force_close_channel_request function, and register the new endpoint in the service router.
enigbe
left a comment
enigbe
left a comment
There was a problem hiding this comment.
I've reviewed and tested this. Looks good to me.
Add `close-channel` command to the CLI
5 participants
CLI: Add support for
close-channelcommandThis adds a simple
close-channelcommand to the CLI interface.It reuses the existing
close_channelmethod fromldk-server-client.Closes #55.