Merge bitcoin/bitcoin#29997: rpc: Remove index-based Arg accessor

fa3169b rpc: Remove index-based Arg accessor (MarcoFalke)

Pull request description:

  The index-based Arg accessor is redundant with the name-based one. It does not provide any benefit to the code reader, or otherwise, so remove it.

ACKs for top commit:
  stickies-v:
    re-ACK fa3169b, addressed doc nits
  achow101:
    ACK fa3169b
  ryanofsky:
    Code review ACK fa3169b. One changes since last review are some documentation improvements

Tree-SHA512: f9da1c049dbf38c3b47a8caf8d24d195c2d4b88c7ec45a9ccfb78f1e39f29cb86869f84b308f6e49856b074c06604ab634c90eb89c9c93d2a8169e070aa1bd40

Author: achow101

src/rpc/mempool.cpp

@@ -82,7 +82,7 @@ static RPCHelpMan sendrawtransaction()
 
             CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
 
-            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};
+            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>("maxfeerate"))};
 
             int64_t virtual_size = GetVirtualTransactionSize(*tx);
             CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
@@ -162,7 +162,7 @@ static RPCHelpMan testmempoolaccept()
                                    "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
             }
 
-            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};
+            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>("maxfeerate"))};
 
             std::vector<CTransactionRef> txns;
             txns.reserve(raw_transactions.size());
@@ -873,7 +873,7 @@ static RPCHelpMan submitpackage()
             }
 
             // Fee check needs to be run with chainstate and package context
-            const CFeeRate max_raw_tx_fee_rate = ParseFeeRate(self.Arg<UniValue>(1));
+            const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>("maxfeerate"))};
             std::optional<CFeeRate> client_maxfeerate{max_raw_tx_fee_rate};
             // 0-value is special; it's mapped to no sanity check
             if (max_raw_tx_fee_rate == CFeeRate(0)) {

src/rpc/mining.cpp

@@ -485,7 +485,7 @@ static RPCHelpMan prioritisetransaction()
     LOCK(cs_main);
 
     uint256 hash(ParseHashV(request.params[0], "txid"));
-    const auto dummy{self.MaybeArg<double>(1)};
+    const auto dummy{self.MaybeArg<double>("dummy")};
     CAmount nAmount = request.params[2].getInt<int64_t>();
 
     if (dummy && *dummy != 0) {

src/rpc/net.cpp

@@ -401,7 +401,7 @@ static RPCHelpMan addconnection()
     } else {
         throw JSONRPCError(RPC_INVALID_PARAMETER, self.ToString());
     }
-    bool use_v2transport = self.Arg<bool>(2);
+    bool use_v2transport{self.Arg<bool>("v2transport")};
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     CConnman& connman = EnsureConnman(node);

src/rpc/util.cpp

@@ -677,7 +677,7 @@ static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>&
 
 static void CheckRequiredOrDefault(const RPCArg& param)
 {
-    // Must use `Arg<Type>(i)` to get the argument or its default value.
+    // Must use `Arg<Type>(key)` to get the argument or its default value.
     const bool required{
         std::holds_alternative<RPCArg::Optional>(param.m_fallback) && RPCArg::Optional::NO == std::get<RPCArg::Optional>(param.m_fallback),
     };

src/rpc/util.h

@@ -414,19 +414,16 @@ class RPCHelpMan
      * 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 instantiation of this helper for type R must match the corresponding RPCArg::Type.
      *
-     * 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.
+     * @return The value of the RPC argument (or the default value) cast to type R.
      *
      * @see MaybeArg for handling optional arguments without default values.
      */
     template <typename R>
-    auto Arg(size_t i) const
+    auto Arg(std::string_view key) const
     {
+        auto i{GetParamIndex(key)};
         // Return argument (required or with default value).
         if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
             // Return numbers by value.
@@ -436,11 +433,6 @@ 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.
      *
@@ -452,21 +444,18 @@ class RPCHelpMan
      * 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 instantiation of this helper for type R must match the corresponding RPCArg::Type.
      *
-     * The Type passed to this helper must match the corresponding RPCArg::Type.
+     * @return For integral and floating-point types, a std::optional<R> is returned.
+     *         For other types, a R* pointer to the argument is returned. If the
+     *         argument is not provided, std::nullopt or a null pointer is returned.
      *
-     * @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
+    auto MaybeArg(std::string_view key) const
     {
+        auto i{GetParamIndex(key)};
         // Return optional argument (without default).
         if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
             // Return numbers by value, wrapped in optional.
@@ -476,11 +465,6 @@ 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;

src/test/rpc_tests.cpp

@@ -614,40 +614,26 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper)
 
     //! 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());
+            BOOST_CHECK_EQUAL(self.Arg<int>("req_int"), request.params[0].getInt<int>());
+            BOOST_CHECK_EQUAL(self.Arg<std::string>("req_str"), request.params[1].get_str());
+            BOOST_CHECK_EQUAL(self.Arg<uint64_t>("def_uint64_t"), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt<uint64_t>());
+            BOOST_CHECK_EQUAL(self.Arg<std::string>("def_string"), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str());
+            BOOST_CHECK_EQUAL(self.Arg<bool>("def_bool"), 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());
+                BOOST_CHECK_EQUAL(self.MaybeArg<double>("opt_double").value(), request.params[5].get_real());
             } else {
-                BOOST_CHECK(!self.MaybeArg<double>(5));
+                BOOST_CHECK(!self.MaybeArg<double>("opt_double"));
             }
             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());
+                BOOST_CHECK(self.MaybeArg<std::string>("opt_string"));
+                BOOST_CHECK_EQUAL(*self.MaybeArg<std::string>("opt_string"), request.params[6].get_str());
             } else {
-                BOOST_CHECK(!self.MaybeArg<std::string>(6));
+                BOOST_CHECK(!self.MaybeArg<std::string>("opt_string"));
             }
             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/wallet.cpp

@@ -395,7 +395,7 @@ static RPCHelpMan createwallet()
     if (!request.params[4].isNull() && request.params[4].get_bool()) {
         flags |= WALLET_FLAG_AVOID_REUSE;
     }
-    if (self.Arg<bool>(5)) {
+    if (self.Arg<bool>("descriptors")) {
 #ifndef USE_SQLITE
         throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)");
 #endif
@@ -489,7 +489,7 @@ static RPCHelpMan unloadwallet()
         // Release the "main" shared pointer and prevent further notifications.
         // Note that any attempt to load the same wallet would fail until the wallet
         // is destroyed (see CheckUniqueFileid).
-        std::optional<bool> load_on_start{self.MaybeArg<bool>(1)};
+        std::optional<bool> load_on_start{self.MaybeArg<bool>("load_on_startup")};
         if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
             throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
         }