Skip to content

Add close-channel command to the CLI #65

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

moisesPompilio
Copy link

CLI: Add support for close-channel command

This adds a simple close-channel command to the CLI interface.
It reuses the existing close_channel method from ldk-server-client.

Closes #55.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 18, 2025

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz June 18, 2025 19:49
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing!

Comment on lines 214 to 215
force_close,
force_close_reason,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a commit updating handle_close_channel_request to only allow force_close_reason when force_close is Some(true)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change applied as requested, thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@tnull tnull Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tnull tnull requested a review from jkczyz June 20, 2025 08:59
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Move parsing logic into dedicated helper functions in the api module and update close_channel
handling to use them.
Introduce ForceCloseChannelRequest and ForceCloseChannelResponse to the API,
implement the handle_force_close_channel_request function, and register the new
endpoint in the service router.
@moisesPompilio
Copy link
Author

Split channel closing into separate endpoints for mutual and force close

  • Removed force_close and force_close_reason from CloseChannel.
  • Added a dedicated ForceCloseChannel endpoint.
  • This ensures proper argument validation and avoids misuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

command in CLI to close channel
4 participants