rpc: Pass argument descriptions to RPCHelpMan

MarcoFalke commented at 10:50 pm on November 23, 2018: member

This will normalize the type names and formatting for the rpc arguments

MarcoFalke added the label RPC/REST/ZMQ on Nov 23, 2018

MarcoFalke added the label Refactoring on Nov 23, 2018

MarcoFalke added the label Docs on Nov 23, 2018

promag commented at 1:03 am on November 24, 2018: member

:clap:

DrahtBot commented at 1:37 am on November 24, 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)
#14601 ([rpc] Descriptions: Consistent arg labels for types ‘object’, ‘array’, ‘boolean’, and ‘string’ by ch4ot1c)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)
#11484 (Optional update rescan option in importmulti RPC by pedrobranco)
#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.

MarcoFalke force-pushed on Nov 25, 2018

in src/rpc/util.cpp:180 in 1609da6cb3 outdated

175+        }
176+        case RPCArg::Type::OBJ:
177+        case RPCArg::Type::OBJ_USER_KEYS: {
178+            const auto right = outer_type == OuterType::NAMED_ARG ? "" : arg.ToDescriptionString(/* implicitly_required */ outer_type == OuterType::ARR);
179+            PushSection({indent + "{", right});
180+            for (const auto& arg : arg.m_inner) {

practicalswift commented at 9:04 pm on November 25, 2018:

Shadows const RPCArg& arg :-)

in src/rpc/util.cpp:195 in 1609da6cb3 outdated

190+            auto left = indent;
191+            left += outer_type == OuterType::OBJ ? "\"" + arg.m_name + "\":" : "";
192+            left += "[";
193+            const auto right = outer_type == OuterType::NAMED_ARG ? "" : arg.ToDescriptionString(/* implicitly_required */ outer_type == OuterType::ARR);
194+            PushSection({left, right});
195+            for (const auto& arg : arg.m_inner) {

practicalswift commented at 9:04 pm on November 25, 2018:

Same here.

MarcoFalke commented at 4:09 pm on November 26, 2018:

Thx. Fixed both

MarcoFalke force-pushed on Nov 26, 2018

DrahtBot added the label Needs rebase on Nov 26, 2018

MarcoFalke force-pushed on Nov 26, 2018

DrahtBot removed the label Needs rebase on Nov 26, 2018

DrahtBot added the label Needs rebase on Nov 27, 2018

rpc: Pass argument descriptions to RPCHelpMan 1db0096f61

MarcoFalke force-pushed on Nov 27, 2018

DrahtBot removed the label Needs rebase on Nov 27, 2018

in src/wallet/rpcwallet.cpp:4270 in 1db0096f61 outdated

4107+                            {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "", "A json array of integers.\n"
4108                             "                              The fee will be equally deducted from the amount of each specified output.\n"
4109                             "                              The outputs are specified by their zero-based index, before any change output is added.\n"
4110                             "                              Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n"
4111-                            "                              If no outputs are specified here, the sender pays the fee.\n"
4112-                            "                                  [vout_index,...]\n"

ryanofsky commented at 8:38 pm on December 3, 2018:

New help seems little strange with vout_index on its own line and with no description.

0       "subtractFeeFromOutputs":[    (json array, optional) A json array of integers.
1                                     The fee will be equally deducted from the amount of each specified output.
2                                     The outputs are specified by their zero-based index, before any change output is added.
3                                     Those recipients will receive less bitcoins than you enter in their corresponding amount field.
4                                     If no outputs are specified here, the sender pays the fee.
5         vout_index,                 (numeric)
6         ...
7       ],

Maybe vout_index should be given a description like “The zero-based output index, before a change output is added”

MarcoFalke commented at 4:59 pm on December 4, 2018:

Done

in src/wallet/rpcdump.cpp:1156 in 1db0096f61 outdated

1160                         },
1161-                        true, "\"options\""},
1162+                        "\"options\""},
1163                 }}
1164                 .ToString() +
1165-            "Arguments:\n"

ryanofsky commented at 9:04 pm on December 3, 2018:

Output is changing here as follows:

0-"redeemscript": "<script>"  
1+"redeemscript":"str",

But previous output seems better, and it would be nice if the same pattern could be used in other places.

The “<variable>” notation is nice because it allows distinguishing variable strings from literal strings. The current convention where you’re supposed to magically know that “redeemscript” is a literal string, while “str” is a variable string is unnecessarily confusing, and the confusion is worse in other cases. For example “height” from getblockstats is a literal string:

0"height",     (string) Selected statistic

while “key” from createmultisig is a variable string:

0"key",        (string) The hex-encoded public key

And “address” from createpsbt is a variable string:

0 "address":amount,

While “data” from createpsbt is a literal string:

0"data":"hex"

And of course “hex” is a variable string.

It would be nicer to show “redeemscript”, “<script>”, “height”, “<key>”, “<address>”, “data”, and “<hex>” respectively in these cases to make documentation more clear and obvious. And it would be good if RPCHelpMan could help with formatting and maintaining the literal/variable distinction.

MarcoFalke commented at 5:36 pm on December 4, 2018:

This should be done in a separate pull request, since it will change most of the existing documentation and this pull request’s goal is to normalize all documentation to a single standard.

in src/rpc/mining.cpp:244 in 1db0096f61 outdated

249-            "1. \"txid\"       (string, required) The transaction id.\n"
250-            "2. dummy          (numeric, optional) API-Compatibility for previous API. Must be zero or null.\n"
251-            "                  DEPRECATED. For forward compatibility use named arguments and omit this parameter.\n"
252-            "3. fee_delta      (numeric, required) The fee value (in satoshis) to add (or subtract, if negative).\n"
253+                    {"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction id."},
254+                    {"dummy", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "API-Compatibility for previous API. Must be zero or null.\n"

ryanofsky commented at 9:21 pm on December 3, 2018:

Output is:

02. dummy        (numeric, required) API-Compatibility for previous API. Must be zero or null.

Should this say optional instead of required since value can be null?

MarcoFalke commented at 5:37 pm on December 4, 2018:

This conflicts the existing one-line summary, so I’d prefer to keep the changes to the documentation minimal and only normalize everything and then do the cleanup in follow up pull requests.

This is among the changes I have queued up.

in src/rpc/rawtransaction.cpp:1622 in 1db0096f61 outdated

1630-                            {"", RPCArg::Type::OBJ,
1631+                                },
1632+                            {"", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "", "",
1633                                 {
1634-                                    {"data", RPCArg::Type::STR_HEX, false},
1635+                                    {"data", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "A key-value pair. The key must be \"data\", the value is hex-encoded data"},

ryanofsky commented at 9:43 pm on December 3, 2018:

Output changes as follows:

0-"address": x.xxx
1+"address":amount,
2-"data": "hex"
3+"data":"hex",

I think it would be a little nicer to keep the spaces after colons. If you look for JSON examples on the internet they tend to include spaces e.g., https://en.wikipedia.org/wiki/JSON#JSON_sample, https://json.org/example.html, and tools like jq and javascript devtools also show spaces when pretty printing.

MarcoFalke commented at 5:38 pm on December 4, 2018:

Good point.

in src/rpc/util.h:52 in 1db0096f61 outdated

50-    RPCArg(const std::string& name, const Type& type, const bool optional, const std::string& oneline_description = "")
51-        : m_name{name}, m_type{type}, m_optional{optional}, m_oneline_description{oneline_description}
52+    const std::vector<std::string> m_type_str; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_type_str.at(0) will override the type of the value in a key-value pair, m_type_str.at(1) will override the type in the argument description.
53+
54+    RPCArg(
55+        const std::string& name,

ryanofsky commented at 9:54 pm on December 3, 2018:

Would be more optimal to take strings by value instead of const reference to avoid copying:

0RPCArg(std::string name) : m_name(std::move(name)) {}

In general this a better pattern to follow when a function inserts a value into a structure.

in src/rpc/util.h:49 in 1db0096f61 outdated

47+    const std::string m_description;
48     const std::string m_oneline_description; //!< Should be empty unless it is supposed to override the auto-generated summary line
49-
50-    RPCArg(const std::string& name, const Type& type, const bool optional, const std::string& oneline_description = "")
51-        : m_name{name}, m_type{type}, m_optional{optional}, m_oneline_description{oneline_description}
52+    const std::vector<std::string> m_type_str; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_type_str.at(0) will override the type of the value in a key-value pair, m_type_str.at(1) will override the type in the argument description.

ryanofsky commented at 9:59 pm on December 3, 2018:

Might be more appropriate to use optional<tuple<string, string>>

in src/rpc/util.h:93 in 1db0096f61 outdated

 96-    std::string ToString() const;
 97-
 98-private:
 99+    std::string ToString(bool oneline = false) const;
100     std::string ToStringObj() const;
101+    std::string ToDescriptionString(bool implicitly_required = false) const;

ryanofsky commented at 10:00 pm on December 3, 2018:

Can you add a comment saying what implicitly_required means?

MarcoFalke commented at 6:28 pm on December 4, 2018:

It is set to true for arguments in an array, because those are commonly omitted in the current help texts.

I presume this is because they are implicitly required if you pass an array, but are still optional because you can pass different number of args in. I think it should be clear from context what the interface looks like.

in src/rpc/util.cpp:143 in 1db0096f61 outdated

138+
139+struct Sections {
140+    std::vector<Section> m_sections;
141+    size_t m_max_pad{0};
142+
143+    void PushSection(const Section& s)

ryanofsky commented at 10:05 pm on December 3, 2018:

Generally a better pattern anytime you have a function which inserts a value into a larger data structure is to take the argument by value instead of reference, and use std::move() to avoid copying.

in src/rpc/util.h:45 in 1db0096f61 outdated

42@@ -43,24 +43,54 @@ struct RPCArg {
43     const Type m_type;
44     const std::vector<RPCArg> m_inner; //!< Only used for arrays or dicts
45     const bool m_optional;

ryanofsky commented at 10:14 pm on December 3, 2018:

Why keep this member? It seems like keeping it could only lead to inconsistencies or omissions in the output. It would be easy to define an bool IsOptional() const { return !m_default.value.empty(); } helper if needed for convenience.

MarcoFalke commented at 4:44 pm on December 4, 2018:

Sure, but this will be a follow up pull request, since it needs someone to actually supply the default value when the argument is optional. See also the comment in the cpp file:

0// TODO enable this assert, when all optional parameters have their default value documented
1//assert(false);

ryanofsky approved

ryanofsky commented at 10:24 pm on December 3, 2018: member

ACK 1db0096f61680d2b2a9cfe454154de3ad121a9d3. Verified output using hack from #14726#pullrequestreview-175423309.

MarcoFalke force-pushed on Dec 4, 2018

rpc: Add description to fundrawtransaction vout_index fafd040f73

MarcoFalke force-pushed on Dec 4, 2018

RPCHelpMan: Add space after colons in extended description

Also, add doxygen comment to ToDescriptionString

fabca42c68

MarcoFalke force-pushed on Dec 4, 2018

ryanofsky approved

ryanofsky commented at 8:42 pm on December 4, 2018: member

utACK fabca42c68ee4cdff08d30e91412ccf1de6d7b41. Only changes since last review are adding vout_index description (should maybe be followed up by removing redundant text from subtractFeeFromOutputs description), adding space after colon in json formatting, and adding some code comments.

MarcoFalke merged this on Dec 5, 2018

MarcoFalke closed this on Dec 5, 2018

MarcoFalke referenced this in commit e2c473ff75 on Dec 5, 2018

MarcoFalke deleted the branch on Dec 5, 2018

deadalnix referenced this in commit 578deeb08e on Apr 13, 2020

deadalnix referenced this in commit 969731122b on Apr 14, 2020

deadalnix referenced this in commit f7eb5edff8 on Apr 14, 2020

deadalnix referenced this in commit 3413afd1ce on Apr 14, 2020

deadalnix referenced this in commit b17d1db1ba on Apr 14, 2020

deadalnix referenced this in commit 5850294bc6 on Apr 15, 2020

deadalnix referenced this in commit c69d41b2bc on Apr 15, 2020

deadalnix referenced this in commit b8e343ee30 on Apr 15, 2020

deadalnix referenced this in commit 245bf8cc7f on Apr 15, 2020

deadalnix referenced this in commit a0950966fd on Apr 15, 2020

deadalnix referenced this in commit 02ab89366a on Apr 15, 2020

MarcoFalke locked this on Sep 8, 2021

rpc: Pass argument descriptions to RPCHelpMan #14796

Conflicts