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

Plugins can deprecate too! #3883

Conversation

rustyrussell
Copy link
Contributor

I wrote all this then realized that the cmd I wanted to patch wasn't in a plugin at all!

Oh well!

@rustyrussell rustyrussell added this to the Next Release milestone Jul 28, 2020
lightningd/plugin.c Outdated Show resolved Hide resolved
tests/plugins/test_libplugin.c Show resolved Hide resolved
Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Nice, this plays well with the legacy "nothing's deprecated"

doc/PLUGINS.md Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/plugin.py Show resolved Hide resolved
@rustyrussell rustyrussell force-pushed the guilt/plugins-can-deprecate-too branch from 3283abb to 08bb87a Compare July 28, 2020 21:06
doc/PLUGINS.md Outdated Show resolved Hide resolved
doc/PLUGINS.md Outdated Show resolved Hide resolved
It's --allow-deprecated-apis=false.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell force-pushed the guilt/plugins-can-deprecate-too branch from 08bb87a to 5735673 Compare August 3, 2020 02:47
@rustyrussell
Copy link
Contributor Author

Trivial rebase, with @ZmnSCPxj 's wording improvement (which incorporated @niftynei typo fix as a side-effect).

Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

Looks fine, but some style/grammar nits.

lightningd/jsonrpc.c Outdated Show resolved Hide resolved
plugins/libplugin.c Outdated Show resolved Hide resolved
doc/PLUGINS.md Outdated Show resolved Hide resolved
@rustyrussell rustyrussell force-pushed the guilt/plugins-can-deprecate-too branch from 5735673 to 98ff0e3 Compare August 4, 2020 20:44
@rustyrussell
Copy link
Contributor Author

Minor fixes, here's the diff:

diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md
index 6b3fdc617..e03b13894 100644
--- a/doc/PLUGINS.md
+++ b/doc/PLUGINS.md
@@ -60,8 +60,8 @@ interface.
 The `getmanifest` method is required for all plugins and will be
 called on startup with optional parameters (in particular, it may have
 `allow-deprecated-apis: false`, but you should accept, and ignore,
-other parameters your plugin does not expect others).  It MUST return
-a JSON object similar to this example:
+other parameters).  It MUST return a JSON object similar to this
+example:
 
 ```json
 {
diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
index 50c47e0ad..cdb75aea8 100644
--- a/lightningd/jsonrpc.c
+++ b/lightningd/jsonrpc.c
@@ -390,13 +390,13 @@ static struct command_result *json_help(struct command *cmd,
 		if (!one_cmd)
 			return command_fail(cmd, JSONRPC2_METHOD_NOT_FOUND,
 					    "Unknown command '%.*s'",
-					    cmdtok->end - cmdtok->start,
-					    buffer + cmdtok->start);
+					    json_tok_full_len(cmdtok),
+					    json_tok_full(buffer, cmdtok));
 		if (!deprecated_apis && one_cmd->deprecated)
 			return command_fail(cmd, JSONRPC2_METHOD_NOT_FOUND,
 					    "Deprecated command '%.*s'",
-					    cmdtok->end - cmdtok->start,
-					    buffer + cmdtok->start);
+					    json_tok_full_len(cmdtok),
+					    json_tok_full(buffer, cmdtok));
 	} else
 		one_cmd = NULL;
 
@@ -850,8 +850,8 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 	if (c->json_cmd->deprecated && !deprecated_apis) {
 		return command_fail(c, JSONRPC2_METHOD_NOT_FOUND,
 				    "Command '%.*s' is deprecated",
-				    method->end - method->start,
-				    jcon->buffer + method->start);
+				    json_tok_full_len(method),
+				    json_tok_full(jcon->buffer, method));
 	}
 
 	rpc_hook = tal(c, struct rpc_command_hook_payload);
diff --git a/plugins/libplugin.c b/plugins/libplugin.c
index 2d4e009a0..d0cb66607 100644
--- a/plugins/libplugin.c
+++ b/plugins/libplugin.c
@@ -580,8 +580,12 @@ handle_getmanifest(struct command *getmanifest_cmd,
 	dep = json_get_member(buf, getmanifest_params, "allow-deprecated-apis");
 	if (!dep)
 		deprecated_apis = true;
-	else
-		json_to_bool(buf, dep, &deprecated_apis);
+	else {
+		if (!json_to_bool(buf, dep, &deprecated_apis))
+			plugin_err(p, "Invalid allow-deprecated-apis '%.*s'",
+				   json_tok_full_len(dep),
+				   json_tok_full(buf, dep));
+	}
 
 	json_array_start(params, "options");
 	for (size_t i = 0; i < tal_count(p->opts); i++) {

rustyrussell and others added 8 commits August 6, 2020 09:57
If allow-deprecated-apis=false, don't mention them at all (we already
disallow calling them in that case).  Otherwise, note that they're
deprecated in the help msg.

Signed-off-by: Rusty Russell <[email protected]>
This allows plugins to choose how to present things in getmanifest.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: plugins: `getmanifest` may now include "allow-deprecated-apis" boolean flag.
Changelog-Deprecated: plugins: `getmanifest` without any parameters; plugins should accept any parameters for future use.
We currently do it by calling listconfigs, but that may be too late.

Signed-off-by: Rusty Russell <[email protected]>
…ecated.

This lets us handle it the same way we handle builtin commands, and also
lets us warn if they use deprecated apis and allow-deprecated-apis=false.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugins: can now mark their options and commands deprecated.
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: pyln-client: commands and options can now mark themselves deprecated.
Suggested-by: Rusty Russell <@rustyrussell>
Changelog-Deprecated: JSON-RPC: `listsendpays` will no longer add `null` if we don't know the `amount_msat` for a payment.
@rustyrussell rustyrussell force-pushed the guilt/plugins-can-deprecate-too branch from 98ff0e3 to bd61cd5 Compare August 6, 2020 00:38
@rustyrussell
Copy link
Contributor Author

Trivial rebase (quote changed broke string match in tests)...

@rustyrussell
Copy link
Contributor Author

Ack bd61cd5

@rustyrussell rustyrussell merged commit 639eaaf into ElementsProject:master Aug 10, 2020
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