-
Notifications
You must be signed in to change notification settings - Fork 36.3k
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
rpc: Add support for JSON-RPC named arguments #8811
Conversation
8f66e77
to
a6b4276
Compare
This is useful because a lot of features add new arguments to an existing call. For example #8751. Currently the user of the API has to explicitly specify defaults for the intermediate arguments. With named arguments this would no longer be necessary. It could also help clean up the API for calls that already have a lot of optional arguments such as |
AuthServiceProxy.__id_count += 1 | ||
|
||
log.debug("-%s-> %s %s"%(AuthServiceProxy.__id_count, self._service_name, | ||
json.dumps(args, default=EncodeDecimal, ensure_ascii=self.ensure_ascii))) | ||
if args and argsn: | ||
raise ValueError('Cannot handle both named and positional arguments') | ||
postdata = json.dumps({'version': '1.1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the OP you are linking to the "JSON-RPC 2.0 Specification", which states
jsonrpc
A String specifying the version of the JSON-RPC protocol. MUST be exactly "2.0".
Can you explain why you keep this at 1.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May make sense to change it to that (even irrespective of this pull) however I'm not sure that we support the entire 2.0 standard, so that may involve other changes too.
The only alternative I see would be to only accept one object as first positional argument for all methods, but this will likely never happen, so Concept ACK. |
Concept ACK. I think this is very useful. After this, we should update the documents (and RPC help examples) to slowly move away from recommending fixed position arguments. |
a6b4276
to
b71acdb
Compare
Rebased to get rid of the univalue change |
b71acdb
to
e4aa9b3
Compare
Needed rebase, rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept ACK, and aside from the error mentioned, looks like good minimal support
for (const std::string &argName: argNames) { | ||
auto fr = argsIn.find(argName); | ||
if (fr != argsIn.end()) { | ||
for (int i = 0; i < hole; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after this loop hole
should be reset to 0, otherwise holes will be repeated for every subsequent entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch, thanks!
// Process expected parameters. | ||
int hole = 0; | ||
for (const std::string &argName: argNames) { | ||
auto fr = argsIn.find(argName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does fr stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find returnvalue? Just a guess.
node.getblock(hash=h) | ||
|
||
assert_equal(node.echo(), []) | ||
assert_equal(node.echo(arg0=0,arg9=9), [0] + [None]*8 + [9]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to add test of [exist, hole, exist, exist] to catch gap repeat error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the test:
assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]_2 + [3] + [None]_5 + [9])
Verified that with the code as is it fails, but with instagibbs fix of hole=0 it succeeds.
Concept ACK, needs rebase |
e4aa9b3
to
3c7647e
Compare
Rebased, added @instagibbs' test and fix. |
ACK |
Concept ACK! Do you intend to include the echo command in the final merge? |
utACK 3c7647e |
3c7647e
to
376bec8
Compare
As there is interest in this I'm continuing work on the issues that are still open in the OP. |
376bec8
to
76973f4
Compare
Added support to bitcoin-cli. This should complete the feature set for this pull. I'd appreciate testing and review. Especially important to review are the parameter names in the RPC tables, as they will become part of the API:
More examples
Note right now,
It doesn't know how to handle the univalue null for the account parameter. It should interpret it as the default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, and code generally looks good. I added a few nits, most of which have actually already been addressed in subsequent commits.
At a high level, I'm worried about introducing the "txid" param name for a bunch of calls that will likely need to make the distinction between txid/wtxid soon. Wallet especially. Maybe some convention like "txhash" should be preferred instead?
{ "util", "signmessagewithprivkey", &signmessagewithprivkey, true }, | ||
{ "control", "getinfo", &getinfo, true, {} }, /* uses wallet if enabled */ | ||
{ "control", "getmemoryinfo", &getmemoryinfo, true, {} }, | ||
{ "util", "validateaddress", &validateaddress, true, {"bitcoinaddress"} }, /* uses wallet if enabled */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referred to as "address" in the help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address/privkey/pubkey usage is made consistent in a later commit:
8b6fd17
{ "control", "getmemoryinfo", &getmemoryinfo, true, {} }, | ||
{ "util", "validateaddress", &validateaddress, true, {"bitcoinaddress"} }, /* uses wallet if enabled */ | ||
{ "util", "createmultisig", &createmultisig, true, {"nrequired","keys"} }, | ||
{ "util", "verifymessage", &verifymessage, true, {"bitcoinaddress","signature","message"} }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
{ "network", "getconnectioncount", &getconnectioncount, true, {} }, | ||
{ "network", "ping", &ping, true, {} }, | ||
{ "network", "getpeerinfo", &getpeerinfo, true, {} }, | ||
{ "network", "addnode", &addnode, true, {"node","command"} }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe these should be "address" rather than "node", so that in the future we can differentiate? (nodeid, subnet, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'address' is already used for bitcoin addresses, so I'd prefer not overloading that for network addresses. Other suggestions are welcome, though
{ "network", "getaddednodeinfo", &getaddednodeinfo, true, {"node"} }, | ||
{ "network", "getnettotals", &getnettotals, true, {} }, | ||
{ "network", "getnetworkinfo", &getnetworkinfo, true, {} }, | ||
{ "network", "setban", &setban, true, {"ip", "command", "bantime", "absolute"} }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably better to call this subnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
|
||
{ "generating", "generate", &generate, true, {"numblocks","maxtries"} }, | ||
{ "generating", "generatetoaddress", &generatetoaddress, true, {"numblocks","address","maxtries"} }, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unify nblocks/numblocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
throw(std::runtime_error("No '=' in named argument, a value needs to be provided for every argument")); | ||
} | ||
std::string name = s.substr(ptr, pos-ptr); | ||
std::string value = s.substr(pos+1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably make sure that !name.empty() && !value.empty()
for good measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it should do the expected thing: send an empty name or value.
An empty value is perfectly acceptable if you want to send an empty string.
An empty name is a bit strange, agreed, but it gets caught server-side as an unknown argument.
if (haveNamed && havePositional) { | ||
throw runtime_error("cannot have both named and positional arguments, did you mean to pass arguments before the command?"); | ||
} else if(haveNamed) { | ||
params = RPCConvertNamedValues(strMethod, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing args here with GetBoolArg/GetArg is a little scary. If some command took a (for example) "-version=foo" argument, I believe it would be swallowed by line 81 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseParameters stops at the first argument that doesn't start with -
, which happens to be the command. This starts after the command. So unless I miss something they will never interfere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's the part I was missing, thanks. Maybe a comment to that effect to remind me next time I have to ask? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment :)
Why is there a "wtxid" ever needed as a param? "txhash" and "txid" are all I see existing at all (hash != txid). |
I think a lot of the options params ought to be flattened? |
Possibly. Not in this pull, though. JSON can perfectly deal with nested structures. Also this would break backwards compatibility, unlike anything in this pull. |
c9d3a54
to
65bf5d2
Compare
Rebased for minor argument documentation conflict. |
utACK 65bf5d27c578e7560128a28af08fef57135ec841. I have not read through the test code in detail, nor verified whether the RPC help names match the named arguments in the server and client tables. I have also not checked the consistency of the chosen names (but I don't care strongly). I have played around briefly with the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't bother to audit that help text matches the server and client tables, but glanced at a few and they look good. None of the comments really matter all that much.
utACK 65bf5d27c578e7560128a28af08fef57135ec841
const std::vector<UniValue>& values = in.params.getValues(); | ||
std::unordered_map<std::string, const UniValue*> argsIn; | ||
for (size_t i=0; i<keys.size(); ++i) { | ||
argsIn[keys[i]] = &values[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because univalue does not check for duplicate keys, things are a bit weird. I'm not sure if there's much that this PR could/should do about this, but as long as we're adding more user-provided-json-parsing stuff I thought I'd mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost no JSON implementation checks for duplicate keys. The usual behavior is that the last value assigned to the key takes precedence. I think that will happen here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe the last one will take precedence. I'm ok with not fixing this, but it seems super strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if it's any guide, python's JSON behaves exactly the same:
>>> json.loads('{"a":"b","a":"C"}')
{'a': 'C'}
What I would not mind is checking at the client side for duplicate arguments, as a user friendly feature, but I'm not going to do it in this pull.
@@ -279,7 +279,7 @@ UniValue waitforblockheight(const JSONRPCRequest& request) | |||
"of the current tip.\n" | |||
"\nReturns the current block on timeout or exit.\n" | |||
"\nArguments:\n" | |||
"1. block height to wait for (int)\n" | |||
"1. height (required, int) Block height to wait for (int)\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs here still say "blockheight" for one-line help but height for multi-line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will make that consistent.
std::string name = s.substr(0, pos); | ||
std::string value = s.substr(pos+1); | ||
|
||
// allow include-mempool=true as well as include_mempool=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was useful when the syntax was -arg=value. I guess it no longer really is now that plain name=value pairs are used.
Usage e.g.: $ src/bitcoin-cli -testnet -named echo arg0="dfdf" [ "dfdf" ] Argument conversion also works, for arguments thus flagged in the table in `src/rpc/client.cpp`. $ src/bitcoin-cli -testnet -named echojson arg0="[1,2,3]" [ [ 1, 2, 3 ] ] Unknown parameter (detected server-side): $ src/bitcoin-cli -testnet -named getinfo arg0="dfdf" error code: -8 error message: Unknown named parameter arg0
65bf5d2
to
4e7e2e1
Compare
Fixed @bluematt's documentation nit and removed the |
4e7e2e1 Update RPC argument names (John Newbery) 481f289 rpc: Named argument support for bitcoin-cli (Wladimir J. van der Laan) 9adb4e1 rpc: Argument name consistency (Wladimir J. van der Laan) 8d713f7 rpc: Named arguments for rawtransaction calls (Wladimir J. van der Laan) 37a166f rpc: Named arguments for wallet calls (Wladimir J. van der Laan) 78b684f rpc: Named arguments for mining calls (Wladimir J. van der Laan) b8ebc59 rpc: Named arguments for net calls (Wladimir J. van der Laan) 2ca9dcd test: Add test for RPC named arguments (Wladimir J. van der Laan) fba1a61 rpc: Named arguments for misc calls (Wladimir J. van der Laan) 286ec08 rpc: Add 'echo' call for testing (Wladimir J. van der Laan) 495eb44 rpc: Named arguments for blockchain calls (Wladimir J. van der Laan) 6f1c76a rpc: Support named arguments (Wladimir J. van der Laan) 5865d41 authproxy: Add support for RPC named arguments (Wladimir J. van der Laan)
Concept ACK (I'm surprised I didn't do it before). |
This commit makes a few changes to the 'blockheight' parameter: - `getblockhash` can now take a negative height to indicate block depth counting back from the chain tip. - `getblock` and `getblockheader` can now take a height instead of a blockhash to indicate which block is requested. Height can be negative to indicate depth from the chain tip. Only one out of hash and height should be specified. - `getblock` and `getblockheader` can now be called using positional arguments (following bitcoin#8811).
This commit makes a few changes to the 'blockheight' parameter: - `getblockhash` can now take a negative height to indicate block depth counting back from the chain tip. - `getblock` and `getblockheader` can now take a height instead of a blockhash to indicate which block is requested. Height can be negative to indicate depth from the chain tip. Only one out of hash and height should be specified. - `getblock` and `getblockheader` can now be called using positional arguments (following bitcoin#8811).
4e7e2e1 Update RPC argument names (John Newbery) 481f289 rpc: Named argument support for bitcoin-cli (Wladimir J. van der Laan) 9adb4e1 rpc: Argument name consistency (Wladimir J. van der Laan) 8d713f7 rpc: Named arguments for rawtransaction calls (Wladimir J. van der Laan) 37a166f rpc: Named arguments for wallet calls (Wladimir J. van der Laan) 78b684f rpc: Named arguments for mining calls (Wladimir J. van der Laan) b8ebc59 rpc: Named arguments for net calls (Wladimir J. van der Laan) 2ca9dcd test: Add test for RPC named arguments (Wladimir J. van der Laan) fba1a61 rpc: Named arguments for misc calls (Wladimir J. van der Laan) 286ec08 rpc: Add 'echo' call for testing (Wladimir J. van der Laan) 495eb44 rpc: Named arguments for blockchain calls (Wladimir J. van der Laan) 6f1c76a rpc: Support named arguments (Wladimir J. van der Laan) 5865d41 authproxy: Add support for RPC named arguments (Wladimir J. van der Laan)
4e7e2e1 Update RPC argument names (John Newbery) 481f289 rpc: Named argument support for bitcoin-cli (Wladimir J. van der Laan) 9adb4e1 rpc: Argument name consistency (Wladimir J. van der Laan) 8d713f7 rpc: Named arguments for rawtransaction calls (Wladimir J. van der Laan) 37a166f rpc: Named arguments for wallet calls (Wladimir J. van der Laan) 78b684f rpc: Named arguments for mining calls (Wladimir J. van der Laan) b8ebc59 rpc: Named arguments for net calls (Wladimir J. van der Laan) 2ca9dcd test: Add test for RPC named arguments (Wladimir J. van der Laan) fba1a61 rpc: Named arguments for misc calls (Wladimir J. van der Laan) 286ec08 rpc: Add 'echo' call for testing (Wladimir J. van der Laan) 495eb44 rpc: Named arguments for blockchain calls (Wladimir J. van der Laan) 6f1c76a rpc: Support named arguments (Wladimir J. van der Laan) 5865d41 authproxy: Add support for RPC named arguments (Wladimir J. van der Laan)
4e7e2e1 Update RPC argument names (John Newbery) 481f289 rpc: Named argument support for bitcoin-cli (Wladimir J. van der Laan) 9adb4e1 rpc: Argument name consistency (Wladimir J. van der Laan) 8d713f7 rpc: Named arguments for rawtransaction calls (Wladimir J. van der Laan) 37a166f rpc: Named arguments for wallet calls (Wladimir J. van der Laan) 78b684f rpc: Named arguments for mining calls (Wladimir J. van der Laan) b8ebc59 rpc: Named arguments for net calls (Wladimir J. van der Laan) 2ca9dcd test: Add test for RPC named arguments (Wladimir J. van der Laan) fba1a61 rpc: Named arguments for misc calls (Wladimir J. van der Laan) 286ec08 rpc: Add 'echo' call for testing (Wladimir J. van der Laan) 495eb44 rpc: Named arguments for blockchain calls (Wladimir J. van der Laan) 6f1c76a rpc: Support named arguments (Wladimir J. van der Laan) 5865d41 authproxy: Add support for RPC named arguments (Wladimir J. van der Laan)
9ed1bdd [RPC] Remove obsolete and ignored 'detach' argument of stop command (random-zebra) 7d9af29 [Doc] Add RPC named arguments to release notes (random-zebra) 1582bdf [QA] Add rpc_named_arguments.py functional test (random-zebra) d87da93 rpc: Rename first named arg of createrawtransaction (random-zebra) 7ae8964 [Refactor] Sort RPC command tables alphabetically (random-zebra) da63bb8 rpc: Named argument support for pivx-cli (random-zebra) 22ee72a rpc: Named arguments for rawtransaction calls (Wladimir J. van der Laan) 3a93690 rpc: Named arguments for wallet calls (random-zebra) f137eca rpc: Named arguments for mining calls (Wladimir J. van der Laan) d524cc1 RPC: Named arguments for masternode, budget, and evo calls (random-zebra) 81de632 rpc: Named arguments for net calls (Wladimir J. van der Laan) 8e5bed7 rpc: Named arguments for misc calls (random-zebra) 4b2b980 rpc: Add 'echo' call for testing (Wladimir J. van der Laan) f8617c2 rpc: Named arguments for blockchain calls (random-zebra) a00e323 rpc: Support named arguments (Wladimir J. van der Laan) a0da903 authproxy: Add support for RPC named arguments (Wladimir J. van der Laan) Pull request description: The [JSON-RPC specification](http://www.jsonrpc.org/specification) allows either passing parameters as an Array, for positional arguments, or as an Object, for named arguments. Currently PIVX Core only supports positional arguments. This pull request adds support for named arguments, but preserves full backwards compatibility. APIs using by-name arguments are easier to extend - as arguments can be left out - and easier to use - no need to guess which argument goes where. This is especially nice in languages such as Python, which have native support for named arguments. Examples from the test: ```python h = node.help(command='getinfo') h = node.getblockhash(height=0) h = node.getblock(hash=h) ``` Backported from: - bitcoin#8811 : Add support for JSON-RPC named arguments _[W. J. van der Laan]_ - bitcoin#10084 : Rename first named arg of createrawtransaction _[MarcoFalke]_ ACKs for top commit: furszy: cool feature, ACK 9ed1bdd Fuzzbawls: ACK 9ed1bdd Tree-SHA512: 03001e5d1525e30af2126ebbabda275256df3dda170dbaf5732547af50dff90d655db78312666017840959704f6e0f268f99d91542e3a5f15484f1417a7bc904
The JSON-RPC specification allows either passing parameters as an Array, for by-position arguments, or as an Object, for by-name arguments. Currently Bitcoin Core only supports by-position arguments.
This pull request makes the (minimal) changes to support by-name arguments, but preserves full backwards compatibility. APIs using by-name arguments are easier to extend - as arguments can be left out - and easier to use - no need to guess which argument goes where.
This is especially nice in languages such as Python, which have native support for named arguments. Examples from the test:
RPC_INVALID_PARAMETER
) will be return if an unknown named argument is specified.Currently no calls have support for "holes" between arguments, but this should be implemented later on a per-call basis. These should be replaced with default values.
TODO
DONE see rpc: Add support for JSON-RPC named arguments #8811 (comment)bitcoin-cli
should grow support for providing named argumentsCalls inmining
,net
,rawtransaction
wallet
still need argument names filled in. I'll do this if reaction to this is positive.Later