Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Pass argument descriptions to RPCHelpMan #14796

Merged
merged 3 commits into from Dec 5, 2018

Conversation

Projects
None yet
5 participants
@MarcoFalke
Copy link
Member

commented Nov 23, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

👏

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2018

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 MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 4 times, most recently Nov 25, 2018

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch 2 times, most recently Nov 26, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Nov 26, 2018

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch to 1db0096 Nov 27, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 27, 2018

@ryanofsky
Copy link
Contributor

left a comment

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

Show resolved Hide resolved src/wallet/rpcwallet.cpp
}}
.ToString() +
"Arguments:\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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 comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 4, 2018

Author Member

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.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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 comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 4, 2018

Author Member

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.

Show resolved Hide resolved src/rpc/rawtransaction.cpp
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,

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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.


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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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

std::string ToStringObj() const;
std::string ToDescriptionString(bool implicitly_required = false) const;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

Can you add a comment saying what implicitly_required means?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 4, 2018

Author Member

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.
std::vector<Section> m_sections;
size_t m_max_pad{0};

void PushSection(const Section& s)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 3, 2018

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 4, 2018

Author Member

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);

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch Dec 4, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch to fafd040 Dec 4, 2018

RPCHelpMan: Add space after colons in extended description
Also, add doxygen comment to ToDescriptionString

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-rpcHelpMan branch to fabca42 Dec 4, 2018

@ryanofsky
Copy link
Contributor

left a comment

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.

@MarcoFalke MarcoFalke merged commit fabca42 into bitcoin:master Dec 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 5, 2018

Merge #14796: rpc: Pass argument descriptions to RPCHelpMan
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

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1810-rpcHelpMan branch Dec 5, 2018

@jnewbery jnewbery referenced this pull request Dec 10, 2018

Closed

rpc help helper class #14378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.