rpc: Allow named and positional arguments to be used together

ryanofsky commented at 3:49 pm on August 18, 2020: contributor

It’s nice to be able to use named options and positional arguments together.

Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python’s approach so as a motivating example:

0bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1

Can be shortened to:

0bitcoin-cli -named createwallet mywallet load_on_startup=1

JSON-RPC standard doesn’t have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused "args" named parameter as a positional parameter array.

This change is backwards compatible. It doesn’t change the interpretation of any previously valid calls, just treats some previously invalid calls as valid.

Another use case even if you only occasionally use named arguments is that you can define an alias:

0alias bcli='bitcoin-cli -named'

And now use both named named and unnamed arguments from the same alias without having to manually add -named option for named arguments or see annoying error “No ‘=’ in named argument… this needs to be present for every argument (even if it is empty)`” for unnamed arguments

jonatack commented at 4:07 pm on August 18, 2020: contributor

Concept ACK. As a frequent cli user, this seems quite handy.

DrahtBot added the label RPC/REST/ZMQ on Aug 18, 2020

DrahtBot added the label Tests on Aug 18, 2020

practicalswift commented at 5:45 pm on August 18, 2020: contributor

Concept ACK: neat!

in src/rpc/client.cpp:283 in f8cd575fe3 outdated

247@@ -248,11 +248,13 @@ UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::s
248 UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<std::string> &strParams)
249 {
250     UniValue params(UniValue::VOBJ);
251+    UniValue positional_args(UniValue::VARR);
252 
253     for (const std::string &s: strParams) {
254         size_t pos = s.find('=');

luke-jr commented at 10:18 pm on August 19, 2020:

Should we support a way to escape the ‘=’ for positional string literals?

ryanofsky commented at 0:37 am on August 21, 2020:

re: #19762 (review)

Should we support a way to escape the ‘=’ for positional string literals?

I’m not sure how useful it would be but I implemented a way to do this in a new commit on top of this PR: b4b155e52b7446d15c957a77d8a34e9e23cb80b0 (branch). Change is backward compatible so it could be added here or be a separate followup PR.

Reason why I don’t think this change is useful is that if find yourself in a situation where you are invoking bitcoin-cli and passing named parameters, but also want to pass an unnamed argument, and then want to escape the argument in case it contains = so the call is more robust, IMO you might as well make it a named parameter so the call is even more robust.

in src/rpc/server.cpp:439 in f8cd575fe3 outdated

434+        out.params = *positional_args->second;
435+        argsIn.erase(positional_args);
436+        for (size_t i = 0; i < named_args.size(); ++i) {
437+            if (i >= out.params.size()) {
438+                out.params.push_back(named_args[i]);
439+            } else if (!named_args[i].isNull()) {

luke-jr commented at 10:24 pm on August 19, 2020:

Where do we overwrite the null with the named arg value??

ryanofsky commented at 0:38 am on August 21, 2020:

re: #19762 (review)

Where do we overwrite the null with the named arg value??

This code isn’t overwriting the previous list of positional parameters, but building a new list from args and parts of the old list.

in test/functional/interface_bitcoin_cli.py:89 in f8cd575fe3 outdated

41@@ -42,6 +42,10 @@ def run_test(self):
42         rpc_response = self.nodes[0].getblockchaininfo()
43         assert_equal(cli_response, rpc_response)
44 
45+        self.log.info("Test named arguments")
46+        assert_equal(self.nodes[0].cli.echo(0, 1, arg3=3, arg5=5), ['0', '1', None, '3', None, '5'])
47+        assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, 1, arg1=1)

luke-jr commented at 10:25 pm on August 19, 2020:

Please also test echo(0, null, 2, arg1=1)

ryanofsky commented at 0:38 am on August 21, 2020:

re: #19762 (review)

Please also test echo(0, null, 2, arg1=1)

Thanks, added this test case, and also changed behavior in this case (now it complains arg1 was specified twice).

in test/functional/rpc_named_arguments.py:33 in f8cd575fe3 outdated

29@@ -30,6 +30,8 @@ def run_test(self):
30         assert_equal(node.echo(arg1=1), [None, 1])
31         assert_equal(node.echo(arg9=None), [None]*10)
32         assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]*2 + [3] + [None]*5 + [9])
33+        assert_equal(node.echo(0, 1, arg3=3, arg5=5), [0, 1, None, 3, None, 5])

luke-jr commented at 10:26 pm on August 19, 2020:

Please also test echo(0, null, 2, arg1=1)

ryanofsky commented at 0:38 am on August 21, 2020:

re: #19762 (review)

Please also test echo(0, null, 2, arg1=1)

Thanks, added this test case, and also changed behavior in this case (now it complains arg1 was specified twice).

luke-jr changes_requested

laanwj commented at 12:54 pm on August 20, 2020: member

Concept ACK on this functionality in the client. I wish it could be client-only, though and would not introduce a non-standard server side convention (and further complicate the RPC server’s argument parsing logic).

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

Seems useful for non-CLI callers too?

DrahtBot commented at 7:55 pm on August 20, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, achow101, aureleoules
Concept ACK	practicalswift, laanwj, promag, meshcollider, MarcoFalke, adam2k
Stale ACK	jonatack

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26506 ([WIP] refactor: rpc: use convenience fn to auto convert non-RFC JSON values by stickies-v)

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.

ryanofsky referenced this in commit b4b155e52b on Aug 21, 2020

ryanofsky force-pushed on Aug 21, 2020

ryanofsky commented at 10:18 pm on August 21, 2020: contributor

Updated f8cd575fe31ae07c4c69653b00b6aff68dbee564 -> f0bb56e8a647972f6c9580b54fe6e280410d05ca (pr/named.1 -> pr/named.2, compare) with a few changes:

Added JSON-RPC documentation and release notes
Added two test cases suggested by Luke
Added stricter server error checking in the two test cases. Instead of allowing the same parameter to be specified twice (in positional and named forms if the positional value is null), the server always throws an error if a parameter is specified twice. This could be changed later, but for now being more strict seems better for preventing mistakes.

luke-jr commented at 1:53 am on August 22, 2020: member

Added stricter server error checking in the two test cases. Instead of allowing the same parameter to be specified twice (in positional and named forms if the positional value is null), the server always throws an error if a parameter is specified twice.

null is supposed to be equivalent to omitted, so I think it should work.

maflcko removed the label RPC/REST/ZMQ on Aug 23, 2020

maflcko removed the label Tests on Aug 23, 2020

DrahtBot added the label Docs on Aug 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 23, 2020

maflcko removed the label Docs on Aug 23, 2020

promag commented at 11:32 pm on August 23, 2020: member

Concept ACK.

promag commented at 9:17 am on August 25, 2020: member

On the cli we could also support passing JSON arguments like query_options.minimumAmount=0.001 query_options.maximumCount=1.

in src/rpc/server.cpp:429 in f0bb56e8a6 outdated

425@@ -425,6 +426,21 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
426         } else {
427             hole += 1;
428         }
429+        if (out.params.size() == 0) skipped += 1;

jonatack commented at 6:00 pm on August 31, 2020:

0        if (out.params.empty()) skipped += 1;

ryanofsky commented at 3:49 pm on October 22, 2020:

re: #19762 (review)

Seems more consistent to keep logic expressed in numbers

in doc/release-notes-19762.md:4 in f0bb56e8a6 outdated

0@@ -0,0 +1,18 @@
1+JSON-RPC
2+---
3+
4+All JSON-RPC methods accept a new [named parameter](JSON-RPC-interface.md)

jonatack commented at 6:06 pm on August 31, 2020:

perhaps link directly to the new section

0All JSON-RPC methods accept a new [named parameter](JSON-RPC-interface.md#parameter-passing)

ryanofsky commented at 3:49 pm on October 22, 2020:

re: #19762 (review)

perhaps link directly to the new section

Thanks, added

in doc/JSON-RPC-interface.md:15 in f0bb56e8a6 outdated

10+The JSON-RPC server supports both _by-position_ and _by-name_ [parameter
11+structures](https://www.jsonrpc.org/specification#parameter_structures)
12+described in the JSON-RPC specification. For extra convenience, to avoid the
13+need to name every parameter value, all RPC methods accept a named parameter
14+called `args`, which can be set to an array of positional values that are
15+combined with named values.

jonatack commented at 6:15 pm on August 31, 2020:

I think it’s important to provide usage examples here. Perhaps more so than in the release notes, which could optionally just link to and refer to this section.

ryanofsky commented at 3:48 pm on October 22, 2020:

re: #19762 (review)

I think it’s important to provide usage examples here. Perhaps more so than in the release notes, which could optionally just link to and refer to this section.

Thanks, added

jonatack commented at 7:09 pm on August 31, 2020: contributor

Tested almost-ACK. The example in the PR description works, but I wasn’t able to make it work for me in some other cases. Perhaps I’m misunderstanding how it works, or the documentation can be improved.

These work:

0$ ./src/bitcoin-cli -named getrawtransaction txid=abc verbose=true
1
2$ ./src/bitcoin-cli -named getrawtransaction abc verbose=true

but

0$ ./src/bitcoin-cli -named getrawtransaction txid=abc true
1error code: -8
2error message:
3Parameter txid specified twice both as positional and named argument
4
5$ ./src/bitcoin-cli -named getrawtransaction txid=abc 1
6error code: -8
7error message:
8Parameter txid specified twice both as positional and named argument

in src/rpc/server.cpp:435 in f0bb56e8a6 outdated

425@@ -425,6 +426,21 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
426         } else {
427             hole += 1;
428         }
429+        if (out.params.size() == 0) skipped += 1;
430+    }
431+    // If leftover "args" param was found, use it as a source of positional
432+    // arguments and add named arguments after.
433+    auto positional_args = argsIn.find("args");

promag commented at 4:35 pm on September 1, 2020:

nit 1: call it _args to have some internal “feeling”?

nit 2: forbid registering arguments with this name.

ryanofsky commented at 4:09 pm on October 22, 2020:

re: #19762 (review)

nit 1: call it _args to have some internal “feeling”?

I don’t think it would be good have RPC methods accepting both “_args” and “args” parameters. Naming was chosen to discourage this (while not disallowing it). Also it isn’t really meant to be internal. It’s supported by two clients in this PR, explained in documentation, and meant to be something that can be used by other clients.

nit 2: forbid registering arguments with this name.

I think it’s fine if RPC methods want to handle positional arguments with their own logic, and don’t think it would be good to add extra code to detect and forbid it a priori.

maflcko commented at 5:07 pm on December 22, 2021:

I think it’s fine if RPC methods want to handle positional arguments with their own logic, and don’t think it would be good to add extra code to detect and forbid it a priori.

It makes this code needlessly complex. If in the future someone wants to add an “args” arg, they can modify the code.

maflcko commented at 5:08 pm on December 22, 2021:

What about popping "args" before the loop to simplify the logic?

ryanofsky commented at 10:40 pm on March 29, 2022:

re: #19762 (review)

It makes this code needlessly complex. If in the future someone wants to add an “args” arg, they can modify the code.

It is true that this decision makes the implementation of the transformNamedArguments function slightly more complex. But it keeps the design as a whole simpler because:

It handles args parameter entirely within transformNamedArguments, and doesn’t require adding special cases for parameters named args in other parts of the code.
It avoids adding a footgun where parameters are treated differently based on their names and choosing a different parameter name could cause errors or munged data.

I also think the transformNamedArguments function would be easier to simplify in the future with some changes to the UniValue class. The current UniValue API basically makes JSON objects append-only instead of read-write, and transformNamedArguments could be simpler and more efficient if this were changed,

ryanofsky commented at 10:41 pm on March 29, 2022:

re: #19762 (review)

What about popping "args" before the loop to simplify the logic?

args is handled after the loop because it could appear in argNamePattern, and we won’t know if it does until after the loop.

in src/rpc/server.cpp:436 in f0bb56e8a6 outdated

425@@ -425,6 +426,21 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
426         } else {
427             hole += 1;
428         }
429+        if (out.params.size() == 0) skipped += 1;
430+    }
431+    // If leftover "args" param was found, use it as a source of positional
432+    // arguments and add named arguments after.
433+    auto positional_args = argsIn.find("args");
434+    if (positional_args != argsIn.end() && positional_args->second->isArray()) {

promag commented at 4:38 pm on September 1, 2020:

nit,

0if (positional_args != argsIn.end()
1    if (positional_args->second->isArray()) {
2        // ...
3    } else {
4        // warn incorrect type
5    }
6}

ryanofsky commented at 4:11 pm on October 22, 2020:

re: #19762 (review)

nit,

I wouldn’t want to change this. It makes logic messier and maybe turns an error into a warning (though I don’t think that was the intent)

ryanofsky force-pushed on Oct 23, 2020

ryanofsky commented at 2:52 pm on October 23, 2020: contributor

Updated f0bb56e8a647972f6c9580b54fe6e280410d05ca -> 894c414dafb8735d6a104dfe30f4c2c57458e65d (pr/named.2 -> pr/named.3, compare) with suggested doc improvments

re: #19762#pullrequestreview-478825831

Tested almost-ACK. The example in the PR description works, but I wasn’t able to make it work for me in some other cases. Perhaps I’m misunderstanding how it works, or the documentation can be improved.

I changed documentation to say “initial positional values” instead of “positional values” (and feel free to suggest other documentation updates that would help) but the examples cited wouldn’t be accepted before or after this PR, and don’t work in python either. The first positional argument is always the first positional argument. The second positional argument is always the second positional argument. You can’t bump a positional argument into a different positions by adding named arguments. If you try, you see an explicit error about conflicting values for the positional argument.

re: #19762 (comment)

On the cli we could also support passing JSON arguments like query_options.minimumAmount=0.001 query_options.maximumCount=1.

Presumably this would be for another PR, but it seems like a nice idea

ryanofsky force-pushed on Oct 23, 2020

ryanofsky commented at 5:30 pm on November 16, 2020: contributor

Looking over recent irc conversation, if you don’t like the createwallet API, you probably won’t like this PR. Even still, this PR should make the createwallet RPC easier to call in a backward compatible way, without getting in the way of future improvements. This PR can also nicely complement client side evaluation features like #20273

jonatack commented at 6:50 pm on November 16, 2020: contributor

Thanks for the comment; reminds me to re-test this PR.

jonatack commented at 11:39 am on November 17, 2020: contributor

Tested ACK 894c414dafb8735d6a104dfe30f4c2c57458e65d

Thanks for the updates. Not sure why I was confused the first time, but this seems much clearer.

in src/rpc/server.cpp:432 in 894c414daf outdated

425@@ -425,6 +426,21 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
426         } else {
427             hole += 1;
428         }
429+        if (out.params.size() == 0) skipped += 1;
430+    }
431+    // If leftover "args" param was found, use it as a source of positional
432+    // arguments and add named arguments after.

jonatack commented at 11:41 am on November 17, 2020:

Outside of this PR changeset, it may not be clear where “args” comes from. It may be good to mention in the comment here that “args” is set in rpc/client.cpp::RPCConvertNamedValues().

ryanofsky commented at 12:05 pm on November 17, 2020:

re: #19762 (review)

Outside of this PR changeset, it may not be clear where “args” comes from. It may be good to mention in the comment here that “args” is set in rpc/client.cpp::RPCConvertNamedValues().

Thanks, if I follow up I’ll reference JSON-RPC-interface.md#parameter-passing here. Better to reference the documentation than the implementation details of one specific client, though. Referencing the RPCConvertNamedValues function where “args” is set in the CLI client would be equivalent to referencing AuthServiceProxy.get_request method where “args” is set in the python client. These would be potentially helpful comments, but also likely to become out of date.

ryanofsky commented at 10:38 pm on March 29, 2022:

re: #19762 (review)

Thanks, if I follow up I’ll reference JSON-RPC-interface.md#parameter-passing here.

Thanks again, added this comment now

jonatack commented at 12:21 pm on November 17, 2020: contributor

This PR would make it much nicer to use the fee_rate sat/vB param in #20305 with, for example, the sendtoaddress, sendmany, and send RPCs. Thumbs up.

ryanofsky commented at 12:36 pm on November 17, 2020: contributor

I forgot to mention earlier the way this could combine with #20273. #20273 adds support for calling RPC methods as function expressions. When it detects () characters used it parses arguments differently, so makes it possible to pass named arguments without having to use -named option and without having to break bitcoin-cli backwards compatibility. So

0bitcoin-cli -named createwallet mywallet load_on_startup=1

could potentially be written as

0bitcoin-cli "createwallet('mywallet', load_on_startup=1)"

or something similar. #20273 may or may not be a good idea, and discussion about it belongs in that PR, but I wanted to clarify how the two PRs might interact.

in src/rpc/client.cpp:285 in 894c414daf outdated

252 
253     for (const std::string &s: strParams) {
254         size_t pos = s.find('=');
255         if (pos == std::string::npos) {
256-            throw(std::runtime_error("No '=' in named argument '"+s+"', this needs to be present for every argument (even if it is empty)"));
257+            positional_args.push_back(rpcCvtTable.convert(strMethod, positional_args.size()) ? ParseNonRFCJSONValue(s) : s);

luke-jr commented at 0:07 am on November 19, 2020:

I think we should probably still error if params has anything in it.

Either that, or if params happened to match the positional args, convert them to positional args.

ryanofsky commented at 12:43 pm on November 19, 2020:

re: #19762 (review)

I think we should probably still error if params has anything in it.

Either that, or if params happened to match the positional args, convert them to positional args.

It’s already an error server side, so implementing this wouldn’t have a practical effect and would complicate this PR. I can see benefits to doing fancier client side stuff, but I’d prefer to keep client changes minimal here, and add client features in a followup PR. If this relates to your other idea of making server validation less restrictive and merging parameters server-side, I also think that should be a followup PR.

In general, nothing in this PR gets in the way of providing merging features you are angling for, but this PR is intentionally only allowing basic (python-style) concatenation as a starting point

luke-jr commented at 9:46 pm on November 19, 2020:

For example, bitcoin-cli getrawtransaction txid blockhash=blah 1 should probably fail, but unless I’m missing something, this will end up with the 1 as verbosity despite it being the 3rd on the command line option

ryanofsky commented at 5:07 pm on December 7, 2020:

re: #19762 (review)

For example, bitcoin-cli getrawtransaction txid blockhash=blah 1 should probably fail, but unless I’m missing something, this will end up with the 1 as verbosity despite it being the 3rd on the command line option

I think you forgot -named in your example, but otherwise this is right and seems good to me. One of the nice things about named arguments is that that they bind unambiguously so you are free to place them anywhere. Only the order of positional arguments matters, so:

0bitcoin-cli -named getrawtransaction txid 1 blockhash=blah
1bitcoin-cli -named getrawtransaction txid blockhash=blah 1
2bitcoin-cli -named getrawtransaction blockhash=blah txid 1

are all equivalent, and safe.

Meanwhile there is strict checking for validity of positional arguments. We always enforce that the first positional argument is the first RPC method argument, and the second positional argument is the second RPC method argument, etc. We explicitly disallow cases like the ones Jon cited #19762#pullrequestreview-478825831:

0bitcoin-cli -named getrawtransaction txid=abc true
1Parameter txid specified twice both as positional and named argument
2
3bitcoin-cli -named getrawtransaction txid=abc 1
4Parameter txid specified twice both as positional and named argument

where someone attempts to used named arguments to bump up positions of positional arguments. These could be allowed, but I think would be more dangerous and not actually useful.

ryanofsky commented at 7:08 pm on July 11, 2021:

re: #19762 (comment)

To make this concrete, lets say you have an RPC with 4 parameters a b c d that you want to pass values a b c d.

This is allowed:

0bitcoin-cli -named RPC a b c=c d=d
1bitcoin-cli -named RPC c=c d=d a b

This is an error:

0bitcoin-cli -named RPC a=a b=b c d
1bitcoin-cli -named RPC a b=b c=c d

The client does not have parameter information so the errors are detected server side. This also means the client is less fragile and the server side error checking is compatible with other clients.

maflcko commented at 4:51 pm on December 22, 2021:

Nice. I had the same question and the “abcd” example clarified that this is safe.

stickies-v commented at 12:40 pm on November 14, 2022:

Is there a particular reason to allow keyword arguments before positional arguments? Even though from the above I understand there is currently no technical ambiguity, I think disallowing it (such as e.g. enforced in Python) makes for a more clear interface. Users cannot be confused about whether or not a positional argument after a keyword argument gets bumped up or not - it is simply not allowed, and the server can trivially make that clear in its response. Additionally, it’ll be easier to become more lenient in the future than to revert this behaviour, so I would be careful about allowing it now if there is no clear benefit.

If you think the current behaviour is warranted, perhaps that should be documented more clearly in JSON-RPC-interface.md?

ryanofsky commented at 4:14 pm on November 14, 2022:

re: #19762 (review)

Is there a particular reason to allow keyword arguments before positional arguments? Even though from the above I understand there is currently no technical ambiguity, I think disallowing it (such as e.g. enforced in Python) makes for a more clear interface. Users cannot be confused about whether or not a positional argument after a keyword argument gets bumped up or not - it is simply not allowed, and the server can trivially make that clear in its response. Additionally, it’ll be easier to become more lenient in the future than to revert this behaviour, so I would be careful about allowing it now if there is no clear benefit.

If you think the current behaviour is warranted, perhaps that should be documented more clearly in JSON-RPC-interface.md?

In the JSON-RPC request, keyword arguments are not really allowed before or after positional arguments because JSON objects are supposed to be “unordered”, so there should not be a concept of before or after. The request object contains a params object, and named arguments are fields of that object. By convention after this PR, the params object can have an extra field called args which has a list of initial positional arguments. I don’t think it would be good to enforce any particular field order in the params JSON object, because it would break potential clients sending requests that have different field orders but are otherwise unambiguous. The RFC even says “implementations whose behavior does not depend on member ordering” are more interoperable, so that is the goal here. It’s why the server doesn’t care about the field order and why JSON-RPC-interface.md is not recommending any field order.

On the client side, it is up to individual clients to choose what syntax to allow and how to generate JSON-RPC requests from that syntax.

In the python RPC client, we’re taking advantage of python function call syntax, and because python syntax only allows named arguments after unnamed ones, that is what our python RPC calls allow.

In the bitcoin-cli client, things are different because command line conventions are not the same as function calling conventions. The most common and user-friendly command line tools tend to not put restrictions on where their named parameters (usually options beginning with - or --) are placed or what order they need to be specified. Even command line tools like git that do restrict where options go usually require options to be specified before positional arguments, not after so I think bitcoin-cli requiring options to be specified last would give it a uniquely awkward and unconventional interface. Personally, I think bitcoin-cli should try to act like other command line tools that are pleasant to use, and allow options to be placed anywhere in any order. There are lots of hurdles we could place in front of command line users to slow them down and catch potential mistakes, but I think we should avoid going out of our way to add a hurdle that doesn’t need to exist and doesn’t make usage more safe or convenient in a specific way.

I think allowing options in any order is just one way this PR can make the command line easier to use. Followup #26485 builds on this and adds support for passing name-only options (similar to https://peps.python.org/pep-3102/). This PR can also combine nicely with #20273 (as described #19762 (comment)) to support the python-style syntax we both like in bitcoin-cli.

stickies-v commented at 2:07 pm on November 15, 2022:

You raise very solid points regarding JSON-RPC, python RPC and bitcoin-cli. Consider me fully convinced that further restrictions on positioning named before positional arguments should not be imposed. I was a bit too Python-biased in my assumptions. Thank you for the clear and comprehensive explanation.

in src/rpc/server.cpp:438 in 894c414daf outdated

425@@ -425,6 +426,21 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
426         } else {
427             hole += 1;
428         }
429+        if (out.params.size() == 0) skipped += 1;
430+    }
431+    // If leftover "args" param was found, use it as a source of positional

achow101 commented at 9:53 pm on July 29, 2021:

It seems to me that it would be clearer to check for args first and then fill in the named parameters afterwards. Overall, I think the logic would be easier to follow. Additionally, I think it would be clearer as to how we detect that an argument was both positional and named. The current method of detecting that is confusing to me.

ryanofsky commented at 10:38 pm on March 29, 2022:

re: #19762 (review)

It seems to me that it would be clearer to check for args first and then fill in the named parameters afterwards. Overall, I think the logic would be easier to follow. Additionally, I think it would be clearer as to how we detect that an argument was both positional and named. The current method of detecting that is confusing to me.

I definitely think transformNamedArguments could be cleaned up in the future, but this suggestion would require a bigger rewrite of the preceding code, while this change leaves that code alone just adding a for-loop that that sets out.params = positional_args + named_args.

I don’t want to change the preceding code because I want to avoid unintended changes in behavior. Also I think the code will be easier to clean up later with some UniValue API improvements (see other comment).

in doc/JSON-RPC-interface.md:24 in 894c414daf outdated

19+```sh
20+# "params": ["mywallet", false, false, "", false, false, true]
21+bitcoin-cli createwallet mywallet false false "" false false true
22+
23+# "params": {"wallet_name": "mywallet", "load_on_startup": true}
24+bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=true

promag commented at 12:58 pm on September 2, 2021:

Eventually we can omit -named.

Sjors commented at 2:29 pm on November 19, 2021:

That would be nice, since I can never remember how to used named arguments :-)

meshcollider commented at 4:41 am on September 28, 2021: contributor

Strong concept ACK, briefly reviewed and looks good but will review more thoroughly soon.

antonilol commented at 8:10 am on December 16, 2021: none

closes https://github.com/bitcoin/bitcoin/issues/23786

in doc/JSON-RPC-interface.md:26 in 894c414daf outdated

21+bitcoin-cli createwallet mywallet false false "" false false true
22+
23+# "params": {"wallet_name": "mywallet", "load_on_startup": true}
24+bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=true
25+
26+# "params": {"args": ["mywallet"], "load_on_startup": true}

maflcko commented at 4:34 pm on December 22, 2021:

does this need any kind of linter to avoid (unlikely) collisions?

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 57e3da0351..ddde65d853 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -508,6 +508,7 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
 5         // Should have unique named arguments
 6         for (const std::string& name : names) {
 7             CHECK_NONFATAL(named_args.insert(name).second);
 8+            CHECK_NONFATAL(name!="args");
 9         }
10         // Default value type should match argument type only when defined
11         if (arg.m_fallback.index() == 2) {
12diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
13index ccb380e25b..10399fc731 100755
14--- a/test/functional/rpc_help.py
15+++ b/test/functional/rpc_help.py
16@@ -77,6 +77,7 @@ class HelpRpcTest(BitcoinTestFramework):
17         all_methods_by_argname = defaultdict(list)
18         converts_by_argname = defaultdict(list)
19         for m in mapping_server:
20+            assert m[2] != "args"
21             all_methods_by_argname[m[2]].append(m[0])
22             converts_by_argname[m[2]].append(m[3])
23

ryanofsky commented at 10:36 pm on March 29, 2022:

re: #19762 (review)

does this need any kind of linter to avoid (unlikely) collisions?

I’d like args to be a convenience for clients that is handled entirely within transformNamedArguments on the server side, not a special case or reserved keyword that needs to be handled in many parts of the code. I think passing an args parameter should be a convention that RPC clients and RPC methods can take advantage of, not something that should dictate how parameters are named.

maflcko commented at 9:58 am on November 30, 2022:

not something that should dictate how parameters are named.

I don’t see a way how this does not dictate that the name can’t be args.

For reference, the functional tests will fail with TypeError: dict() got multiple values for keyword argument 'args' if args as named arg is used along with the args feature added in this pull. And bitcoin-cli will silently eat the args named arg and preserve only the args feature added in this pull.

ryanofsky commented at 5:00 pm on December 2, 2022:

not something that should dictate how parameters are named.

I don’t see a way how this does not dictate that the name can’t be args.

Sorry, I’m talking about the server side, not the client side here. I think it is better if the RPC server framework does not reserve parameter names for itself and does not constrain how RPC individual methods are allowed to name their parameters or use parameter values. If an individual RPC method wants to take a named args parameter and interpret it some different way, or just for forbid positional arguments and require named ones, I think it should be able to do that. The benefit is just not more flexibility for server methods, but having a simpler server framework where an the args param can be entirely handled in transformNamedArguments and doesn’t need to be a special case in other parts of the server code.

For reference, the functional tests will fail with TypeError: dict() got multiple values for keyword argument 'args' if args as named arg is used along with the args feature added in this pull.

This seems like a good thing to me, or I’m not sure what behavior would be better. I described design goals for python and command line RPC clients in this comment: #19762 (review). Basically I just think these clients should be easy to use, and that it’s ok for them to use constrained syntax that doesn’t make it possible to generate every possible JSON request (like requests with duplicate fields or specific field orders), as long as they can generate every useful JSON request.

And bitcoin-cli will silently eat the args named arg and preserve only the args feature added in this pull.

Good catch! This seems like a bug and was definitely not intended. I wrongly assumed UniValue::pushKV function would push an additional parameter called args not silently overwrite the preexisting parameter. It looks like the simplest fix might be replace the new pushKV call with __pushKV to trigger an appropriate error server side. I’ll try to write a fix and test for this.

maflcko approved

maflcko commented at 5:10 pm on December 22, 2021: member

This might be a bit too general and thus complex?

Concept ACK 894c414dafb8735d6a104dfe30f4c2c57458e65d 🐨

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK 894c414dafb8735d6a104dfe30f4c2c57458e65d 🐨
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiwCQv/UtGwzGTYsryNDcEhDxDFgDRDsNud/3VrutzPY0r5tx84JGhMouXv6vMi
 8uxbcJaBXQ5sfRu/AnV70seTttXLTdPDxOkUkrFqLFO/aiJKV50YuVXWz3hookWJc
 9wjl6slBwx5IsXWAD4NT8Wel9bejXJbU8ZkAEHWOv7XQgPbNdXN+65UDu8uGuc1Hf
10RNAfpB7BuDL9g/8q18Ztli1ViKj4MPn3CrtDz2ltosqAGRdCMkj6aN9ThECOZoAz
11R5wFs2Kn9z3Gq+sK2N1M6+cqzfoSMVYticqscwWKHRjj1j+5l4gmScNvCc91IfZW
12VrtKiFEmyxVnuwHquOmIn4A3AvPicnIocaXBEOekml7holdb+vZU9eUoQ4cmBkW9
137vSwyns7xChqdkett059At/mZG2z+QBpXSgVqyFkGfTg9XcSPbfgts/3hXwlETfJ
14RI/sNja6yB7k/erlkTNsdkIKP5P0wOY/vT40wulfywcs2axk0CYPdNeDAucPYBYa
15QWTV89Y4
16=Qi0W
17-----END PGP SIGNATURE-----

uvhw referenced this in commit 47d44ccc3e on Feb 14, 2022

adam2k commented at 8:23 pm on November 1, 2022: none

Concept ACK, but cannot fully test. @ryanofsky I am trying to test out this PR on a Mac, but I’m running into the following issue:

 0$ make
 1Making all in src
 2(CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh '/Users/adam2k/Documents/bitcoin-test/bitcoin/build-aux/missing' autoheader)
 3rm -f src/config/stamp-h1
 4touch src/config/bitcoin-config.h.in
 5cd . && /bin/sh ./config.status src/config/bitcoin-config.h
 6config.status: creating src/config/bitcoin-config.h
 7  CXX      bitcoind-bitcoind.o
 8In file included from bitcoind.cpp:13:
 9In file included from ./init.h:11:
10In file included from ./util/system.h:20:
11./fs.h:14:10: fatal error: 'boost/filesystem.hpp' file not found
12#include <boost/filesystem.hpp>
13         ^~~~~~~~~~~~~~~~~~~~~~
141 error generated.
15make[2]: *** [bitcoind-bitcoind.o] Error 1
16make[1]: *** [all-recursive] Error 1
17make: *** [all-recursive] Error 1

As a sanity check, I can make on master and if I merge master into this branch I can make. Is it possible to rebase this PR? After reading through your comments on #4176 it is probably best to get this PR merged first. I’m willing to help test this out more before continuing to work on #4176.

aureleoules commented at 10:16 am on November 3, 2022: member

Concept ACK

Could -named be enabled by default? An issue I can find with this change is for example creating a wallet with the following command: ./src/bitcoin-cli -named -testnet createwallet 'a=b'

error code: -8 error message: Unknown named parameter a

Instead of creating a wallet named ‘a=b’. I can still create a wallet named ‘a=b’ with this tough: ./src/bitcoin-cli -named -testnet createwallet wallet_name='a=b'

{ “name”: “a=b”, “warning”: "" }

This is an edge case and I don’t think it matters or affect any other RPC argument.

I also suggest applying this diff:

 0diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
 1index 30024ea68..aa63b7115 100644
 2--- a/src/rpc/client.cpp
 3+++ b/src/rpc/client.cpp
 4@@ -269,7 +269,7 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
 5         }
 6     }
 7 
 8-    if (positional_args.size()) {
 9+    if (!positional_args.empty()) {
10         params.pushKV("args", positional_args);
11     }
12 
13diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
14index df86aceeb..7189d18a3 100644
15--- a/src/rpc/server.cpp
16+++ b/src/rpc/server.cpp
17@@ -426,13 +426,13 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
18         } else {
19             hole += 1;
20         }
21-        if (out.params.size() == 0) skipped += 1;
22+        if (out.params.empty()) skipped += 1;
23     }
24     // If leftover "args" param was found, use it as a source of positional
25     // arguments and add named arguments after.
26     auto positional_args = argsIn.find("args");
27     if (positional_args != argsIn.end() && positional_args->second->isArray()) {
28-        if (positional_args->second->size() > skipped && skipped < argNames.size()) {
29+        if (skipped < std::min(argNames.size(), positional_args->second->size())) {
30             throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[skipped] + " specified twice both as positional and named argument");
31         }
32         UniValue named_args = out.params;

@adam2k

Is it possible to rebase this PR?

~~Yes, I suggest you rebase locally the PR in the meantime.~~ Which is what you’ve done :+1:

rpc: Allow named and positional arguments to be used together

It's nice to be able to use named options and positional arguments together.

Most shell tools accept both, and python functions combine options and
arguments allowing them to be passed with even more flexibility. This change
adds support for python's approach so as a motivating example:

    bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1

Can be shortened to:

    bitcoin-cli -named createwallet mywallet load_on_startup=1

JSON-RPC standard doesn't have a convention for passing named and positional
parameters together, so this implementation makes one up and interprets any
unused "args" named parameter as a positional parameter array.

d8b12a75db

ryanofsky referenced this in commit 737c285f69 on Nov 10, 2022

ryanofsky force-pushed on Nov 10, 2022

ryanofsky commented at 6:44 pm on November 10, 2022: contributor

Rebased 894c414dafb8735d6a104dfe30f4c2c57458e65d -> fa15c9b843bbfef8f90bad9bb276753363ff18c8 (pr/named.3 -> pr/named.4, compare) implementing some suggestions.

re: #19762 (comment)

I also suggest applying this diff:

Thanks, applied

An issue I can find with this change is for example creating a wallet with the following command

I think that would happen before this PR, too

Could -named be enabled by default?

It’s a little outside the scope of what this PR is trying to do, but definitely this PR would make it more possible.

achow101 referenced this in commit 7ef730ca84 on Nov 10, 2022

achow101 commented at 8:55 pm on November 10, 2022: member

ACK fa15c9b843bbfef8f90bad9bb276753363ff18c8

sidhujag referenced this in commit 7659a3bf7f on Nov 11, 2022

in src/rpc/server.cpp:413 in fa15c9b843 outdated

402@@ -403,6 +403,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
403     }
404     // Process expected parameters.
405     int hole = 0;
406+    int initial_hole_size = 0;

stickies-v commented at 12:03 pm on November 14, 2022:

Var name not entirely self explanatory, perhaps good to add a brief docstring?

ryanofsky commented at 4:14 pm on November 14, 2022:

re: #19762 (review)

Var name not entirely self explanatory, perhaps good to add a brief docstring?

Yeah I think I’ve changed the variable name a few times trying to make it clearer. I added a comment before the loop now explaining what the variables mean and how it works.

in src/rpc/server.cpp:435 in fa15c9b843 outdated

430     }
431+    // If leftover "args" param was found, use it as a source of positional
432+    // arguments and add named arguments after. This is a convenience for
433+    // clients that want to pass a combination of named and positional
434+    // arguments, described in doc/JSON-RPC-interface.md#parameter-passing
435+    auto positional_args = argsIn.find("args");

stickies-v commented at 2:10 pm on November 14, 2022:

could use extract here too, to avoid the need for erase later on?

ryanofsky commented at 4:14 pm on November 14, 2022:

re: #19762 (review)

could use extract here too, to avoid the need for erase later on?

This is great! Updated now, and this is the first time I’ve used extract.

stickies-v commented at 2:51 pm on November 14, 2022: contributor

Concept ACK. Improves the current quite clunky behaviour of using named arguments.

Nit: a bunch of copy initialization instead of list initialization

ryanofsky force-pushed on Nov 14, 2022

ryanofsky commented at 6:22 pm on November 14, 2022: contributor

Thanks for the reviews!

Updated fa15c9b843bbfef8f90bad9bb276753363ff18c8 -> 1ef90f8b522f3efb9512c1675fb336033df65c25 (pr/named.4 -> pr/named.5, compare) with suggestions

ryanofsky force-pushed on Nov 14, 2022

ryanofsky commented at 9:33 pm on November 14, 2022: contributor

Updated 1ef90f8b522f3efb9512c1675fb336033df65c25 -> ce881493438f2cafb4d248a04eef2ba806d196bc (pr/named.5 -> pr/named.6, compare) dropping std::move that has no effect because the input request object is const, caught by linter https://cirrus-ci.com/task/4757419577311232?logs=ci#L4542

aureleoules approved

aureleoules commented at 2:35 pm on November 15, 2022: member

ACK ce881493438f2cafb4d248a04eef2ba806d196bc

in src/rpc/server.cpp:452 in ce88149343 outdated

445+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[initial_hole_size] + " specified twice both as positional and named argument");
446+        }
447+        UniValue named_args = std::move(out.params);
448+        out.params = *positional_args.mapped();
449+        for (size_t i = out.params.size(); i < named_args.size(); ++i) {
450+            out.params.push_back(named_args[i]);

stickies-v commented at 5:46 pm on November 15, 2022:

nit: I think this block could benefit from a brief docstring that explains that we’re just overwriting the first out.params elements with the values in positional_args. Unfortunately out.params being a UniValue makes this quite verbose for a pretty simple operation, but I don’t see a way around that (since the [] operator returns a const ref, probably for validation/internal consistency reasons).

ryanofsky commented at 8:13 pm on November 15, 2022:

re: #19762 (review)

nit: I think this block could benefit from a brief docstring that explains that we’re just overwriting the first out.params elements with the values in positional_args

Thanks, added

Unfortunately out.params being a UniValue makes this quite verbose for a pretty simple operation, but I don’t see a way around that (since the [] operator returns a const ref, probably for validation/internal consistency reasons).

I think there’s probably no good reason for this and this whole method could be a lot simpler if UniValue objects were mutable rather than append-only. Probably Univalue not providing mutable methods is just an artifact of most of our other JSON code only needing to append data.

in src/rpc/server.cpp:444 in ce88149343 outdated

439+    // arguments and add named arguments after. This is a convenience for
440+    // clients that want to pass a combination of named and positional
441+    // arguments, described in doc/JSON-RPC-interface.md#parameter-passing
442+    auto positional_args = argsIn.extract("args");
443+    if (positional_args && positional_args.mapped()->isArray()) {
444+        if (initial_hole_size < std::min<int>(positional_args.mapped()->size(), argNames.size())) {

stickies-v commented at 6:11 pm on November 15, 2022:

The std::min confuses me. If positional_args.mapped()->size() > argNames.size(), isn’t that a different issue than parameters being specified both as positional and named? If we’re specifying more positional arguments than the method supports, it doesn’t really matter which other named arguments we’ve provided - it’s gonna be an invalid call either way? Additionally, this could be triggered without providing any (non-“arg”) named parameters at all, resulting in a misleading error message? If my reasoning is correct, maybe best to split that up into two cases/checks?

ryanofsky commented at 7:51 pm on November 15, 2022:

re: #19762 (review)

The std::min confuses me. If positional_args.mapped()->size() > argNames.size(), isn’t that a different issue than parameters being specified both as positional and named? If we’re specifying more positional arguments than the method supports, it doesn’t really matter which other named arguments we’ve provided - it’s gonna be an invalid call either way? Additionally, this could be triggered without providing any (non-“arg”) named parameters at all, resulting in a misleading error message? If my reasoning is correct, maybe best to split that up into two cases/checks?

This line of code is only trying to check for a conflict between a named argument and a positional argument. If more named arguments were specified than exist that triggers a different error (“Unknown named parameter” below). And if more positional arguments were specified than exist that triggers an error showing the full method help text (when IsValidNumArgs is called later). I think it is good to trigger that same error whenever too many positional arguments are provided, regardless of how they are provided.

The std::min usage was suggested by aureleoules here #19762 (comment), and I accepted the suggestion because it was equivalent to previous code, but I think previous code was less confusing so I reverted it now.

stickies-v commented at 12:26 pm on November 16, 2022:

Your explanation makes sense, thanks. I erroneously understood this line to check for positional_args being smaller than argNames - whereas actually it checks for initial_hole_size being smaller than argNames. Since initial_hole_size can never be strictly larger than argNames, I suppose this is just to check if they’re equal in size, which would indicate that not a single named argument was provided (and that argNames[initial_hole_size] can’t be out of bounds)? In which case, I think it could be helpful to store that in an aptly named boolean just for readability because it’s not immediately obvious I think, e.g.

0bool has_named_arguments{initial_hole_size < (int)argNames.size()};
1if (initial_hole_size < (int)positional_args.mapped()->size() && has_named_arguments) {

Other than that, I think the error handling of providing too many positional arguments is exactly how it should be as you described, so you can consider this resolved. 👍

ryanofsky commented at 4:29 pm on November 16, 2022:

re: #19762 (review)

I think it could be helpful to store that in an aptly named boolean just for readability

Thanks. Applied your suggestion. I also added unit tests to check for the intended behavior and make sure this doesn’t get broken unintentionally in the future.

stickies-v commented at 6:18 pm on November 15, 2022: contributor

Approach ACK ce8814934

ryanofsky force-pushed on Nov 15, 2022

ryanofsky commented at 9:12 pm on November 15, 2022: contributor

Thanks for the review!

Updated ce881493438f2cafb4d248a04eef2ba806d196bc -> c31e1635558806af567f454fc13a6180d755c83b (pr/named.6 -> pr/named.7, compare) reverting a previous suggestion that added std::min and adding a comment

aureleoules approved

aureleoules commented at 12:29 pm on November 16, 2022: member

reACK c31e1635558806af567f454fc13a6180d755c83b

stickies-v approved

stickies-v commented at 12:54 pm on November 16, 2022: contributor

ACK c31e16355

Left a small readability suggestion that I think should be quick to add, but nothing blocking. Very nice usability improvement. bitcoin-cli users worldwide, rejoice!

achow101 commented at 4:16 pm on November 16, 2022: member

ACK c31e1635558806af567f454fc13a6180d755c83b

ryanofsky force-pushed on Nov 16, 2022

ryanofsky commented at 4:31 pm on November 16, 2022: contributor

Thanks for the reviews!

Updated c31e1635558806af567f454fc13a6180d755c83b -> d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf (pr/named.7 -> pr/named.8, compare) applying readability suggestion and using more brace initialization. Also added unit tests to make sure intended behavior isn’t accidentally broken later.

in src/test/rpc_tests.cpp:87 in d8b12a75db

81@@ -45,6 +82,29 @@ UniValue RPCTestingSetup::CallRPC(std::string args)
82 
83 BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup)
84 
85+BOOST_AUTO_TEST_CASE(rpc_namedparams)
86+{
87+    const std::vector<std::string> arg_names{{"arg1", "arg2", "arg3", "arg4", "arg5"}};

stickies-v commented at 6:48 pm on November 17, 2022:

nit

0    const std::vector<std::string> arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"};

ryanofsky commented at 5:07 pm on December 2, 2022:

re: #19762 (review)

nit

Thanks, will incorporate this in followup #26485

stickies-v approved

stickies-v commented at 7:37 pm on November 17, 2022: contributor

re-ACK d8b12a75d

Latest change only introduces brace initialization, minor doc and naming updates, and additional unit tests.

achow101 commented at 7:18 pm on November 22, 2022: member

ACK d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf

aureleoules approved

aureleoules commented at 10:18 am on November 27, 2022: member

re-ACK d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf

achow101 merged this on Nov 29, 2022

achow101 closed this on Nov 29, 2022

in src/test/rpc_tests.cpp:103 in d8b12a75db

 98+
 99+    // Make sure an overlap between a named argument and positional argument raises an exception
100+    BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1,2,3], "arg4": 4, "arg2": 2})"), arg_names), UniValue,
101+                          HasJSON(R"({"code":-8,"message":"Parameter arg2 specified twice both as positional and named argument"})"));
102+
103+    // Make sure extra positional arguments can be passed through to the method implemenation, as long as they don't overlap with named arguments.

maflcko commented at 1:15 pm on November 30, 2022:

implemenation-> implementation

ryanofsky commented at 5:08 pm on December 2, 2022:

re: #19762 (review)

implemenation-> implementation

Thanks, will incorporate this in followup #26485

in src/rpc/server.cpp:452 in d8b12a75db

447+        }
448+        // Assign positional_args to out.params and append named_args after.
449+        UniValue named_args{std::move(out.params)};
450+        out.params = *positional_args.mapped();
451+        for (size_t i{out.params.size()}; i < named_args.size(); ++i) {
452+            out.params.push_back(named_args[i]);

maflcko commented at 1:16 pm on November 30, 2022:

Could std::move?

ryanofsky commented at 5:06 pm on December 2, 2022:

re: #19762 (review)

Could std::move?

This also doesn’t work because UniValue doesn’t currently provide a non-const operator[]. I do think this would be good to improve in the future.

in src/rpc/server.cpp:450 in d8b12a75db

445+        if (initial_hole_size < (int)positional_args.mapped()->size() && has_named_arguments) {
446+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[initial_hole_size] + " specified twice both as positional and named argument");
447+        }
448+        // Assign positional_args to out.params and append named_args after.
449+        UniValue named_args{std::move(out.params)};
450+        out.params = *positional_args.mapped();

maflcko commented at 1:16 pm on November 30, 2022:

Could std::move?

ryanofsky commented at 5:05 pm on December 2, 2022:

re: #19762 (review)

Could std::move?

Unfortunately it doesn’t work because argsIn is a map to const UniValue*, because input params are const. I do think it would be good to add give UniValue objects non-const accessors, so they could be modified in place without this copying. Now that this code has unit test coverage, it should be easier to improve in the future without fear of introducing bugs.

maflcko commented at 1:16 pm on November 30, 2022: member

3 nits

sidhujag referenced this in commit d179b002dd on Dec 1, 2022

ryanofsky referenced this in commit 891952fb49 on Dec 2, 2022

ryanofsky referenced this in commit 763d3947d3 on Dec 2, 2022

ryanofsky referenced this in commit e2c3b18e67 on Dec 9, 2022

ryanofsky referenced this in commit d1ca563825 on Dec 9, 2022

ryanofsky referenced this in commit 8c3ff7d52a on Dec 9, 2022

janus referenced this in commit ccfc29d951 on Jan 20, 2023

janus referenced this in commit 6fdce0a339 on Jan 20, 2023

janus referenced this in commit 86ae49b974 on Jan 20, 2023

kwvg referenced this in commit e88969e3ea on Jul 18, 2023

kwvg referenced this in commit a17e57bff7 on Jul 19, 2023

kwvg referenced this in commit 6f3799b859 on Jul 19, 2023

kwvg referenced this in commit 7c2eb367a8 on Jul 19, 2023

kwvg referenced this in commit 6826deade3 on Jul 23, 2023

kwvg referenced this in commit 470924a247 on Jul 23, 2023

kwvg referenced this in commit c88d9241e9 on Jul 24, 2023

kwvg referenced this in commit 2b189efd20 on Jul 24, 2023

kwvg referenced this in commit 8316f974a0 on Jul 24, 2023

kwvg referenced this in commit 6ff956e80f on Jul 25, 2023

kwvg referenced this in commit 75ef114da7 on Jul 25, 2023

kwvg referenced this in commit f40a5417a5 on Jul 25, 2023

kwvg referenced this in commit 5e7086f50a on Jul 25, 2023

kwvg referenced this in commit eb400a7a9a on Jul 25, 2023

kwvg referenced this in commit ca729a0ea3 on Jul 25, 2023

UdjinM6 referenced this in commit 2bca58f82b on Jul 27, 2023

kwvg referenced this in commit bdd1b74f89 on Jul 27, 2023

PastaPastaPasta referenced this in commit d40f28edb4 on Jul 28, 2023

PastaPastaPasta referenced this in commit d9a0b32cbf on Jul 28, 2023

bitcoin locked this on Dec 2, 2023

rpc: Allow named and positional arguments to be used together #19762

Reviews

Conflicts