Skip to content

doc: Properly report optional RPC args #26706

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 18, 2023
Merged

doc: Properly report optional RPC args #26706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fab92a5
refactor: Remove const to fix performance-move-const-arg clang-tidy e…
Jan 3, 2023
fa09cb6
refactor: Introduce is_top_level_arg
Jan 3, 2023
fad56f7
doc: Properly report optional RPC args
Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions src/rpc/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ struct Sections {
const auto indent = std::string(current_indent, ' ');
const auto indent_next = std::string(current_indent + 2, ' ');
const bool push_name{outer_type == OuterType::OBJ}; // Dictionary keys must have a name
const bool is_top_level_arg{outer_type == OuterType::NONE}; // True on the first recursion

switch (arg.m_type) {
case RPCArg::Type::STR_HEX:
Expand All @@ -396,41 +397,41 @@ struct Sections {
case RPCArg::Type::AMOUNT:
case RPCArg::Type::RANGE:
case RPCArg::Type::BOOL: {
if (outer_type == OuterType::NONE) return; // Nothing more to do for non-recursive types on first recursion
if (is_top_level_arg) return; // Nothing more to do for non-recursive types on first recursion
auto left = indent;
if (arg.m_opts.type_str.size() != 0 && push_name) {
left += "\"" + arg.GetName() + "\": " + arg.m_opts.type_str.at(0);
} else {
left += push_name ? arg.ToStringObj(/*oneline=*/false) : arg.ToString(/*oneline=*/false);
}
left += ",";
PushSection({left, arg.ToDescriptionString()});
PushSection({left, arg.ToDescriptionString(/*is_named_arg=*/push_name)});
break;
}
case RPCArg::Type::OBJ:
case RPCArg::Type::OBJ_USER_KEYS: {
const auto right = outer_type == OuterType::NONE ? "" : arg.ToDescriptionString();
const auto right = is_top_level_arg ? "" : arg.ToDescriptionString(/*is_named_arg=*/push_name);
PushSection({indent + (push_name ? "\"" + arg.GetName() + "\": " : "") + "{", right});
for (const auto& arg_inner : arg.m_inner) {
Push(arg_inner, current_indent + 2, OuterType::OBJ);
}
if (arg.m_type != RPCArg::Type::OBJ) {
PushSection({indent_next + "...", ""});
}
PushSection({indent + "}" + (outer_type != OuterType::NONE ? "," : ""), ""});
PushSection({indent + "}" + (is_top_level_arg ? "" : ","), ""});
break;
}
case RPCArg::Type::ARR: {
auto left = indent;
left += push_name ? "\"" + arg.GetName() + "\": " : "";
left += "[";
const auto right = outer_type == OuterType::NONE ? "" : arg.ToDescriptionString();
const auto right = is_top_level_arg ? "" : arg.ToDescriptionString(/*is_named_arg=*/push_name);
PushSection({left, right});
for (const auto& arg_inner : arg.m_inner) {
Push(arg_inner, current_indent + 2, OuterType::ARR);
}
PushSection({indent_next + "...", ""});
PushSection({indent + "]" + (outer_type != OuterType::NONE ? "," : ""), ""});
PushSection({indent + "]" + (is_top_level_arg ? "" : ","), ""});
break;
}
} // no default case, so the compiler can warn about missing cases
Expand Down Expand Up @@ -627,7 +628,7 @@ std::string RPCHelpMan::ToString() const
if (i == 0) ret += "\nArguments:\n";

// Push named argument name and description
sections.m_sections.emplace_back(::ToString(i + 1) + ". " + arg.GetFirstName(), arg.ToDescriptionString());
sections.m_sections.emplace_back(::ToString(i + 1) + ". " + arg.GetFirstName(), arg.ToDescriptionString(/*is_named_arg=*/true));
sections.m_max_pad = std::max(sections.m_max_pad, sections.m_sections.back().m_left.size());

// Recursively push nested args
Expand Down Expand Up @@ -721,7 +722,7 @@ bool RPCArg::IsOptional() const
}
}

std::string RPCArg::ToDescriptionString() const
std::string RPCArg::ToDescriptionString(bool is_named_arg) const
{
std::string ret;
ret += "(";
Expand Down Expand Up @@ -767,14 +768,12 @@ std::string RPCArg::ToDescriptionString() const
ret += ", optional, default=" + std::get<RPCArg::Default>(m_fallback).write();
} else {
switch (std::get<RPCArg::Optional>(m_fallback)) {
case RPCArg::Optional::OMITTED_NAMED_ARG: // Deprecated alias for OMITTED, can be removed
case RPCArg::Optional::OMITTED: {
if (is_named_arg) ret += ", optional"; // Default value is "null" in dicts. Otherwise,
// nothing to do. Element is treated as if not present and has no default value
break;
}
case RPCArg::Optional::OMITTED_NAMED_ARG: {
ret += ", optional"; // Default value is "null"
break;
}
case RPCArg::Optional::NO: {
ret += ", required";
break;
Expand Down
79 changes: 41 additions & 38 deletions src/rpc/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,21 +154,24 @@ struct RPCArg {
/** Required arg */
NO,
/**
* The arg is optional for one of two reasons:
*
* Optional arg that is a named argument and has a default value of
* `null`. When possible, the default value should be specified.
*/
OMITTED_NAMED_ARG,
/**
* `null`.
*
* Optional argument with default value omitted because they are
* implicitly clear. That is, elements in an array or object may not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that "or object" was removed from here? My understanding is that RPCArg::Optional::OMITTED still applies to elements in objects, such as here

bitcoin/src/wallet/rpc/backup.cpp

Lines 1258 to 1293 in 392dc68

{"requests", RPCArg::Type::ARR, RPCArg::Optional::NO, "Data to be imported",
{
{"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
{
{"desc", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Descriptor to import. If using descriptor, do not also provide address/scriptPubKey, scripts, or pubkeys"},
{"scriptPubKey", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of scriptPubKey (string for script, json for address). Should not be provided if using a descriptor",
RPCArgOptions{.type_str={"\"<script>\" | { \"address\":\"<address>\" }", "string / json"}}
},
{"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, "Creation time of the key expressed in " + UNIX_EPOCH_TIME + ",\n"
"or the string \"now\" to substitute the current synced blockchain time. The timestamp of the oldest\n"
"key will determine how far back blockchain rescans need to begin for missing wallet transactions.\n"
"\"now\" can be specified to bypass scanning, for keys which are known to never have been used, and\n"
"0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest key\n"
"creation time of all keys being imported by the importmulti call will be scanned.",
RPCArgOptions{.type_str={"timestamp | \"now\"", "integer / string"}}
},
{"redeemscript", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Allowed only if the scriptPubKey is a P2SH or P2SH-P2WSH address/scriptPubKey"},
{"witnessscript", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Allowed only if the scriptPubKey is a P2SH-P2WSH or P2WSH address/scriptPubKey"},
{"pubkeys", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Array of strings giving pubkeys to import. They must occur in P2PKH or P2WPKH scripts. They are not required when the private key is also provided (see the \"keys\" argument).",
{
{"pubKey", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""},
}
},
{"keys", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Array of strings giving private keys to import. The corresponding public keys must occur in the output or redeemscript.",
{
{"key", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""},
}
},
{"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED, "If a ranged descriptor is used, this specifies the end or the range (in the form [begin,end]) to import"},
{"internal", RPCArg::Type::BOOL, RPCArg::Default{false}, "Stating whether matching outputs should be treated as not incoming payments (also known as change)"},
{"watchonly", RPCArg::Type::BOOL, RPCArg::Default{false}, "Stating whether matching outputs should be considered watchonly."},
{"label", RPCArg::Type::STR, RPCArg::Default{""}, "Label to assign to the address, only allowed with internal=false"},
{"keypool", RPCArg::Type::BOOL, RPCArg::Default{false}, "Stating whether imported public keys should be added to the keypool for when users request new addresses. Only allowed when wallet private keys are disabled"},
},
},
},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"or object" is covered by the first of the two reasons:

Optional arg that is a named argument and has a default value of null

(named arguments are basically keys in a dictionary/object)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, the root of my confusion was the definition of named arguments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are improvements, they can be appended to #26919, which is a follow-up

* implicitly clear. That is, elements in an array may not
* exist by default.
* When possible, the default value should be specified.
*/
OMITTED,
OMITTED_NAMED_ARG, // Deprecated alias for OMITTED, can be removed
};
/** Hint for default value */
using DefaultHint = std::string;
/** Default constant value */
using Default = UniValue;
using Fallback = std::variant<Optional, /* hint for default value */ DefaultHint, /* default constant value */ Default>;
using Fallback = std::variant<Optional, DefaultHint, Default>;

const std::string m_names; //!< The name of the arg (can be empty for inner args, can contain multiple aliases separated by | for named request arguments)
const Type m_type;
Expand All @@ -178,10 +181,10 @@ struct RPCArg {
const RPCArgOptions m_opts;

RPCArg(
const std::string name,
const Type type,
const Fallback fallback,
const std::string description,
std::string name,
Type type,
Fallback fallback,
std::string description,
RPCArgOptions opts = {})
: m_names{std::move(name)},
m_type{std::move(type)},
Expand All @@ -193,11 +196,11 @@ struct RPCArg {
}

RPCArg(
const std::string name,
const Type type,
const Fallback fallback,
const std::string description,
const std::vector<RPCArg> inner,
std::string name,
Type type,
Fallback fallback,
std::string description,
std::vector<RPCArg> inner,
RPCArgOptions opts = {})
: m_names{std::move(name)},
m_type{std::move(type)},
Expand Down Expand Up @@ -234,7 +237,7 @@ struct RPCArg {
* Return the description string, including the argument type and whether
* the argument is required.
*/
std::string ToDescriptionString() const;
std::string ToDescriptionString(bool is_named_arg) const;
};

struct RPCResult {
Expand Down Expand Up @@ -263,12 +266,12 @@ struct RPCResult {
const std::string m_cond;

RPCResult(
const std::string cond,
const Type type,
const std::string m_key_name,
const bool optional,
const std::string description,
const std::vector<RPCResult> inner = {})
std::string cond,
Type type,
std::string m_key_name,
bool optional,
std::string description,
std::vector<RPCResult> inner = {})
: m_type{std::move(type)},
m_key_name{std::move(m_key_name)},
m_inner{std::move(inner)},
Expand All @@ -282,19 +285,19 @@ struct RPCResult {
}

RPCResult(
const std::string cond,
const Type type,
const std::string m_key_name,
const std::string description,
const std::vector<RPCResult> inner = {})
: RPCResult{cond, type, m_key_name, false, description, inner} {}
std::string cond,
Type type,
std::string m_key_name,
std::string description,
std::vector<RPCResult> inner = {})
: RPCResult{std::move(cond), type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner)} {}

RPCResult(
const Type type,
const std::string m_key_name,
const bool optional,
const std::string description,
const std::vector<RPCResult> inner = {},
Type type,
std::string m_key_name,
bool optional,
std::string description,
std::vector<RPCResult> inner = {},
bool skip_type_check = false)
: m_type{std::move(type)},
m_key_name{std::move(m_key_name)},
Expand All @@ -308,12 +311,12 @@ struct RPCResult {
}

RPCResult(
const Type type,
const std::string m_key_name,
const std::string description,
const std::vector<RPCResult> inner = {},
Type type,
std::string m_key_name,
std::string description,
std::vector<RPCResult> inner = {},
bool skip_type_check = false)
: RPCResult{type, m_key_name, false, description, inner, skip_type_check} {}
: RPCResult{type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner), skip_type_check} {}

/** Append the sections of the result. */
void ToSections(Sections& sections, OuterType outer_type = OuterType::NONE, const int current_indent = 0) const;
Expand Down