refactor: Add ParseBool to rpc/util

fjahr commented at 9:52 pm on July 17, 2020: member

Mini refactoring for DRYer code and better readability. Replaces several instances of bool parsing in the RPC code with a rpc/util function. Also uses const when appropriate.

fjahr commented at 11:05 pm on July 17, 2020: member

#19501 introduces more verbose flags, which is how I got the idea from reviewing it.

DrahtBot added the label Refactoring on Jul 17, 2020

DrahtBot added the label RPC/REST/ZMQ on Jul 17, 2020

in src/rpc/blockchain.cpp:539 in f76742a880 outdated

535@@ -536,10 +536,7 @@ static UniValue getrawmempool(const JSONRPCRequest& request)
536                 },
537             }.Check(request);
538 
539-    bool fVerbose = false;
540-    if (!request.params[0].isNull())
541-        fVerbose = request.params[0].get_bool();
542-
543+    const bool fVerbose = GetBool(request.params[0], false);

MarcoFalke commented at 6:08 am on July 18, 2020:

Doesn’t this change behavior when passed an int?

If you want to refactor this to a single line without changing behavior, it would read maybe so:

0    const bool fVerbose{request.params[0].isNull() ? false : request.params[0].get_bool()};

fjahr commented at 9:41 am on July 18, 2020:

Yeah, sorry, I forgot to comment on that it would allow us to use ints in more cases. I guess the int case is only used in rawtransaction.cpp for historical reasons and we rather want to error in other cases? I removed the int part from the function and removed its use from the one place where it’s relevant. And I know this can be made a one-liner but I don’t like reading ternary operators for the same base case because I still feel like I need to make sure there is not something less intuitive going on like with the is_witness stuff, a ! hiding somewhere etc. But if you don’t see the same benefit it’s fine if we close it, I think just using ternary operators isn’t worth the change.

laanwj commented at 10:30 am on July 22, 2020:

I like the idea of factoring this out to a function, but I don’t personally think ints should be accepted where bools are expected in more cases. Being specific about input types helps catch, for example, bugs when the argument is shifted.

fjahr commented at 9:47 pm on August 2, 2020:

I agree, I changed that already so there is no behavior change anymore now. I just didn’t make a clearer comment on it.

DrahtBot commented at 8:44 am on July 18, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19849 (Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) by MarcoFalke)
#19572 (ZMQ: Create “sequence” notifier, enabling client-side mempool tracking by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fjahr force-pushed on Jul 18, 2020

in src/rpc/util.h:84 in 2049105dfc outdated

79@@ -80,6 +80,9 @@ extern std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKe
80 
81 CoinStatsHashType ParseHashType(const UniValue& param, const CoinStatsHashType default_type);
82 
83+/** Parse bool from UniValue param with default fallback */
84+bool GetBool(const UniValue& param, const bool default_value);

promag commented at 8:57 am on July 23, 2020:

nit, ParseBool?

fjahr commented at 9:48 pm on August 2, 2020:

done

in src/rpc/util.cpp:135 in 2049105dfc outdated

129@@ -130,6 +130,14 @@ CoinStatsHashType ParseHashType(const UniValue& param, const CoinStatsHashType d
130     }
131 }
132 
133+bool GetBool(const UniValue& param, const bool default_value)
134+{
135+    if (!param.isNull()) {

promag commented at 8:57 am on July 23, 2020:

Just return param.isNull() ? default_value : param.get_bool();?

fjahr commented at 9:48 pm on August 2, 2020:

done

promag commented at 8:58 am on July 23, 2020: member

Concept ACK.

fjahr force-pushed on Aug 2, 2020

fjahr commented at 9:48 pm on August 2, 2020: member

Took suggestions by @promag

fjahr force-pushed on Aug 2, 2020

refactor: Add ParseBool to rpc/util

The helper function encapsulates the boolean parse logic in one place.

c2551d00af

promag commented at 10:12 pm on August 2, 2020: member

Need to update PR title and reword commit.

fjahr force-pushed on Aug 2, 2020

fjahr commented at 10:15 pm on August 2, 2020: member

Need to update PR title and reword commit.

Done, also rebased on master to get access to the git commit message linter that failed in the CI but not locally on my one-liner commit message.

fjahr renamed this:
~~refactor: Add GetBool to rpc/util~~
refactor: Add ParseBool to rpc/util
on Aug 2, 2020

practicalswift commented at 3:30 pm on August 14, 2020: contributor

ACK c2551d00afc60d8adac1b2f97be1441c8e9f6f9d – suggested version is easier to reason about and patch looks correct

MarcoFalke commented at 5:49 pm on August 27, 2020: member

Should a helper for every type be added?

It would probably be better to hide all parsing etc within RPCHelpMan, so that the logic in rpc methods itself doesn’t need to worry about that

fjahr commented at 8:52 am on August 28, 2020: member

Should a helper for every type be added?

It would probably be better to hide all parsing etc within RPCHelpMan, so that the logic in rpc methods itself doesn’t need to worry about that

Yeah, makes sense I think. Unless that is something you are already working on, I will explore it and make a suggestion.

MarcoFalke commented at 9:08 am on August 28, 2020: member

I’d suspect this can only be done after #18531

DrahtBot commented at 5:07 pm on September 22, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Sep 22, 2020

fanquake commented at 7:04 am on April 8, 2021: member

#18531 is now merged. @fjahr are you still interested in working on this?

MarcoFalke commented at 10:18 am on April 8, 2021: member

Related: #20017

fjahr commented at 2:53 pm on April 11, 2021: member

#18531 is now merged. @fjahr are you still interested in working on this?

Yes but I guess #20017 took the same idea and is further along now, so closing.

fjahr closed this on Apr 11, 2021

fjahr deleted the branch on Apr 11, 2021

DrahtBot locked this on Aug 16, 2022

refactor: Add ParseBool to rpc/util #19544

Conflicts