rpc: Document default values for optional arguments

MarcoFalke commented at 10:00 pm on December 5, 2018: member

MarcoFalke added the label Docs on Dec 5, 2018

MarcoFalke added the label RPC/REST/ZMQ on Dec 5, 2018

DrahtBot commented at 10:53 pm on December 5, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14641 (rpc: Add min_conf option to fund transaction calls by promag)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

in src/rpc/rawtransaction.cpp:464 in eeee62f5c1 outdated

461+                            {"", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "", "",
462                                 {
463                                     {"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction id"},
464                                     {"vout", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "The output number"},
465-                                    {"sequence", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "The sequence number"},
466+                                    {"sequence", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "depends on the value of the 'replacable' and 'locktime' arguments", "The sequence number"},

practicalswift commented at 3:14 pm on December 6, 2018:

Should be “replaceable” :-)

in src/rpc/rawtransaction.cpp:1606 in eeee62f5c1 outdated

1602@@ -1603,7 +1603,7 @@ UniValue createpsbt(const JSONRPCRequest& request)
1603                                 {
1604                                     {"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction id"},
1605                                     {"vout", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "The output number"},
1606-                                    {"sequence", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "The sequence number"},
1607+                                    {"sequence", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "depends on the value of the 'replacable' and 'locktime' arguments", "The sequence number"},

practicalswift commented at 3:15 pm on December 6, 2018:

Same here – should be “replaceable” :-)

in src/rpc/rawtransaction.cpp:522 in eeee62f5c1 outdated

518@@ -519,7 +519,7 @@ static UniValue decoderawtransaction(const JSONRPCRequest& request)
519                 "\nReturn a JSON object representing the serialized, hex-encoded transaction.\n",
520                 {
521                     {"hexstring", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction hex string"},
522-                    {"iswitness", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "", "Whether the transaction hex is a serialized witness transaction\n"
523+                    {"iswitness", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "depends on heuritic tests", "Whether the transaction hex is a serialized witness transaction\n"

practicalswift commented at 3:15 pm on December 6, 2018:

Should be “heuristic” :-)

practicalswift changes_requested

MarcoFalke force-pushed on Dec 6, 2018

MarcoFalke commented at 6:38 pm on December 6, 2018: member

Thanks @practicalswift. Fixed my language

DrahtBot added the label Needs rebase on Dec 7, 2018

rpc: Document default values for optional arguments fa0c24c96e

MarcoFalke force-pushed on Dec 7, 2018

DrahtBot removed the label Needs rebase on Dec 7, 2018

in src/rpc/mining.cpp:891 in fa0c24c96e

887@@ -888,9 +888,9 @@ static UniValue estimaterawfee(const JSONRPCRequest& request)
888                 "defined in BIP 141 (witness data is discounted).\n",
889                 {
890                     {"conf_target", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "Confirmation target in blocks (1 - 1008)"},
891-                    {"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "The proportion of transactions in a given feerate range that must have been\n"
892+                    {"threshold", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0.95", "The proportion of transactions in a given feerate range that must have been\n"

jonasschnelli commented at 11:09 pm on December 9, 2018:

what about using a stringprint of the threshold variable?

jonasschnelli commented at 11:14 pm on December 9, 2018: contributor

utACK fa0c24c96e9937f666dcdd83d12145720c7b0329 Somehow I would be in favour of putting brackets around descriptive values ((null) instead of null and (fallback to nodeid) instead of fallback to nodeid) to avoid confusion between real default values and behavioural description.

kristapsk commented at 0:33 am on December 10, 2018: contributor

ACK fa0c24c96e9937f666dcdd83d12145720c7b0329

in src/rpc/net.cpp:522 in fa0c24c96e

517@@ -518,8 +518,8 @@ static UniValue setban(const JSONRPCRequest& request)
518                 {
519                     {"subnet", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "The IP/Subnet (see getpeerinfo for nodes IP) with an optional netmask (default is /32 = single IP)"},
520                     {"command", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "'add' to add an IP/Subnet to the list, 'remove' to remove an IP/Subnet from the list"},
521-                    {"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
522-                    {"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},
523+                    {"bantime", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "time in seconds how long (or until when if [absolute] is set) the IP is banned (0 or empty means using the default time of 24h which can also be overwritten by the -bantime startup argument)"},
524+                    {"absolute", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "If set, the bantime must be an absolute timestamp in seconds since epoch (Jan 1 1970 GMT)"},

ryanofsky commented at 5:33 pm on December 10, 2018:

/If set/If true/ might be more accurate

ryanofsky approved

ryanofsky commented at 5:48 pm on December 10, 2018: member

ACK fa0c24c96e9937f666dcdd83d12145720c7b0329. I checked help output using the hack from #14726#pullrequestreview-175423309

This is a nice documentation change. If you wanted to follow up with more improvements I have some suggestions below:

The fallback descriptions like “fallback to wallet's default” are vague and could be described in more detail.
I don’t think saying (default=null) or (default=omitted) is ever helpful. Just because of the way we check arguments, omitting an argument is always equivalent to passing null. And (default=omitted) is just a tautology. The point of the default documentation should be to say what an RPC call will do when an argument is null or omitted. I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can’t be replaced with something more constructive, to avoid creating a bad precedent for future changes.

kristapsk commented at 5:58 pm on December 10, 2018: contributor

I don’t think saying (default=null) or (default=omitted) is ever helpful. (…) I actually think it would be better to drop the all the (default=null) and (default=omitted) strings if they can’t be replaced with something more constructive, to avoid creating a bad precedent for future changes.

If there is “default=” string outputted for each optional argument, functional test could be made that checks that people don’t forget to document optional argument defaults for a new RPCs.

ryanofsky commented at 6:29 pm on December 10, 2018: member

If there is “default=” string outputted for each optional argument, functional test could be made that checks that people don’t forget to document optional argument defaults for a new RPCs.

IMO, better to write that test with a list of exceptions for grandfathered, undocumented default arguments, than to leave bad documentation scattered over the codebase, especially since existing documentation is often used as a template for new documentation

kristapsk commented at 6:41 pm on December 10, 2018: contributor

Oh, yes, short list of a few exceptions probably is better idea.

MarcoFalke commented at 6:56 pm on December 10, 2018: member

I have changes piled up locally to enforce that it is either a required arg or a default value is provided at compile time.

MarcoFalke merged this on Dec 10, 2018

MarcoFalke closed this on Dec 10, 2018

MarcoFalke referenced this in commit 5f23460c7e on Dec 10, 2018

MarcoFalke deleted the branch on Dec 10, 2018

ryanofsky commented at 8:52 pm on December 10, 2018: member

@MarcoFalke what do you want to do with all the (default=null) / (default=omitted) cruft? Do you want to address it in a future PR or maybe mark this one up for grabs so someone else can fill in useful default values?

MarcoFalke commented at 8:57 pm on December 10, 2018: member

@ryanofsky Your suggestion is a one-line change after #14918, so either you leave feedback there or create a pull request after #14918 is merged.

MarcoFalke referenced this in commit 04226f8706 on Jan 30, 2019

deadalnix referenced this in commit 5a5256baa9 on May 22, 2020

Munkybooty referenced this in commit ad34d67319 on Aug 21, 2021

Munkybooty referenced this in commit 8e2a2aac53 on Aug 23, 2021

Munkybooty referenced this in commit 5367671054 on Aug 24, 2021

Munkybooty referenced this in commit 68c722f867 on Aug 24, 2021

Munkybooty referenced this in commit fcd4c15ad6 on Aug 24, 2021

UdjinM6 referenced this in commit 6cc03c0e81 on Aug 24, 2021

Munkybooty referenced this in commit 9b233fbc4c on Aug 24, 2021

DrahtBot locked this on Sep 8, 2021

rpc: Document default values for optional arguments #14877

Conflicts