breez-sdk

breez-sdk copied to clipboard

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

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?

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

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)

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

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?

It would be a breaking interface change

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

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 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.

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.

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.