RPC: Internal named params #17356

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:internal_named_params changing 4 files +65 −23

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

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
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:

bool isPositional = in.params.isObject();
out.m_used_positional_args = isPositional;
if (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

Contributors

Labels

RPC/REST/ZMQ Needs rebase

Linked (view graph)

#11660 RPC: Internal named params #20250 Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely

RPC: Internal named params #17356

Conflicts