RPC: Internal named params #11660

luke-jr · 2017-11-11T11:25:31Z

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

jonasschnelli · 2017-11-13T20:54:26Z

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

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 · 2017-11-14T15:48:10Z

@ryanofsky 👍

promag

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.

src/rpc/server.cpp

promag · 2017-11-14T15:53:09Z

src/rpc/server.cpp

+    return out;
+}
+
+static inline JSONRPCRequest transformArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames, const bool want_named_args)


arg_names.

src/rpc/server.cpp

promag · 2017-11-14T15:54:58Z

src/rpc/server.h

@@ -19,6 +20,8 @@

 static const unsigned int DEFAULT_RPC_SERIALIZE_VERSION = 1;

+static const std::string ARGS_WERE_POSITIONAL = "_positional";


promag · 2017-11-14T15:55:08Z

src/rpc/server.cpp

+        return out;
+    }
+    out.params = UniValue(UniValue::VOBJ);
+    out.params.pushKV(ARGS_WERE_POSITIONAL, true);


Remove or add usage?

It's intended for #11413

How? Should be added there?

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.

promag · 2017-11-14T16:09:59Z

src/wallet/rpcwallet.cpp

@@ -3082,24 +3082,22 @@ UniValue bumpfee(const JSONRPCRequest& request)
            HelpExampleCli("bumpfee", "<txid>"));
    }

-    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VOBJ});
+    RPCTypeCheckObj(request.params, {


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.

How would we detect unexpected params?

src/wallet/rpcwallet.cpp

src/rpc/server.cpp

jnewbery · 2017-11-15T19:58:40Z

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 · 2017-11-15T20:06:17Z

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 · 2017-11-15T20:09:40Z

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 · 2017-11-16T03:11:34Z

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.

ryanofsky · 2017-11-16T13:28:39Z

src/wallet/rpcwallet.cpp

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


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:

bitcoin/src/univalue/lib/univalue.cpp

Lines 211 to 219 in 99bc0b4

const UniValue& UniValue::operator[](size_t index) const

{

if (typ != VOBJ && typ != VARR)

return NullUniValue;

if (index >= values.size())

return NullUniValue;

return values.at(index);

}

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.

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?

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 · 2018-03-31T20:25:01Z

Rebased...

DrahtBot · 2018-11-08T23:16:38Z

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.

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

luke-jr mentioned this pull request Nov 11, 2017

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

Closed

ryanofsky reviewed Nov 14, 2017

View reviewed changes

promag reviewed Nov 14, 2017

View reviewed changes

ryanofsky reviewed Nov 16, 2017

View reviewed changes

jnewbery mentioned this pull request Dec 12, 2017

[rpc] createrawtransaction: Accept sorted outputs #11872

Merged

ryanofsky mentioned this pull request Jan 23, 2018

RPC Whitelist Files #12248

Closed

luke-jr added 6 commits March 31, 2018 19:42

RPC: Support using named params internally

fe8743d

RPC: Support multiple types for RPCTypeCheck*

4a9ba9a

RPC: Use internal named params for help RPC

3a0aeda

RPC: RPCTypeCheckObj should ignore unexpected null keys

69316a8

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

7f3707b

RPC/Wallet: Convert bumpfee to use named params internally

83d1613

luke-jr force-pushed the internal_named_params branch from 2ca71c0 to 83d1613 Compare March 31, 2018 20:24

maflcko added the Needs rebase label Jun 6, 2018

DrahtBot added the Up for grabs label Nov 8, 2018

DrahtBot closed this Nov 8, 2018

luke-jr mentioned this pull request Aug 30, 2019

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413

Merged

laanwj removed the Needs rebase label Oct 24, 2019

luke-jr mentioned this pull request Nov 2, 2019

RPC: Internal named params #17356

Closed

ryanofsky mentioned this pull request Jul 28, 2021

rpc: Allow walletprocesspsbt to sign without finalizing #22513

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -19,6 +20,8 @@

		static const unsigned int DEFAULT_RPC_SERIALIZE_VERSION = 1;

		static const std::string ARGS_WERE_POSITIONAL = "_positional";

	const UniValue& UniValue::operator[](size_t index) const
	{
	if (typ != VOBJ && typ != VARR)
	return NullUniValue;
	if (index >= values.size())
	return NullUniValue;

	return values.at(index);
	}

RPC: Internal named params #11660

RPC: Internal named params #11660

Uh oh!

Conversation

luke-jr commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Nov 13, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 14, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Nov 15, 2017

Uh oh!

luke-jr commented Nov 15, 2017

Uh oh!

jnewbery commented Nov 15, 2017

Uh oh!

luke-jr commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Mar 31, 2018

Uh oh!

DrahtBot commented Nov 8, 2018

Uh oh!

Uh oh!

luke-jr commented Nov 11, 2017 •

edited

Loading

ryanofsky Nov 16, 2017 •

edited

Loading