RPC: Internal named params

luke-jr commented at 7:36 pm on November 2, 2019: member

Reworked #11660 to add a new m_params to JSONRPCRequest in parallel to the legacy params to ease the transition.

Dropped other unnecessary improvements and “options” handling for now to keep this PR smaller, those can go in later in other PRs.

fanquake added the label RPC/REST/ZMQ on Nov 2, 2019

in src/rpc/net.cpp:577 in 9172f5f233 outdated

542@@ -543,8 +543,9 @@ static UniValue setban(const JSONRPCRequest& request)
543                 },
544     };
545     std::string strCommand;
546-    if (!request.params[1].isNull())
547-        strCommand = request.params[1].get_str();
548+    if (!request.m_params["command"].isNull()) {

promag commented at 10:29 pm on November 3, 2019:

I think you could use a function instead of exposing m_params, like

0std::string strCommand = request.GetString("command");

luke-jr commented at 4:58 pm on November 4, 2019:

I don’t see any benefit to doing that…?

promag commented at 5:03 pm on November 4, 2019:

Less code, supports specifying default value, abstract UniValue.

Just a suggestion though.

luke-jr commented at 5:34 pm on November 4, 2019:

I did think of that when making these changes and opened https://github.com/jgarzik/univalue/pull/63 as a road forward in that regard.

At the end of the day, it is unrelated to this PR though…

promag commented at 10:32 pm on November 3, 2019: member

Concept ACK. Should mention in developer notes that named params is preferable for new code.

luke-jr commented at 5:42 pm on November 4, 2019: member

Well, my hope is that after this is merged, we can migrate existing code to use it and drop params entirely…

luke-jr commented at 9:56 pm on November 4, 2019: member

The echo method (rpc/misc) wants up to 100 args, but only names 10 of them. Should I teach the positional-to-named-args conversion to make ad-hoc argN as needed (currently in branch as a fixup), extend the named arg list out to 100, or reduce the number allowed by echo?

MarcoFalke commented at 10:05 pm on November 4, 2019: member

100 args are only needed to throw an internal bug report. You can replace that with a check that checks if the first argument is the string “throw_internal_bug_report” (or anything else you can come up with)

DrahtBot commented at 11:39 pm on November 4, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18531 (rpc: remove deprecated CRPCCommand constructor by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Jan 22, 2020

luke-jr force-pushed on Mar 17, 2020

DrahtBot removed the label Needs rebase on Mar 17, 2020

sipa commented at 6:28 pm on March 17, 2020: member

Concept ACK

luke-jr commented at 2:40 am on March 18, 2020: member

Alternative fix for rpc_misc pushed after a revert of the “fixup” version

DrahtBot added the label Needs rebase on May 21, 2020

RPC: Change transformNamedArguments to deal with UniValue params objects instead of entire JSONRPCRequests 3088302b07

RPC: Support using named params internally a9b42ba184

RPC/Net: Use internal named params for setban RPC da4653a502

QA/GUI: Give rpcNestedTest parameters names

With internal named parameters, all parameters MUST have names

f47bd9896b

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 6:32 pm on August 20, 2020: member

Rebased. Thanks to changes in master, neither hack is needed anymore.

DrahtBot removed the label Needs rebase on Aug 20, 2020

DrahtBot added the label Needs rebase on Nov 19, 2020

DrahtBot commented at 2:27 pm on November 19, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

luke-jr commented at 11:26 pm on July 6, 2021: member

Doesn’t seem to be much point in rebasing this. It’d be nice to have, but without any review interest, just wasting time.

luke-jr closed this on Jul 6, 2021

jonatack commented at 10:46 pm on September 30, 2021: member

Concept ACK

in src/rpc/server.cpp:460 in f47bd9896b

455+}
456+
457+static inline JSONRPCRequest transformArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames)
458+{
459+    auto out = in;
460+    if (in.params.isObject()) {

yanmaani commented at 10:38 am on October 3, 2021:

Nit: wouldn’t it be slightly cleaner to do something like:

0bool isPositional = in.params.isObject();
1out.m_used_positional_args = isPositional;
2if (isPositional) { ... }

Same loc count, but reads slightly cleaner IMHO.

in src/rpc/server.cpp:466 in f47bd9896b

461+        out.m_used_positional_args = false;
462+        out.m_params = in.params;
463+        out.params = transformNamedArguments(in.params, argNames);
464+    } else {
465+        out.m_used_positional_args = true;
466+        if (in.params.size() > argNames.size()) {

yanmaani commented at 10:42 am on October 3, 2021:

Is there some reason to do this after setting out.m_used_positional_args? Otherwise it seems either cleaner to put it first, or to put it last and take out the return out;.

yanmaani commented at 10:44 am on October 3, 2021: none

Concept ACK. This would make a lot of code much cleaner!

DrahtBot locked this on Oct 30, 2022

RPC: Internal named params #17356

Conflicts