rpc: Pass argument descriptions to RPCHelpMan #14796

maflcko · 2018-11-23T22:50:40Z

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

promag · 2018-11-24T01:03:50Z

👏

DrahtBot · 2018-11-24T01:37:59Z

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.

src/rpc/util.cpp

ryanofsky

ACK 1db0096. Verified output using hack from #14726 (review).

src/wallet/rpcwallet.cpp

ryanofsky · 2018-12-03T21:04:04Z

src/wallet/rpcdump.cpp

+                        "\"options\""},
                }}
                .ToString() +
-            "Arguments:\n"


Output is changing here as follows:

-"redeemscript": "<script>" +"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:

"height", (string) Selected statistic

while "key" from createmultisig is a variable string:

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

And "address" from createpsbt is a variable string:

"address":amount,

While "data" from createpsbt is a literal string:

"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.

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.

ryanofsky · 2018-12-03T21:21:08Z

src/rpc/mining.cpp

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


Output is:

2. 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?

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.

src/rpc/rawtransaction.cpp

ryanofsky · 2018-12-03T21:54:57Z

src/rpc/util.h

+    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.
+
+    RPCArg(
+        const std::string& name,


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

RPCArg(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.

ryanofsky · 2018-12-03T21:59:24Z

src/rpc/util.h

-
-    RPCArg(const std::string& name, const Type& type, const bool optional, const std::string& oneline_description = "")
-        : m_name{name}, m_type{type}, m_optional{optional}, m_oneline_description{oneline_description}
+    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.


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

ryanofsky · 2018-12-03T22:00:48Z

src/rpc/util.h

-private:
+    std::string ToString(bool oneline = false) const;
    std::string ToStringObj() const;
+    std::string ToDescriptionString(bool implicitly_required = false) const;


Can you add a comment saying what implicitly_required means?

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.

ryanofsky · 2018-12-03T22:05:56Z

src/rpc/util.cpp

+    std::vector<Section> m_sections;
+    size_t m_max_pad{0};
+
+    void PushSection(const Section& s)


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.

ryanofsky · 2018-12-03T22:14:54Z

src/rpc/util.h

    const std::string m_name; //!< The name of the arg (can be empty for inner args)
    const Type m_type;
    const std::vector<RPCArg> m_inner; //!< Only used for arrays or dicts
    const bool m_optional;


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.

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:

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

Also, add doxygen comment to ToDescriptionString

ryanofsky

utACK fabca42. 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.

fabca42 RPCHelpMan: Add space after colons in extended description (MarcoFalke) fafd040 rpc: Add description to fundrawtransaction vout_index (MarcoFalke) 1db0096 rpc: Pass argument descriptions to RPCHelpMan (MarcoFalke) Pull request description: This will normalize the type names and formatting for the rpc arguments Tree-SHA512: 6ab344882f0fed36046ab4636cb2fa5d2479c6aae22666ca9a0d067edbb9eff8de98010ad97c8ce40ab532d15d1ae67120a561b0bf3da837090d7de427679f4f

Summary: This tree in favor of D5571. Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs abcPatched ./checkrpc.sh RPCs master abcPatched D5700diff Verify the changes in `D5700diff`. Scripts: {F4246191} {F4246190} D5700diff: {F4246192} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5700

Summary: Application of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5725diff Verify there is no `D5725diff`. Scripts: {F4246191} {F4246190} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5725

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5710diff Verify there is no `D5710diff`. Scripts: {F4246191} {F4246190} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5710

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5712diff Verify the changes in `D5712diff`. Scripts: {F4246191} {F4246190} D5712diff: {F4249393} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5712

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5716diff Verify the changes in `D5716diff`. Scripts: {F4246191} {F4246190} D5716diff: {F4249397} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5716

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5713diff Verify the changes in `D5713diff`. Scripts: {F4246191} {F4246190} D5713diff: {F4249395} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5713

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5720diff.txt Verify the changes in `D5720diff.txt`. Scripts: {F4246191} {F4246190} D5720diff: {F4250011} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5720

Summary: Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched D5717diff Verify the changes in `D5717diff`. Scripts: {F4246191} {F4246190} D5717diff: {F4249378} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5717

…elpMan Summary: This is the first of two patches resulting from breaking the changes to `rpcwallet.cpp` into smaller parts. Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched rpcwallethelp.txt Verify the changes in `rpcwallethelp.txt`. Scripts: {F4246191} {F4246190} D5722diff: {F4250088} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5732

…tions to RPCHelpMan Summary: The last of two patches resulting from breaking the changes to `rpcwallet.cpp` into smaller parts. Partial backport of Core [[bitcoin/bitcoin#14796 | PR14796]] Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched rpcwallethelp.txt Verify the changes in `rpcwallethelp.txt`. Scripts: {F4246191} {F4246190} D5722diff: {F4250089} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5722

Summary: Finish [[bitcoin/bitcoin#14796 | PR14796]] by removing and/or renaming old functions in `rpc/util`. Renaming done with `sed "s/ToStringWithArgs/ToString/g" <files>` Depends on D5710, D5712, D5713, D5714. D5716, D5717, D5720, D5722, D5725, D5732 Test Plan: mkdir master mkdir patched On master: ninja ./bitcoind ./bitcoin-cli help > RPCs ./getrpchelps.sh RPCs master On the patched branch: ninja ./bitcoind ./getrpchelps.sh RPCs patched ./checkrpc.sh RPCs master patched finisheddiff Verify there is no `finisheddiff`. Scripts: {F4246191} {F4246190} Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5726

maflcko added RPC/REST/ZMQ Refactoring Docs labels Nov 23, 2018

maflcko force-pushed the Mf1810-rpcHelpMan branch 4 times, most recently from 228d4b2 to 1609da6 Compare November 25, 2018 02:09

practicalswift reviewed Nov 25, 2018

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Nov 25, 2018

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1810-rpcHelpMan branch 2 times, most recently from b3037c3 to 438b14e Compare November 26, 2018 16:08

maflcko mentioned this pull request Nov 26, 2018

doc: clarify RPC rawtransaction documentation #14808

Merged

DrahtBot added the Needs rebase label Nov 26, 2018

maflcko force-pushed the Mf1810-rpcHelpMan branch from 438b14e to 69096b7 Compare November 26, 2018 16:52

DrahtBot added Needs rebase and removed Needs rebase labels Nov 26, 2018

rpc: Pass argument descriptions to RPCHelpMan

1db0096

maflcko force-pushed the Mf1810-rpcHelpMan branch from 69096b7 to 1db0096 Compare November 27, 2018 19:18

DrahtBot removed the Needs rebase label Nov 27, 2018

maflcko mentioned this pull request Dec 3, 2018

Use RPCHelpMan for all RPCs #14726

Merged

ryanofsky approved these changes Dec 3, 2018

View reviewed changes

maflcko force-pushed the Mf1810-rpcHelpMan branch from eb9ad5c to ce5fa56 Compare December 4, 2018 17:06

rpc: Add description to fundrawtransaction vout_index

fafd040

maflcko force-pushed the Mf1810-rpcHelpMan branch from ce5fa56 to fafd040 Compare December 4, 2018 17:06

RPCHelpMan: Add space after colons in extended description

fabca42

Also, add doxygen comment to ToDescriptionString

maflcko force-pushed the Mf1810-rpcHelpMan branch from fae9f4b to fabca42 Compare December 4, 2018 18:48

ryanofsky approved these changes Dec 4, 2018

View reviewed changes

maflcko merged commit fabca42 into bitcoin:master Dec 5, 2018

maflcko deleted the Mf1810-rpcHelpMan branch December 5, 2018 16:07

jnewbery mentioned this pull request Dec 10, 2018

rpc help helper class #14378

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

rpc: Pass argument descriptions to RPCHelpMan #14796

rpc: Pass argument descriptions to RPCHelpMan #14796

Uh oh!

Conversation

maflcko commented Nov 23, 2018

Uh oh!

promag commented Nov 24, 2018

Uh oh!

DrahtBot commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • 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!

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!

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!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Nov 24, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading