From 13525e0c248eab9b199583cde76430c6da2426e2 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 1 Sep 2023 17:46:23 +0100 Subject: [PATCH 1/3] rpc: add arg helper unit test Compare the results of self.Arg with the request.params accessors to ensure they behave the same way. --- src/test/rpc_tests.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 0d2460c6064..934a099661b 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -581,4 +581,58 @@ BOOST_AUTO_TEST_CASE(help_example) BOOST_CHECK_NE(HelpExampleRpcNamed("foo", {{"arg", true}}), HelpExampleRpcNamed("foo", {{"arg", "true"}})); } +static void CheckRpc(const std::vector& params, const UniValue& args, RPCHelpMan::RPCMethodImpl test_impl) +{ + auto null_result{RPCResult{RPCResult::Type::NONE, "", "None"}}; + const RPCHelpMan rpc{"dummy", "dummy description", params, null_result, RPCExamples{""}, test_impl}; + JSONRPCRequest req; + req.params = args; + + rpc.HandleRequest(req); +} + +BOOST_AUTO_TEST_CASE(rpc_arg_helper) +{ + constexpr bool DEFAULT_BOOL = true; + constexpr auto DEFAULT_STRING = "default"; + constexpr uint64_t DEFAULT_UINT64_T = 3; + + //! Parameters with which the RPCHelpMan is instantiated + const std::vector params{ + // Required arg + {"req_int", RPCArg::Type::NUM, RPCArg::Optional::NO, ""}, + {"req_str", RPCArg::Type::STR, RPCArg::Optional::NO, ""}, + // Default arg + {"def_uint64_t", RPCArg::Type::NUM, RPCArg::Default{DEFAULT_UINT64_T}, ""}, + {"def_string", RPCArg::Type::STR, RPCArg::Default{DEFAULT_STRING}, ""}, + {"def_bool", RPCArg::Type::BOOL, RPCArg::Default{DEFAULT_BOOL}, ""}, + // Optional arg without default + {"opt_double", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, ""}, + {"opt_string", RPCArg::Type::STR, RPCArg::Optional::OMITTED, ""} + }; + + //! Check that `self.Arg` returns the same value as the `request.params` accessors + RPCHelpMan::RPCMethodImpl check_positional = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + BOOST_CHECK_EQUAL(self.Arg(0), request.params[0].getInt()); + BOOST_CHECK_EQUAL(self.Arg(1), request.params[1].get_str()); + BOOST_CHECK_EQUAL(self.Arg(2), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt()); + BOOST_CHECK_EQUAL(self.Arg(3), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str()); + BOOST_CHECK_EQUAL(self.Arg(4), request.params[4].isNull() ? DEFAULT_BOOL : request.params[4].get_bool()); + if (!request.params[5].isNull()) { + BOOST_CHECK_EQUAL(self.MaybeArg(5).value(), request.params[5].get_real()); + } else { + BOOST_CHECK(!self.MaybeArg(5)); + } + if (!request.params[6].isNull()) { + BOOST_CHECK(self.MaybeArg(6)); + BOOST_CHECK_EQUAL(*self.MaybeArg(6), request.params[6].get_str()); + } else { + BOOST_CHECK(!self.MaybeArg(6)); + } + return UniValue{}; + }; + CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_positional); + CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_positional); +} + BOOST_AUTO_TEST_SUITE_END() From bbb31269bfa449e82d3b6a20c2c3481fb3dcc316 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 18 Jan 2024 17:27:36 +0000 Subject: [PATCH 2/3] rpc: add named arg helper Overload the Arg and MaybeArg helpers to allow accessing arguments by name as well. Also update the docs to document Arg and MaybeArg separately --- src/rpc/util.cpp | 12 ++++++++ src/rpc/util.h | 62 +++++++++++++++++++++++++++++++++++------- src/test/rpc_tests.cpp | 14 ++++++++++ 3 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 51c88cc1ba5..c90456fe57f 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -24,6 +24,8 @@ #include #include +#include +#include #include #include @@ -728,6 +730,16 @@ std::vector> RPCHelpMan::GetArgNames() const return ret; } +size_t RPCHelpMan::GetParamIndex(std::string_view key) const +{ + auto it{std::find_if( + m_args.begin(), m_args.end(), [&key](const auto& arg) { return arg.GetName() == key;} + )}; + + CHECK_NONFATAL(it != m_args.end()); // TODO: ideally this is checked at compile time + return std::distance(m_args.begin(), it); +} + std::string RPCHelpMan::ToString() const { std::string ret; diff --git a/src/rpc/util.h b/src/rpc/util.h index ad3ed97b2e9..6b35922d072 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -402,18 +402,25 @@ public: UniValue HandleRequest(const JSONRPCRequest& request) const; /** - * Helper to get a request argument. - * This function only works during m_fun(), i.e. it should only be used in - * RPC method implementations. The helper internally checks whether the - * user-passed argument isNull() and parses (from JSON) and returns the - * user-passed argument, or the default value derived from the RPCArg - * documentation, or a falsy value if no default was given. + * @brief Helper to get a required or default-valued request argument. * - * Use Arg(i) to get the argument or its default value. Otherwise, - * use MaybeArg(i) to get the optional argument or a falsy value. + * Use this function when the argument is required or when it has a default value. If the + * argument is optional and may not be provided, use MaybeArg instead. * - * The Type passed to this helper must match the corresponding - * RPCArg::Type. + * This function only works during m_fun(), i.e., it should only be used in + * RPC method implementations. It internally checks whether the user-passed + * argument isNull() and parses (from JSON) and returns the user-passed argument, + * or the default value derived from the RPCArg documentation. + * + * There are two overloads of this function: + * - Use Arg(size_t i) to get the argument (or the default value) by index. + * - Use Arg(const std::string& key) to get the argument (or the default value) by key. + * + * The Type passed to this helper must match the corresponding RPCArg::Type. + * + * @return The value of the RPC argument (or the default value) cast to type Type. + * + * @see MaybeArg for handling optional arguments without default values. */ template auto Arg(size_t i) const @@ -427,6 +434,34 @@ public: return ArgValue(i); } } + template + auto Arg(std::string_view key) const + { + return Arg(GetParamIndex(key)); + } + /** + * @brief Helper to get an optional request argument. + * + * Use this function when the argument is optional and does not have a default value. If the + * argument is required or has a default value, use Arg instead. + * + * This function only works during m_fun(), i.e., it should only be used in + * RPC method implementations. It internally checks whether the user-passed + * argument isNull() and parses (from JSON) and returns the user-passed argument, + * or a falsy value if no argument was passed. + * + * There are two overloads of this function: + * - Use MaybeArg(size_t i) to get the optional argument by index. + * - Use MaybeArg(const std::string& key) to get the optional argument by key. + * + * The Type passed to this helper must match the corresponding RPCArg::Type. + * + * @return For integral and floating-point types, a std::optional is returned. + * For other types, a Type* pointer to the argument is returned. If the + * argument is not provided, std::nullopt or a null pointer is returned. + * + * @see Arg for handling arguments that are required or have a default value. + */ template auto MaybeArg(size_t i) const { @@ -439,6 +474,11 @@ public: return ArgValue(i); } } + template + auto MaybeArg(std::string_view key) const + { + return MaybeArg(GetParamIndex(key)); + } std::string ToString() const; /** Return the named args that need to be converted from string to another JSON type */ UniValue GetArgMap() const; @@ -458,6 +498,8 @@ private: mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun() template R ArgValue(size_t i) const; + //! Return positional index of a parameter using its name as key. + size_t GetParamIndex(std::string_view key) const; }; /** diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 934a099661b..9926fdfdcbd 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -633,6 +633,20 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper) }; CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_positional); CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_positional); + + //! Check that `self.Arg` returns the same value when using index and key + RPCHelpMan::RPCMethodImpl check_named = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + BOOST_CHECK_EQUAL(self.Arg(0), self.Arg("req_int")); + BOOST_CHECK_EQUAL(self.Arg(1), self.Arg("req_str")); + BOOST_CHECK_EQUAL(self.Arg(2), self.Arg("def_uint64_t")); + BOOST_CHECK_EQUAL(self.Arg(3), self.Arg("def_string")); + BOOST_CHECK_EQUAL(self.Arg(4), self.Arg("def_bool")); + BOOST_CHECK(self.MaybeArg(5) == self.MaybeArg("opt_double")); + BOOST_CHECK(self.MaybeArg(6) == self.MaybeArg("opt_string")); + return UniValue{}; + }; + CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_named); + CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_named); } BOOST_AUTO_TEST_SUITE_END() From 30a6c999351041d6a1e8712a9659be1296a1b46a Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 18 Jan 2024 17:27:50 +0000 Subject: [PATCH 3/3] rpc: access some args by name Use the new key-based Arg helper in a few locations to show how it is used. --- src/rpc/blockchain.cpp | 9 +++++---- src/rpc/mining.cpp | 8 ++++---- src/rpc/net.cpp | 6 +++--- src/rpc/signmessage.cpp | 6 +++--- src/wallet/rpc/coins.cpp | 7 ++----- 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 50908e9f968..2f6e5c42179 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2149,7 +2149,8 @@ static RPCHelpMan scantxoutset() [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { UniValue result(UniValue::VOBJ); - if (request.params[0].get_str() == "status") { + const auto action{self.Arg("action")}; + if (action == "status") { CoinsViewScanReserver reserver; if (reserver.reserve()) { // no scan in progress @@ -2157,7 +2158,7 @@ static RPCHelpMan scantxoutset() } result.pushKV("progress", g_scan_progress.load()); return result; - } else if (request.params[0].get_str() == "abort") { + } else if (action == "abort") { CoinsViewScanReserver reserver; if (reserver.reserve()) { // reserve was possible which means no scan was running @@ -2166,7 +2167,7 @@ static RPCHelpMan scantxoutset() // set the abort flag g_should_abort_scan = true; return true; - } else if (request.params[0].get_str() == "start") { + } else if (action == "start") { CoinsViewScanReserver reserver; if (!reserver.reserve()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" or \"status\""); @@ -2235,7 +2236,7 @@ static RPCHelpMan scantxoutset() result.pushKV("unspents", unspents); result.pushKV("total_amount", ValueFromAmount(total_in)); } else { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", request.params[0].get_str())); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", action)); } return result; }, diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ff39a31a438..bb50de34899 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -122,7 +122,7 @@ static RPCHelpMan getnetworkhashps() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetNetworkHashPS(self.Arg(0), self.Arg(1), chainman.ActiveChain()); + return GetNetworkHashPS(self.Arg("nblocks"), self.Arg("height"), chainman.ActiveChain()); }, }; } @@ -229,12 +229,12 @@ static RPCHelpMan generatetodescriptor() "\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const auto num_blocks{self.Arg(0)}; - const auto max_tries{self.Arg(2)}; + const auto num_blocks{self.Arg("num_blocks")}; + const auto max_tries{self.Arg("maxtries")}; CScript coinbase_script; std::string error; - if (!getScriptFromDescriptor(self.Arg(1), coinbase_script, error)) { + if (!getScriptFromDescriptor(self.Arg("descriptor"), coinbase_script, error)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error); } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 5e6f42b5965..666d1586b86 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -322,7 +322,7 @@ static RPCHelpMan addnode() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - const std::string command{request.params[1].get_str()}; + const auto command{self.Arg("command")}; if (command != "onetry" && command != "add" && command != "remove") { throw std::runtime_error( self.ToString()); @@ -331,9 +331,9 @@ static RPCHelpMan addnode() NodeContext& node = EnsureAnyNodeContext(request.context); CConnman& connman = EnsureConnman(node); - const std::string node_arg{request.params[0].get_str()}; + const auto node_arg{self.Arg("node")}; bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2; - bool use_v2transport = self.MaybeArg(2).value_or(node_v2transport); + bool use_v2transport = self.MaybeArg("v2transport").value_or(node_v2transport); if (use_v2transport && !node_v2transport) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: v2transport requested but not enabled (see -v2transport)"); diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp index 8c752ba1fda..9f3c24efcfb 100644 --- a/src/rpc/signmessage.cpp +++ b/src/rpc/signmessage.cpp @@ -38,9 +38,9 @@ static RPCHelpMan verifymessage() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::string strAddress = request.params[0].get_str(); - std::string strSign = request.params[1].get_str(); - std::string strMessage = request.params[2].get_str(); + std::string strAddress = self.Arg("address"); + std::string strSign = self.Arg("signature"); + std::string strMessage = self.Arg("message"); switch (MessageVerify(strAddress, strSign, strMessage)) { case MessageVerificationResult::ERR_INVALID_ADDRESS: diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 0cb0891141b..b6c7396f4ba 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -194,15 +194,12 @@ RPCHelpMan getbalance() LOCK(pwallet->cs_wallet); - const auto dummy_value{self.MaybeArg(0)}; + const auto dummy_value{self.MaybeArg("dummy")}; if (dummy_value && *dummy_value != "*") { throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\"."); } - int min_depth = 0; - if (!request.params[1].isNull()) { - min_depth = request.params[1].getInt(); - } + const auto min_depth{self.Arg("minconf")}; bool include_watchonly = ParseIncludeWatchonly(request.params[2], *pwallet);