diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/rpc/util.cpp | 24 | ||||
| -rw-r--r-- | src/rpc/util.h | 9 | ||||
| -rw-r--r-- | src/test/rpc_tests.cpp | 47 | ||||
| -rw-r--r-- | src/wallet/rpc/spend.cpp | 8 |
4 files changed, 82 insertions, 6 deletions
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 2fa1732faa..dd6b25fba5 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -486,12 +486,32 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP m_results{std::move(results)}, m_examples{std::move(examples)} { - std::set<std::string> named_args; + // Map of parameter names and types just used to check whether the names are + // unique. Parameter names always need to be unique, with the exception that + // there can be pairs of POSITIONAL and NAMED parameters with the same name. + enum ParamType { POSITIONAL = 1, NAMED = 2, NAMED_ONLY = 4 }; + std::map<std::string, int> param_names; + for (const auto& arg : m_args) { std::vector<std::string> names = SplitString(arg.m_names, '|'); // Should have unique named arguments for (const std::string& name : names) { - CHECK_NONFATAL(named_args.insert(name).second); + auto& param_type = param_names[name]; + CHECK_NONFATAL(!(param_type & POSITIONAL)); + CHECK_NONFATAL(!(param_type & NAMED_ONLY)); + param_type |= POSITIONAL; + } + if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + for (const auto& inner : arg.m_inner) { + std::vector<std::string> inner_names = SplitString(inner.m_names, '|'); + for (const std::string& inner_name : inner_names) { + auto& param_type = param_names[inner_name]; + CHECK_NONFATAL(!(param_type & POSITIONAL) || inner.m_opts.also_positional); + CHECK_NONFATAL(!(param_type & NAMED)); + CHECK_NONFATAL(!(param_type & NAMED_ONLY)); + param_type |= inner.m_opts.also_positional ? NAMED : NAMED_ONLY; + } + } } // Default value type should match argument type only when defined if (arg.m_fallback.index() == 2) { diff --git a/src/rpc/util.h b/src/rpc/util.h index d017812e97..b10307e314 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -130,6 +130,15 @@ struct RPCArgOptions { std::string oneline_description{}; //!< Should be empty unless it is supposed to override the auto-generated summary line std::vector<std::string> type_str{}; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_opts.type_str.at(0) will override the type of the value in a key-value pair, m_opts.type_str.at(1) will override the type in the argument description. bool hidden{false}; //!< For testing only + bool also_positional{false}; //!< If set allows a named-parameter field in an OBJ_NAMED_PARAM options object + //!< to have the same name as a top-level parameter. By default the RPC + //!< framework disallows this, because if an RPC request passes the value by + //!< name, it is assigned to top-level parameter position, not to the options + //!< position, defeating the purpose of using OBJ_NAMED_PARAMS instead OBJ for + //!< that option. But sometimes it makes sense to allow less-commonly used + //!< options to be passed by name only, and more commonly used options to be + //!< passed by name or position, so the RPC framework allows this as long as + //!< methods set the also_positional flag and read values from both positions. }; struct RPCArg { diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index f8975dd9b2..7610d4fa39 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -528,6 +528,53 @@ BOOST_AUTO_TEST_CASE(rpc_getblockstats_calculate_percentiles_by_weight) } } +// Make sure errors are triggered appropriately if parameters have the same names. +BOOST_AUTO_TEST_CASE(check_dup_param_names) +{ + enum ParamType { POSITIONAL, NAMED, NAMED_ONLY }; + auto make_rpc = [](std::vector<std::tuple<std::string, ParamType>> param_names) { + std::vector<RPCArg> params; + std::vector<RPCArg> options; + auto push_options = [&] { if (!options.empty()) params.emplace_back(RPCArg{strprintf("options%i", params.size()), RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", std::move(options)}); }; + for (auto& [param_name, param_type] : param_names) { + if (param_type == POSITIONAL) { + push_options(); + params.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description"); + } else { + options.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description", RPCArgOptions{.also_positional = param_type == NAMED}); + } + } + push_options(); + return RPCHelpMan{"method_name", "description", params, RPCResults{}, RPCExamples{""}}; + }; + + // No errors if parameter names are unique. + make_rpc({{"p1", POSITIONAL}, {"p2", POSITIONAL}}); + make_rpc({{"p1", POSITIONAL}, {"p2", NAMED}}); + make_rpc({{"p1", POSITIONAL}, {"p2", NAMED_ONLY}}); + make_rpc({{"p1", NAMED}, {"p2", POSITIONAL}}); + make_rpc({{"p1", NAMED}, {"p2", NAMED}}); + make_rpc({{"p1", NAMED}, {"p2", NAMED_ONLY}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", POSITIONAL}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}}); + make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}}); + + // Error if parameters names are duplicates, unless one parameter is + // positional and the other is named and .also_positional is true. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); + make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + + // Make sure duplicate aliases are detected too. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); +} + BOOST_AUTO_TEST_CASE(help_example) { // test different argument types diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 3f98f93a90..5945c66000 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -453,9 +453,9 @@ RPCHelpMan settxfee() static std::vector<RPCArg> FundTxDoc(bool solving_data = true) { std::vector<RPCArg> args = { - {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, + {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks", RPCArgOptions{.also_positional = true}}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" - "\"" + FeeModes("\"\n\"") + "\""}, + "\"" + FeeModes("\"\n\"") + "\"", RPCArgOptions{.also_positional = true}}, { "replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Marks this transaction as BIP125-replaceable.\n" "Allows this transaction to be replaced by a transaction with higher fees" @@ -1200,7 +1200,7 @@ RPCHelpMan send() {"change_address", RPCArg::Type::STR, RPCArg::DefaultHint{"automatic"}, "The bitcoin address to receive the change"}, {"change_position", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"}, {"change_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -changetype"}, "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", \"bech32\" and \"bech32m\"."}, - {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."}, + {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB.", RPCArgOptions{.also_positional = true}}, {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, @@ -1306,7 +1306,7 @@ RPCHelpMan sendall() Cat<std::vector<RPCArg>>( { {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"}, - {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."}, + {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB.", RPCArgOptions{.also_positional = true}}, {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch-only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, |