rpc/server: Support for specifying options as named parameters

luke-jr commented at 2:32 am on October 3, 2017: member

rpc/server: Support for specifying options as named parameters a4e402c960

QA: Test named params interaction with RPC options 3a2ab53cf9

fanquake added the label RPC/REST/ZMQ on Oct 3, 2017

jnewbery commented at 9:45 pm on October 3, 2017: member

I’m not sure what the motivation is for this, since there’s no PR description.

As currently implemented, this doesn’t seem to work for bitcoin.cli. Options don’t get converted from strings into the correct type:

0→ bitcoin-cli -named fundrawtransaction hexstring=02000000000100bca065010000001976a9146bed745a50ee10d6dbd524b4806f84fec46c4bf388ac00000000 changeAddress=mqMd4D1wY8ye82FpPbT98keCifcue3u35n changePosition=0
1error code: -3
2error message:
3Expected type number for changePosition, got string

It also seems to break argument name checking. With this PR:

0→ bitcoin-cli -named fundrawtransaction hexstirng=02000000000100bca065010000001976a9146bed745a50ee10d6dbd524b4806f84fec46c4bf388ac00000000
1error code: -3
2error message:
3Expected type string, got null

on master:

0→ bitcoin-cli -named fundrawtransaction hexstirng=02000000000100bca065010000001976a9146bed745a50ee10d6dbd524b4806f84fec46c4bf388ac00000000
1error code: -8
2error message:
3Unknown named parameter hexstirng

luke-jr commented at 0:18 am on October 4, 2017: member

As currently implemented, this doesn’t seem to work for bitcoin.cli. Options don’t get converted from strings into the correct type:

That would be a task for an independent PR. This is just for the server.

It also seems to break argument name checking.

Doesn’t look broken to me. It’s seeing the missing argument before checking options. If we want the other behaviour, the RPC method should check options before other arguments.

sipa commented at 0:35 am on October 4, 2017: member

How about going the other way? When an ‘options’ parameter is present, and the RPC has no argument with that name, treat its keys as extra named arguments. This presents an easier interface for implementing RPCs (which can just use named arguments, rather than needing to disassemble and typecheck fields of an object).

This needs motivation regardless.

luke-jr commented at 1:11 am on October 4, 2017: member

The motivation is to fix the named args interface.

Going “the other way” encourages terrible array-args interfaces, which should be discouraged.

sipa commented at 2:15 am on October 4, 2017: member

Going “the other way” encourages terrible array-args interfaces, which should be discouraged. Merge state

The interface would be identical. You’d accept any named argument either as real named argument, or as a key in a positional ‘options’ argument. You either convert one to the other, or the other to one.

But it would be easier to write RPC handlers for.

luke-jr commented at 3:09 am on October 4, 2017: member

But when using positional arguments, you’d have a terrible interface where you have a dozen null values just to reach the one you actually want to set.

laanwj commented at 12:46 pm on October 4, 2017: member

slight NACK - I’m not convinced this is really necessary. As I see it it adds complexity to the argument parsing code and makes nothing new possible. Just follow the darn API and pass these additional options in the options object and don’t be lazy. (a slight argument could be made that this makes using bitcoin-cli with the options object easier, but that’s nullified by the difficulty of getting the specific-option-argument parsing to work here…)

jnewbery commented at 1:11 pm on October 4, 2017: member

That would be a task for an independent PR. This is just for the server.

The interface for bitcoin-cli should be as close as possible to the RPC interface. It’s a useful testing tool for people writing their own RPC clients, so we should avoid there being any surprises when switching between the two. In general, I don’t think we should merge PRs that increase the delta between bitcoin-cli and the RPC interface.

It also seems to break argument name checking.

Doesn’t look broken to me.

The point is that this PR makes the error message less helpful for the user. That’s a regression.

The motivation is to fix the named args interface.

It’s not obvious to me that the named args interface needs ‘fixing’. Named args are arguably better than an options object for a number of reasons:

name and type checking is provided for free by the rpc/server framework.
providers a simpler, flatter interface for client code.
options objects are really awkward to use with bitcoin-cli and shells.

But when using positional arguments, you’d have a terrible interface

No-one is suggesting that users should use positional arguments. Named args solve this problem already.

promag commented at 11:01 pm on October 4, 2017: member

Named arguments with -- prefix (or whatever) could form the options object, just a shortcut to write the JSON. Example:

0bitcoin-cli -named fundrawtransaction hexstring=... --lockUnspents=true

This sounds more command line to me.

Anyway, IMO we should avoid adding fat to bitcoin-cli.

luke-jr commented at 11:11 pm on October 4, 2017: member

Named args should not be used as an excuse to have a crappy positional-args interface.

promag commented at 11:41 pm on October 4, 2017: member

where you have a dozen null values just to reach the one you actually want to set.

This is not improved here. The JSON-RPC implementation supports both positional and named arguments and this can not change. If all RPC method implementations change to use named arguments then we must invert the index->name mapping to keep compatibility with the existing version.

I can’t see the advantages for other clients other than bitcoin-cli. And for bitcoin-cli something like would be enough.

luke-jr commented at 3:44 am on October 5, 2017: member

@promag The other day someone was being suggested to make the problem worse.

luke-jr commented at 11:28 am on November 11, 2017: member

Superceded by #11660, hopefully addressing the concerns here.

luke-jr closed this on Nov 11, 2017

DrahtBot locked this on Sep 8, 2021

rpc/server: Support for specifying options as named parameters #11441