rpc: Remove index-based Arg accessor

maflcko commented at 3:36 pm on April 29, 2024: member

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.

DrahtBot commented at 3:36 pm on April 29, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, ryanofsky, achow101
Concept ACK	laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Apr 29, 2024

laanwj commented at 12:57 pm on April 30, 2024: member

Concept ACK

in src/test/rpc_tests.cpp:616 in fadb3eb57b outdated

613@@ -614,40 +614,26 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper)
614 
615     //! Check that `self.Arg` returns the same value as the `request.params` accessors
616     RPCHelpMan::RPCMethodImpl check_positional = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {

ryanofsky commented at 1:45 pm on April 30, 2024:

In commit “rpc: Remove index-based Arg accessor” (fadb3eb57b4d207a678067b89caa45abf1f93702)

Should s/check_positional/check_named/

maflcko commented at 1:47 pm on April 30, 2024:

I think this means the positional request.params accessor, so this is better to leave as-is, no?

ryanofsky commented at 2:11 pm on April 30, 2024:

I think this means the positional request.params accessor, so this is better to leave as-is, no?

I think this was called check_positional because it was calling the Arg(size_t) function and the the check below was called check_named because it was calling the Arg(string_view) function. So now that the check below is deleted and this now calling Arg(string_view), it would make more sense to call this check_named than check_positional.

But the point of this is really to call the Arg method, so maybe a name like check_args would be better than either.

stickies-v commented at 2:19 pm on April 30, 2024:

I think this was called check_positional because …

Indeed, that was the rationale.

so maybe a name like check_args

or check_equivalence? (no big deal either way ofc)

maflcko commented at 2:30 pm on April 30, 2024:

I am happy to review a follow-up, if there is a better name. But I think the current name is perfectly fine and accurate.

In any case, if the legacy code is removed, the test will go away as well.

ryanofsky approved

ryanofsky commented at 1:45 pm on April 30, 2024: contributor

Code review ACK fadb3eb57b4d207a678067b89caa45abf1f93702

DrahtBot requested review from laanwj on Apr 30, 2024

maflcko requested review from stickies-v on May 7, 2024

stickies-v approved

stickies-v commented at 11:50 am on May 9, 2024: contributor

ACK fadb3eb57b4d207a678067b89caa45abf1f93702

Can’t think of any situations where index-based is preferable over key-based, so this seems like a nice cleanup.

in src/rpc/util.h:417 in fadb3eb57b outdated

413@@ -414,9 +414,7 @@ class RPCHelpMan
414      * argument isNull() and parses (from JSON) and returns the user-passed argument,
415      * or the default value derived from the RPCArg documentation.
416      *
417-     * There are two overloads of this function:
418-     * - Use Arg<Type>(size_t i) to get the argument (or the default value) by index.
419-     * - Use Arg<Type>(const std::string& key) to get the argument (or the default value) by key.
420+     * Use Arg<Type>(const std::string& key) to get the argument (or the default value) by key.

stickies-v commented at 12:35 pm on May 9, 2024:

nit: this is just the brief, so this line should be removed/merged imo (+ for MaybeArg)

maflcko commented at 12:22 pm on May 13, 2024:

Not sure what you mean. This sentence introduces the Type, which is referred to in the next sentence, so I don’t think it can be removed.

Maybe you can share a diff?

stickies-v commented at 12:45 pm on May 13, 2024:

Ah good point, I hadn’t considered that the Type alias is introduced here. I think a preferable approach to me is to use the actual typename instead of aliasing it, such as in this diff. Optionally, I’d also be supportive of renaming R to something more descriptive such as ReturnType, but even with just R I think this is more clear:

 0diff --git a/src/rpc/util.h b/src/rpc/util.h
 1index 9ba7fcf5e6..8fc598b873 100644
 2--- a/src/rpc/util.h
 3+++ b/src/rpc/util.h
 4@@ -414,11 +414,9 @@ public:
 5      * argument isNull() and parses (from JSON) and returns the user-passed argument,
 6      * or the default value derived from the RPCArg documentation.
 7      *
 8-     * Use Arg<Type>(const std::string& key) to get the argument (or the default value) by key.
 9+     * The instantiation of this helper for type R must match the corresponding RPCArg::Type.
10      *
11-     * The Type passed to this helper must match the corresponding RPCArg::Type.
12-     *
13-     * [@return](/bitcoin-bitcoin/contributor/return/) The value of the RPC argument (or the default value) cast to type Type.
14+     * [@return](/bitcoin-bitcoin/contributor/return/) The value of the RPC argument (or the default value) cast to type R.
15      *
16      * [@see](/bitcoin-bitcoin/contributor/see/) MaybeArg for handling optional arguments without default values.
17      */
18@@ -446,12 +444,10 @@ public:
19      * argument isNull() and parses (from JSON) and returns the user-passed argument,
20      * or a falsy value if no argument was passed.
21      *
22-     * Use MaybeArg<Type>(const std::string& key) to get the optional argument by key.
23-     *
24-     * The Type passed to this helper must match the corresponding RPCArg::Type.
25+     * The instantiation of this helper for type R must match the corresponding RPCArg::Type.
26      *
27-     * [@return](/bitcoin-bitcoin/contributor/return/) For integral and floating-point types, a std::optional<Type> is returned.
28-     *         For other types, a Type* pointer to the argument is returned. If the
29+     * [@return](/bitcoin-bitcoin/contributor/return/) For integral and floating-point types, a std::optional<R> is returned.
30+     *         For other types, a R* pointer to the argument is returned. If the
31      *         argument is not provided, std::nullopt or a null pointer is returned.
32      *
33      * [@see](/bitcoin-bitcoin/contributor/see/) Arg for handling arguments that are required or have a default value.

maflcko commented at 3:22 pm on May 15, 2024:

Thanks, pushed your diff.

in src/rpc/util.h:428 in fadb3eb57b outdated

422@@ -425,8 +423,9 @@ class RPCHelpMan
423      * @see MaybeArg for handling optional arguments without default values.
424      */
425     template <typename R>
426-    auto Arg(size_t i) const

stickies-v commented at 12:36 pm on May 9, 2024:

nit: there’s still a remaining reference to Arg<Type>(i) in CheckRequiredOrDefault:

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index f683878054..d4079d9f37 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -673,7 +673,7 @@ static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>&
 5 
 6 static void CheckRequiredOrDefault(const RPCArg& param)
 7 {
 8-    // Must use `Arg<Type>(i)` to get the argument or its default value.
 9+    // Must use `Arg<Type>` to get the argument or its default value.
10     const bool required{
11         std::holds_alternative<RPCArg::Optional>(param.m_fallback) && RPCArg::Optional::NO == std::get<RPCArg::Optional>(param.m_fallback),
12     };

maflcko commented at 3:21 pm on May 15, 2024:

I was thinking that i means the key, as it is the symbol passed to the function.

Clarified in the latest push, by renaming i to key.

stickies-v commented at 12:36 pm on May 9, 2024: contributor

Oops, left my nits on the wrong PR. Nothing blocking, only if you re-touch.

rpc: Remove index-based Arg accessor fa3169b073

maflcko force-pushed on May 15, 2024

stickies-v commented at 4:56 pm on May 15, 2024: contributor

re-ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54, addressed doc nits

DrahtBot requested review from ryanofsky on May 15, 2024

ryanofsky approved

ryanofsky commented at 12:54 pm on May 17, 2024: contributor

Code review ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54. One changes since last review are some documentation improvements

achow101 commented at 0:05 am on June 5, 2024: member

ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54

achow101 merged this on Jun 5, 2024

achow101 closed this on Jun 5, 2024

maflcko deleted the branch on Jun 5, 2024

rpc: Remove index-based Arg accessor #29997

Code Coverage

Reviews

Conflicts