Merge bitcoin#29277: RPC: access RPC arguments by name

30a6c99 rpc: access some args by name (stickies-v)
bbb3126 rpc: add named arg helper (stickies-v)
13525e0 rpc: add arg helper unit test (stickies-v)

Pull request description:

  Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.

  Example usage:
  ```cpp
  const auto action{self.Arg<std::string>("action")};
  ```

  Most of the LoC is adding test coverage and documentation updates. No behaviour change.

  An alternative approach to bitcoin#27788 with significantly less overhaul.

ACKs for top commit:
  fjahr:
    Code review ACK 30a6c99
  maflcko:
    ACK 30a6c99 🥑
  ryanofsky:
    Code review ACK 30a6c99. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too.

Tree-SHA512: 4904f5f914fe1d421d32f60edb7c5a028c8ea0f140a2f207a106b4752d441164e073066a6bf2e17693f859fe847815a96609d3cf521e0ac4178d8cd09362ea3d

Author: ryanofsky

src/rpc/blockchain.cpp

@@ -2500,15 +2500,16 @@ static RPCHelpMan scantxoutset()
     RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VARR});
 
     UniValue result(UniValue::VOBJ);
-    if (request.params[0].get_str() == "status") {
+    const auto action{self.Arg<std::string>("action")};
+    if (action == "status") {
         CoinsViewScanReserver reserver;
         if (reserver.reserve()) {
             // no scan in progress
             return UniValue::VNULL;
         }
         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
@@ -2517,7 +2518,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\"");
@@ -2586,7 +2587,7 @@ static RPCHelpMan scantxoutset()
         result.pushKV("unspents", unspents);
         result.pushKV("total_amount", ValueFromAmount(total_in));
     } else {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command");
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", action));
     }
     return result;
 },

src/rpc/mining.cpp

@@ -118,7 +118,7 @@ static RPCHelpMan getnetworkhashps()
 
     ChainstateManager& chainman = EnsureAnyChainman(request.context);
     LOCK(cs_main);
-    return GetNetworkHashPS(self.Arg<int>(0), self.Arg<int>(1), chainman.ActiveChain());
+    return GetNetworkHashPS(self.Arg<int>("nblocks"), self.Arg<int>("height"), chainman.ActiveChain());
 },
     };
 }
@@ -230,12 +230,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<int>(0)};
-    const auto max_tries{self.Arg<uint64_t>(2)};
+    const auto num_blocks{self.Arg<int>("num_blocks")};
+    const auto max_tries{self.Arg<uint64_t>("maxtries")};
 
     CScript coinbase_script;
     std::string error;
-    if (!getScriptFromDescriptor(self.Arg<std::string>(1), coinbase_script, error)) {
+    if (!getScriptFromDescriptor(self.Arg<std::string>("descriptor"), coinbase_script, error)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
 

src/rpc/net.cpp

@@ -321,7 +321,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<std::string>("command")};
     if (command != "onetry" && command != "add" && command != "remove") {
         throw std::runtime_error(
             self.ToString());
@@ -330,9 +330,9 @@ static RPCHelpMan addnode()
     const 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<std::string>("node")};
     bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2;
-    bool use_v2transport = request.params[2].isNull() ? node_v2transport : request.params[2].get_bool();
+    bool use_v2transport = self.MaybeArg<bool>("v2transport").value_or(node_v2transport);
 
     if (use_v2transport && !node_v2transport) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: v2transport requested but not enabled (see -v2transport)");

src/rpc/signmessage.cpp

@@ -39,9 +39,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<std::string>("address");
+            std::string strSign = self.Arg<std::string>("signature");
+            std::string strMessage = self.Arg<std::string>("message");
 
             switch (MessageVerify(strAddress, strSign, strMessage)) {
             case MessageVerificationResult::ERR_INVALID_ADDRESS:

src/rpc/util.cpp

@@ -18,6 +18,11 @@
 #include <util/system.h>
 #include <util/translation.h>
 
+#include <algorithm>
+#include <iterator>
+#include <string_view>
+#include <tuple>
+
 const std::string UNIX_EPOCH_TIME = "UNIX epoch time";
 const std::string EXAMPLE_ADDRESS[2] = {"XunLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPw0", "XwQQkwA4FYkq2XERzMY2CiAZhJTEDAbtc0"};
 
@@ -599,6 +604,16 @@ std::vector<std::string> 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;

src/rpc/util.h

@@ -387,18 +387,25 @@ class RPCHelpMan
 
     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
-     * documention, or a falsy value if no default was given.
+     * @brief Helper to get a required or default-valued request argument.
      *
-     * Use Arg<Type>(i) to get the argument or its default value. Otherwise,
-     * use MaybeArg<Type>(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<Type>(size_t i) to get the argument (or the default value) by index.
+     * - Use Arg<Type>(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 <typename R>
     auto Arg(size_t i) const
@@ -412,6 +419,34 @@ class RPCHelpMan
             return ArgValue<const R&>(i);
         }
     }
+    template<typename R>
+    auto Arg(std::string_view key) const
+    {
+        return Arg<R>(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<Type>(size_t i) to get the optional argument by index.
+     * - Use MaybeArg<Type>(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<Type> 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 <typename R>
     auto MaybeArg(size_t i) const
     {
@@ -424,6 +459,11 @@ class RPCHelpMan
             return ArgValue<const R*>(i);
         }
     }
+    template<typename R>
+    auto MaybeArg(std::string_view key) const
+    {
+        return MaybeArg<R>(GetParamIndex(key));
+    }
     std::string ToString() const;
     /** Return the named args that need to be converted from string to another JSON type */
     UniValue GetArgMap() const;
@@ -443,6 +483,8 @@ class RPCHelpMan
     mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun()
     template <typename R>
     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;
 };
 
 RPCErrorCode RPCErrorFromTransactionError(TransactionError terr);

src/test/rpc_tests.cpp

@@ -702,4 +702,72 @@ BOOST_AUTO_TEST_CASE(rpc_convert_composite_commands)
     BOOST_CHECK_EQUAL(result[1][1].get_str(), "avgfeerate");
 }
 
+static void CheckRpc(const std::vector<RPCArg>& 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<RPCArg> 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<int>(0), request.params[0].getInt<int>());
+            BOOST_CHECK_EQUAL(self.Arg<std::string>(1), request.params[1].get_str());
+            BOOST_CHECK_EQUAL(self.Arg<uint64_t>(2), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt<uint64_t>());
+            BOOST_CHECK_EQUAL(self.Arg<std::string>(3), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str());
+            BOOST_CHECK_EQUAL(self.Arg<bool>(4), request.params[4].isNull() ? DEFAULT_BOOL : request.params[4].get_bool());
+            if (!request.params[5].isNull()) {
+                BOOST_CHECK_EQUAL(self.MaybeArg<double>(5).value(), request.params[5].get_real());
+            } else {
+                BOOST_CHECK(!self.MaybeArg<double>(5));
+            }
+            if (!request.params[6].isNull()) {
+                BOOST_CHECK(self.MaybeArg<std::string>(6));
+                BOOST_CHECK_EQUAL(*self.MaybeArg<std::string>(6), request.params[6].get_str());
+            } else {
+                BOOST_CHECK(!self.MaybeArg<std::string>(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);
+
+    //! 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<int>(0), self.Arg<int>("req_int"));
+            BOOST_CHECK_EQUAL(self.Arg<std::string>(1), self.Arg<std::string>("req_str"));
+            BOOST_CHECK_EQUAL(self.Arg<uint64_t>(2), self.Arg<uint64_t>("def_uint64_t"));
+            BOOST_CHECK_EQUAL(self.Arg<std::string>(3), self.Arg<std::string>("def_string"));
+            BOOST_CHECK_EQUAL(self.Arg<bool>(4), self.Arg<bool>("def_bool"));
+            BOOST_CHECK(self.MaybeArg<double>(5) == self.MaybeArg<double>("opt_double"));
+            BOOST_CHECK(self.MaybeArg<std::string>(6) == self.MaybeArg<std::string>("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()

src/wallet/rpc/coins.cpp

@@ -197,15 +197,12 @@ RPCHelpMan getbalance()
 
     LOCK(pwallet->cs_wallet);
 
-    const auto dummy_value{self.MaybeArg<std::string>(0)};
+    const auto dummy_value{self.MaybeArg<std::string>("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<int>();
-    }
+    const auto min_depth{self.Arg<int>("minconf")};
 
     const UniValue& addlocked = request.params[2];
     bool fAddLocked = false;

test/functional/rpc_scantxoutset.py

@@ -129,7 +129,7 @@ def run_test(self):
         assert_raises_rpc_error(-1, "scanobjects argument is required for the start action", self.nodes[0].scantxoutset, "start")
 
         # Check that invalid command give error
-        assert_raises_rpc_error(-8, "Invalid command", self.nodes[0].scantxoutset, "invalid_command")
+        assert_raises_rpc_error(-8, "Invalid action 'invalid_command'", self.nodes[0].scantxoutset, "invalid_command")
 
 
 if __name__ == "__main__":