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.

Sign up for GitHub

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

Jump to bottom

Commando enhancements and integrated cli support #5866

Merged
merged 10 commits into from
Jan 11, 2023
Merged

Commando enhancements and integrated cli support #5866

merged 10 commits into from
Jan 11, 2023

Conversation

rustyrussell
Copy link
Contributor

  • Clean up our JSON id handling generally.
  • Clean up JSON ids in commando commands specifically.
  • Support filtering on server-side via filter parameter for commando cmd.
  • Add --commando=peerid/rune convenience wrapper for lightning-cli.

@rustyrussell
libplugin: don't turn non-string JSON ids into strings.
cfd2957 
When called with `"id": 1` we replied with `"id": "1"`.  lightningd doesn't
actually care, but it's weird.

Copy the entire token: this way we don't have to special case anything.

Also, remove the doubled test in json_add_jsonstr.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: treat JSON ids as direct tokens.
3b7deb1 
This avoids any confusion between primitive and string ids, and in
particular stops an issue with commando once it starts chaining ids,
that weird ids can be double-escaped and commando will not recognize
the response, leaving the client hanging.  It's the client's fault for
using a weird id, but it's still rude (and triggered by our tests!).

It also makes substituting the id in passthrough simpler, FTW.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightning-cli: fix error code on invalid options, document them.
a333613 
The top of the file indicates the following errors:

    #define NO_ERROR 0
    #define ERROR_FROM_LIGHTNINGD 1
    #define ERROR_TALKING_TO_LIGHTNINGD 2
    #define ERROR_USAGE 3

But we didn't use the right one for opt_parse failure, and didn't use the
correct constants everywhere.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: require that we have an id field in JSON request.
5c319fa 
We don't do this yet, so we add deprecated to those test (until next
patch!).

Changelog-Deprecated: plugins: `commando` JSON commands without an `id` (see doc/lightningd-rpc.7.md for how to construct a good id field).
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: send id inside JSON request.
599b045 
All JSON-RPC calls should have one!

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: build ID of command based on the id they give us.
190b43a 
We change the libplugin API so commando can provide its own ID base.

This id chaining enables much nicer diagnostics!

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: track incoming and outgoing JSON IDs. Get upset if they don…
fb8b6a3 
…'t match!

They currently don't, so we get some BROKEN messages.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: correctly replace the id field in responses.
fd4ad2f 
This was reported a while ago: now do it properly.

Fixes: ElementsProject#5637
Changelog-Fixed: Plugins: `commando` now responds to remote JSON calls with the correct JSON `id` field.
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
commando: add filtering support.
b8208b9 
1. When we receive a commando command from a remote using the `filter`
   field, use it.
2. Add a `filter` parameter to `commando` to send it: this is usually
   more efficient than using filtering locally.

Of course, older remote nodes will ignore the filter, but that's
harmless.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugins: `commando` now supports `filter` as a parameter (for send and receive).
@rustyrussell
cli: add -c/--commando support.
06fead3 
It's easier to type:

```
lightning-cli --commando=03ce2d830369fc903ffec52ca1d7aba095c3cf5d17175b6c9a3ff058f6aece37bc:V08OylkJ2ZZPClAXbTaxrXJ9YpKnmucJxcQI-wvIGiE9MA== invoice any "Invoice Label" "Invoice Description"
lightning-cli --commando=03ce2d830369fc903ffec52ca1d7aba095c3cf5d17175b6c9a3ff058f6aece37bc:V08OylkJ2ZZPClAXbTaxrXJ9YpKnmucJxcQI-wvIGiE9MA== commando amount_msat=100000 label="invoice label" description="invoice description"
```

Than:

```
commando 03ce2d830369fc903ffec52ca1d7aba095c3cf5d17175b6c9a3ff058f6aece37bc invoice '["any", "Invoice Label", "Invoice Description"]' V08OylkJ2ZZPClAXbTaxrXJ9YpKnmucJxcQI-wvIGiE9MA==
commando 03ce2d830369fc903ffec52ca1d7aba095c3cf5d17175b6c9a3ff058f6aece37bc invoice '{"amount_msat": "100000", "label": "invoice label", "description": "invoice description"}' V08OylkJ2ZZPClAXbTaxrXJ9YpKnmucJxcQI-wvIGiE9MA==
```

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: cli: `--commando=peerid:rune` (or `-c peerid:rune`) as convenient shortcut for running commando commands.
@rustyrussell rustyrussell added this to the v23.02 milestone Jan 3, 2023
vincenzopalazzo
vincenzopalazzo approved these changes Jan 4, 2023
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 06fead3

@SimonVrouwe
Copy link
Collaborator

If commando is disabled via disable-plugin=commando, will everything still work normally?

@rustyrussell
Copy link
Contributor Author

If commando is disabled via disable-plugin=commando, will everything still work normally?

Yes, except obviously --commando will fail, here's the result. l1 has commando, l2 doesn't. But I wrote two quick tests to be sure:

lightning-cli --commando on l1, aiming at l2:

  • Hangs (as expected!)

lightning-cli --commando on l2:

  • "Unknown command 'commando'", exit code 1.

@rustyrussell rustyrussell merged commit 404e961 into ElementsProject:master Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShahanaFarooqui ShahanaFarooqui ShahanaFarooqui approved these changes

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

Assignees
No one assigned
Labels
json-rpc plugin
Projects
None yet
Milestone
v23.02
Development

Successfully merging this pull request may close these issues.
4 participants
@rustyrussell @SimonVrouwe @vincenzopalazzo @ShahanaFarooqui