Skip to content
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

Add listchannels to execute_dev_command #949

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

Conversation

JssDWt
Copy link
Contributor

@JssDWt JssDWt commented Apr 26, 2024

The existing implementation of execute_dev_command did not allow for parameterized commands. The following things were changed:

  • the execute_dev_command sdk function can now take parameterized commands like "listchannels --source xxx"
  • The sdk-cli now handles parameterized commands properly as if it's a commandline.
  • the listchannels function was added, with optional parameters source, destination, and short_channel_id, so we are able to query the graph on greenlight

@JssDWt JssDWt requested review from roeierez and ok300 April 26, 2024 07:59
@ok300
Copy link
Contributor

ok300 commented Apr 26, 2024

Isn't it overkill to include clap, a CLI parsing dependency, in the SDK?

If the goal is to extend dev commands with flexible calls that support optional parameters, then I suppose there's no way around it.

But if we instead want to add selected command variants that are especially useful, maybe we can instead go for e.g.

  • listChannelsForSource <source> with one expected arg
  • listChannelsForSourceAndDest <source> <dest> with two expected args

What are your thoughts on this?

@ok300
Copy link
Contributor

ok300 commented Apr 26, 2024

cc @erdemyerebasmaz : this is also relevant for Breez Cloud.

@erdemyerebasmaz erdemyerebasmaz self-assigned this Apr 26, 2024
@JssDWt JssDWt force-pushed the jssdwt-listchannels branch from beb6875 to 69f350c Compare April 26, 2024 08:59
@JssDWt JssDWt force-pushed the jssdwt-listchannels branch from 69f350c to c5d1696 Compare April 26, 2024 09:03
@JssDWt
Copy link
Contributor Author

JssDWt commented Apr 26, 2024

Isn't it overkill to include clap, a CLI parsing dependency, in the SDK?

My reasoning is: either we write parsing logic for parameterized commands ourselves, or we add a dependency to do so. The advantage is it maps to the enum fields easily. If we would do listChannelsForSource <source>, we would do something like:

  • split string
  • get first element, if there is no first element, error
  • parse first element to enum variant
  • pass the second element (if any) somewhere outside of this flow, so it can be used in the listchannels variant

I thought a cli parsing library was easier and more robust, also considering potential quoted strings with spaces (even though they are not part of the interface currently)

@dangeross
Copy link
Collaborator

Isn't it overkill to include clap, a CLI parsing dependency, in the SDK?

The only other option I would see is to implement a nested subcommand into the CLI to handle the params specific to listchannels, then instead of passing a command string to execute_dev_command, pass an enum with a variant to include the optional params

@JssDWt
Copy link
Contributor Author

JssDWt commented Apr 29, 2024

The only other option I would see is to implement a nested subcommand into the CLI to handle the params specific to listchannels, then instead of passing a command string to execute_dev_command, pass an enum with a variant to include the optional params

That would work too. It would be a breaking interface change, but maybe favorable over magic commandline strings?

@dangeross
Copy link
Collaborator

It would be a breaking interface change

True, but the method should only be used for testing/debugging anyway

@JssDWt
Copy link
Contributor Author

JssDWt commented Apr 29, 2024

The only other option I would see is to implement a nested subcommand into the CLI to handle the params specific to listchannels, then instead of passing a command string to execute_dev_command, pass an enum with a variant to include the optional params

@roeierez What do you think?

@roeierez
Copy link
Member

roeierez commented May 2, 2024

@roeierez What do you think?

What do you think about exposing the node::ClnClient from the NodeAPI and the BreezServices?
This way the user can use directly the node methods without going through the dev command.
Enum variant for every API call add a maintenance burden on us to re-write the cln API.

@JssDWt
Copy link
Contributor Author

JssDWt commented May 3, 2024

What do you think about exposing the node::ClnClient from the NodeAPI and the BreezServices?

It does tie us to the core lightning implementation. Thinking about the 'LDK support' in the roadmap. Also I wonder how that plays with language bindings. I don't personally mind the maintenance burden of supporting this enum.

@roeierez
Copy link
Member

roeierez commented May 8, 2024

What do you think about exposing the node::ClnClient from the NodeAPI and the BreezServices?

It does tie us to the core lightning implementation. Thinking about the 'LDK support' in the roadmap. Also I wonder how that plays with language bindings. I don't personally mind the maintenance burden of supporting this enum.

We can do that without exposing the implementation by returning Any type as the underline type and let the user downcast it but the binding is indeed won't work well with that.
I am good with the enum.

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.

5 participants