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

Make modern string-based JSON ids opt in, deprecate numeric ones #5727

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

rustyrussell
Copy link
Contributor

This is not actually as bad as you'd think; we already handle arbitrary ids, it's just that we need to remember if this request id is a string or not. Most of the code is plumbing, to only set the flag when the plugin says we can.

This fixes clboss!

I get a new rust warning, though, suspect my monkey coding there is wrong:

warning: field is never read: `nonnumericids`
  --> plugins/src/lib.rs:49:5
   |
49 |     nonnumericids: bool,
   |     ^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

…` field.

And document support for it.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugins: `getmanfest` response can contain `nonnumericids` to indicate support for modern string-based JSON request ids.
Changelog-Deprecated: Plugins: numeric JSON request ids: modern ones will be strings (see doc/lightningd-rpc.7.md!)
We also remember whether the id is a string or not, for replacement in
JSON passthrough.

Signed-off-by: Rusty Russell <[email protected]>
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

We can likely drop the entire last commit, since cln-plugin handles the id internally.

ACK a25c5d1

@@ -46,6 +46,7 @@ where
rpcmethods: HashMap<String, RpcMethod<S>>,
subscriptions: HashMap<String, Subscription<S>>,
dynamic: bool,
nonnumericids: bool,
Copy link
Member

Choose a reason for hiding this comment

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

The Rust plugin handles the id completely internally, so this is not needed at all (also why the warning is being returned during compilation).

@cdecker cdecker merged commit ece7784 into ElementsProject:master Nov 21, 2022
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.

2 participants