RPC: Internal named params #11660

luke-jr commented at 11:25 AM on November 11, 2017: member

This allows RPC code to use named parameters internally, greatly increasing readability, as well as helping avoid behaviour tied to param count rather than the presence of specific parameters.

Object type checking is expanded to support multiple allowed types, making param and type-checking clean.

Temporarily, a boolean is added to the end of CRPCCommand to indicate whether the function expects named params. Once all RPC functions have been converted, we can drop it (as well as old internal-positional-param code).

Alternative to #11441

fanquake added the label RPC/REST/ZMQ on Nov 11, 2017

jonasschnelli commented at 8:54 PM on November 13, 2017: contributor

Haven't looked at the code to deep. Using name based arguments rather then arguments by index would increase readability a lot (and we have the position already in the RPCTable). Concept ACK.

ryanofsky commented at 3:24 PM on November 14, 2017: member

Nice change. @promag, you might want to look at this. You were asking about request.params.size() checks everywhere to trigger help (https://botbot.me/freenode/bitcoin-core-dev/msg/93498239/), and this PR gets rid of the need for these with transformPositionalArguments.

promag commented at 3:48 PM on November 14, 2017: member

@ryanofsky 👍

in src/rpc/server.cpp:518 in 2ca71c09a9 outdated

 522 | +    return out;
 523 | +}
 524 | +
 525 | +static inline JSONRPCRequest transformArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames, const bool want_named_args)
 526 | +{
 527 | +    if (!want_named_args && !in.params.isObject()) {

promag commented at 3:51 PM on November 14, 2017:

if (want_named_args == in.params.isObject()) {
    return in;
}

if (want_named_args) {
    return transformPositionalArguments(...);
}

return transformNamedArguments(...);

luke-jr commented at 4:48 PM on November 15, 2017:

That will fail to flatten options

in src/rpc/server.cpp:516 in 2ca71c09a9 outdated

 520 | +    }
 521 | +    // Return request with positional arguments transformed to named arguments
 522 | +    return out;
 523 | +}
 524 | +
 525 | +static inline JSONRPCRequest transformArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames, const bool want_named_args)

promag commented at 3:53 PM on November 14, 2017:

arg_names.

promag commented at 4:19 PM on November 14, 2017:

~~Remove const from want_named_args~~.

luke-jr commented at 4:47 PM on November 15, 2017:

Why?

in src/rpc/server.cpp:524 in 2ca71c09a9 outdated

 528 | +        // Already in desired form; no changes needed
 529 | +        return in;
 530 | +    }
 531 | +    if (want_named_args) {
 532 | +        if (in.params.isObject()) {
 533 | +            const UniValue& options_val = in.params["options"];

promag commented at 3:54 PM on November 14, 2017:

How about leaving options for other PR?

luke-jr commented at 4:49 PM on November 15, 2017:

It's needed for options-using RPCs...

in src/rpc/server.h:23 in 2ca71c09a9 outdated

  19 | @@ -19,6 +20,8 @@
  20 |  
  21 |  static const unsigned int DEFAULT_RPC_SERIALIZE_VERSION = 1;
  22 |  
  23 | +static const std::string ARGS_WERE_POSITIONAL = "_positional";

promag commented at 3:54 PM on November 14, 2017:

Remove?

in src/rpc/server.cpp:494 in 2ca71c09a9 outdated

 498 | +        // Too many params, so just trigger help
 499 | +        out.fHelp = true;
 500 | +        return out;
 501 | +    }
 502 | +    out.params = UniValue(UniValue::VOBJ);
 503 | +    out.params.pushKV(ARGS_WERE_POSITIONAL, true);

promag commented at 3:55 PM on November 14, 2017:

Remove or add usage?

luke-jr commented at 4:47 PM on November 15, 2017:

It's intended for #11413

promag commented at 2:08 PM on November 16, 2017:

How? Should be added there?

luke-jr commented at 2:16 PM on November 16, 2017:

It should probably fail if a named-args caller specifies conf_target with the explicit fee mode, or if feerate is specified with the automatic fee modes, so it would need to check if the duplication is merely because of positional arguments being used or not.

in src/wallet/rpcwallet.cpp:3387 in 2ca71c09a9 outdated

3081 | @@ -3082,24 +3082,22 @@ UniValue bumpfee(const JSONRPCRequest& request)
3082 |              HelpExampleCli("bumpfee", "<txid>"));
3083 |      }
3084 |  
3085 | -    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VOBJ});
3086 | +    RPCTypeCheckObj(request.params, {

promag commented at 4:09 PM on November 14, 2017:

IMO RPCTypeCheckObj is not very elegant. Here argument type is checked, and below it is parsed (which can fail) and sometimes it is validated (for instance out of bounds). Not to mention the "argument is not required and can be null" cases.

Something would be cleaner (don't mind the API):

uint256 hash = RequestParamUint256(request, "txid");

And if it has a default argument then it is not required:

bool replaceable = RequestParamBool(request, "options.replaceable", true);

So the idea is that these RequestParam* calls check, parse and validate the argument.

This is obviously work for other PR, but if you drop the options magic transformation and UniValueType changes then there is no change here too.

luke-jr commented at 4:50 PM on November 15, 2017:

How would we detect unexpected params?

in src/wallet/rpcwallet.cpp:3340 in 2ca71c09a9 outdated

3034 | @@ -3035,7 +3035,7 @@ UniValue bumpfee(const JSONRPCRequest& request)
3035 |      if (!EnsureWalletIsAvailable(pwallet, request.fHelp))
3036 |          return NullUniValue;
3037 |  
3038 | -    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {
3039 | +    if (request.fHelp || request.params["txid"].isNull()) {

promag commented at 4:14 PM on November 14, 2017:

Just if (request.fHelp)?

luke-jr commented at 4:49 PM on November 15, 2017:

That would change behaviour.

in src/rpc/server.cpp:83 in 2ca71c09a9 outdated

  82 | @@ -83,21 +83,31 @@ void RPCTypeCheckObj(const UniValue& o,
  83 |  {
  84 |      for (const auto& t : typesExpected) {
  85 |          const UniValue& v = find_value(o, t.first);
  86 | -        if (!fAllowNull && v.isNull())
  87 | +        if (t.second.IsAllowed(v.type()) || (fAllowNull && v.isNull())) {

promag commented at 4:19 PM on November 14, 2017:

Unnecessary ().

luke-jr commented at 4:45 PM on November 15, 2017:

Necessary for readable clarity...?

promag commented at 4:19 PM on November 14, 2017: member

Concept ACK. Should be a lot friendly to query parameters by name instead. My suggestion is to take the opportunity and somehow also improve the internal API.

jnewbery commented at 7:58 PM on November 15, 2017: member

I've had a quick skim and this looks like a good, clean change that maintains backwards-compatibility. @luke-jr, just so I understand the reasoning here, can you explain what the longer-term plan is:

as it stands, this PR maintains backward-compatible support for positional arguments, I assume any future PRs would do the same?
is the plan to slowly move all RPC methods to have named_args set to true, and then remove the bool and special behaviour?
Are you also planning to remove the options arguments and flatten them down into the regular arguments list?
Changing bumpfee to use the new pattern adds a RPCTypeCheckObj() call to the top of the function. I suppose that will be the case for all RPCs, or could we build something into the RPC framework to make that checking automatic?

luke-jr commented at 8:06 PM on November 15, 2017: member

as it stands, this PR maintains backward-compatible support for positional arguments, I assume any future PRs would do the same?

Positional arguments are still fully supported. (No "backward" about it...)

is the plan to slowly move all RPC methods to have named_args set to true, and then remove the bool and special behaviour?

Right

Are you also planning to remove the options arguments and flatten them down into the regular arguments list?

That's what this does already...?

Changing bumpfee to use the new pattern adds a RPCTypeCheckObj() call to the top of the function. I suppose that will be the case for all RPCs, or could we build something into the RPC framework to make that checking automatic?

I think it's best to have as much of the stuff for a given RPC with it in the code. We already have this for most things.

jnewbery commented at 8:09 PM on November 15, 2017: member

Are you also planning to remove the options arguments and flatten them down into the regular arguments list?

That's what this does already...?

Perhaps I was unclear - is the plan to remove the 'options' argument in the help text so that the 'standard' way to call the RPC is with the options as key-values in the request rather than in an options object in the request?

luke-jr commented at 3:11 AM on November 16, 2017: member

I'm not sure. The help text covers both named and positional arguments, and an options Object is the ideal way to do many things with positional arguments.

RPC: Support using named params internally fe8743dfd4

RPC: Support multiple types for RPCTypeCheck* 4a9ba9af1c

RPC: Use internal named params for help RPC 3a0aedab42

RPC: RPCTypeCheckObj should ignore unexpected null keys 69316a8cef

RPC: Flatten "options" Object even for named->named 7f3707b34a

RPC/Wallet: Convert bumpfee to use named params internally 83d1613850

in src/wallet/rpcwallet.cpp:3311 in 2ca71c09a9 outdated

3307 | @@ -3310,7 +3308,7 @@ static const CRPCCommand commands[] =
3308 |      { "wallet",             "addmultisigaddress",       &addmultisigaddress,       {"nrequired","keys","account"} },
3309 |      { "wallet",             "addwitnessaddress",        &addwitnessaddress,        {"address","p2sh"} },
3310 |      { "wallet",             "backupwallet",             &backupwallet,             {"destination"} },
3311 | -    { "wallet",             "bumpfee",                  &bumpfee,                  {"txid", "options"} },
3312 | +    { "wallet",             "bumpfee",                  &bumpfee,                  {"txid", "options"}, true },

ryanofsky commented at 1:28 PM on November 16, 2017:

There's no need to have this new "named_args" bool field. Univalue's array access operator explicitly works on object values as well as array values:

https://github.com/bitcoin/bitcoin/blob/99bc0b428b03b571afbc311b7f18fd3a707ac5af/src/univalue/lib/univalue.cpp#L211-L219

So instead of introducing different flavors of RPC functions where some receive array params and others receive object params, every RPC function could just receive the same params, and choose to access arguments by name or position, whichever is more convenient. Implementing this would also simplify transformNamedArguments which could get rid of the "hole-filling" logic no longer needed after #11050.

luke-jr commented at 1:41 PM on November 16, 2017:

Hmm, interesting thought. It feels a bit too much like relying on undefined behaviour to me. It's also probably complicated to implement properly - so before I put a bunch of effort into that: What do others think?

Considering the bool is only temporary anyway (I can complete the conversion in a single PR?), is this worth it?

ryanofsky commented at 1:57 PM on November 16, 2017:

Considering the bool is only temporary anyway (I can complete the conversion in a single PR?), is this worth it?

Oh, I would say definitely not worth it if you are planning on getting rid of positional accesses entirely (and deleting transformNamedArguments).

It feels a bit too much like relying on undefined behaviour to me.

I can see how this "feels" like relying on undefined behavior, but of course it is not really. It is just relying on univalue behavior that's outside the realm on the json spec. In some cases where we do this (e.g. interpretation of numeric values, not distinguishing between undefined and null values) this isn't great because it can lead to incompatibility with client json libraries. But in this case we're just talking about passing params from our internal rpc framework to our internal rpc method implementations, so using univalue to pass on position metadata should not be a big deal. Again, though not worth it if you are planning on getting rid of positional accesses.

luke-jr force-pushed on Mar 31, 2018

luke-jr commented at 8:25 PM on March 31, 2018: member

Rebased...

MarcoFalke added the label Needs rebase on Jun 6, 2018

DrahtBot commented at 11:16 PM on November 8, 2018: member

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

DrahtBot added the label Up for grabs on Nov 8, 2018

DrahtBot closed this on Nov 8, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021