rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

maflcko · 2020-06-12T23:21:05Z

This simplifies how omitted args are passed into the rpc help manager.

Previously, omitted named args had to be passed in as OMITTED_NAMED_ARG. However, the rpc help manager already knows when an arg is a top level argument. Thus, OMITTED_NAMED_ARG can simply be replaced by OMITTED.

Some calls have been using the omitted-enum incorrectly, thus the rendered diff (effect of the changes here) is:

diff --git a/createwallet b/createwallet
index 539552e..677a15c 100644
--- a/createwallet
+++ b/createwallet
@@ -6,7 +6,7 @@ Arguments:
 1. wallet_name             (string, required) The name for the new wallet. If this is a path, the wallet will be created at the path location.
 2. disable_private_keys    (boolean, optional, default=false) Disable the possibility of private keys (only watchonlys are possible in this mode).
 3. blank                   (boolean, optional, default=false) Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using sethdseed.
-4. passphrase              (string) Encrypt the wallet with this passphrase.
+4. passphrase              (string, optional) Encrypt the wallet with this passphrase.
 5. avoid_reuse             (boolean, optional, default=false) Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind.
 6. descriptors             (boolean, optional, default=false) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation
 
diff --git a/getblocktemplate b/getblocktemplate
index 24368b1..9f38068 100644
--- a/getblocktemplate
+++ b/getblocktemplate
@@ -11,8 +11,8 @@ For full specification, see BIPs 22, 23, 9, and 145:
 Arguments:
 1. template_request         (json object, optional, default={}) Format of the template
      {
-       "mode": "str",       (string, optional) This must be set to "template", "proposal" (see BIP 23), or omitted
-       "capabilities": [    (json array, optional) A list of strings
+       "mode": "str",       (string) This must be set to "template", "proposal" (see BIP 23), or omitted
+       "capabilities": [    (json array) The capabilities, can be omitted
          "support",         (string) client side supported feature, 'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'
          ...
        ],
diff --git a/scantxoutset b/scantxoutset
index 1f166fb..b09e533 100644
--- a/scantxoutset
+++ b/scantxoutset
@@ -21,7 +21,7 @@ Arguments:
                                  "start" for starting a scan
                                  "abort" for aborting the current scan (returns true when abort was successful)
                                  "status" for progress report (in %) of the current scan
-2. scanobjects                   (json array) Array of scan objects. Required for "start" action
+2. scanobjects                   (json array, optional) Array of scan objects. Required for "start" action
                                  Every scan object is either a string descriptor or an object:
      [
        "descriptor",             (string) An output descriptor

DrahtBot · 2020-06-13T02:47:56Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Stale ACK	ryanofsky

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Add support for name-only parameters by ryanofsky)
#26039 (refactor: Run type check against RPCArgs (1/2) by MarcoFalke)
#25093 (rpc: Check for omitted, but required parameters by MarcoFalke)
#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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.

laanwj · 2020-06-16T17:48:00Z

-       "mode": "str",       (string, optional) This must be set to "template", "proposal" (see BIP 23), or omitted
-       "capabilities": [    (json array, optional) A list of strings
+       "mode": "str",       (string) This must be set to "template", "proposal" (see BIP 23), or omitted
+       "capabilities": [    (json array) The capabilities, can be omitted

This change doesn't seem correct if they're optional ("can be omitted"")?

ryanofsky

Code review ACK fab2f3b. Suggested more changes, but this is fine and seems to be moving in a good direction.

ryanofsky · 2020-06-30T13:57:11Z

src/rpc/util.cpp

    }
    if (m_fallback.which() == 1) {
        ret += ", optional, default=" + boost::get<std::string>(m_fallback);
    } else {
        switch (boost::get<RPCArg::Optional>(m_fallback)) {
        case RPCArg::Optional::OMITTED: {
+            if (is_top_level_arg) {


In commit "refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED" (fab2f3b)

Along lines of laanwj's comment #19262 (comment), it doesn't seem like is_top_level_arg is the right condition to be checking here. It seems helpful to add ", optional" for any field value, not just a top level argument value. Maybe is_top_level_arg should be is_array_element, since it does make sense to hide ", optional" inside a list where presumably every element is optional.

ryanofsky · 2020-06-30T14:04:01Z

src/rpc/util.cpp

-
-            // no default case, so the compiler can warn about missing cases
-        }
+        } // no default case, so the compiler can warn about missing cases
    }
    if (m_fallback.which() == 1) {
        ret += ", optional, default=" + boost::get<std::string>(m_fallback);
    } else {
        switch (boost::get<RPCArg::Optional>(m_fallback)) {
        case RPCArg::Optional::OMITTED: {


In commit "refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED" (fab2f3b)

For a future followup I think it'd be good to consider renaming Optional::OMITTED to Optional::YES to match the Optional::NO. It doesn't seem clear in context what "OMITTED" actually omits.

It doesn't seem clear in context what "OMITTED" actually omits.

It means the default value is omitted. So RPCArg::Optional::OMITTED* could become RPCArg::DefaultOmitted? (Compare to Default and DefaultHint)

DrahtBot · 2022-12-13T23:57:05Z

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

luke-jr · 2022-12-15T00:32:23Z

Concept NACK. Seems like a pointless refactor that just conflates two different meanings behind the same flag. Does it accomplish something?

maflcko · 2022-12-15T07:28:03Z

No it does not. Thanks for the NACK and closing for now.

maflcko · 2022-12-15T07:28:58Z

Well, there are some bugfixes here, but I'll submit them separately.

maflcko · 2022-12-15T16:12:41Z

Removed the refactor in #26706

maflcko added Refactoring RPC/REST/ZMQ Docs and removed Refactoring labels Jun 12, 2020

maflcko force-pushed the 2006-rpcHelpRefactor branch from fa6601e to fab2f3b Compare June 12, 2020 23:51

This was referenced Jun 13, 2020

test: checks that bitcoin-cli autocomplete is in sync #18606

Closed

[RPC] Include coinbase transactions in receivedby RPCs #14707

Merged

DrahtBot mentioned this pull request Jun 17, 2020

rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654

Merged

ryanofsky approved these changes Jun 30, 2020

View reviewed changes

maflcko marked this pull request as draft June 30, 2020 23:09

DrahtBot added the Needs rebase label Dec 13, 2021

maflcko mentioned this pull request May 10, 2022

doc: Fix incorrect sendmany RPC doc #25093

Merged

MarcoFalke added 2 commits December 12, 2022 12:14

refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED

de73738

refactor: Introduce is_top_level_arg

c7753f5

maflcko force-pushed the 2006-rpcHelpRefactor branch from fab2f3b to c7753f5 Compare December 12, 2022 11:16

DrahtBot removed the Needs rebase label Dec 12, 2022

This was referenced Dec 12, 2022

RPC: Accept options as named-only parameters #26485

Merged

refactor: Run type check against RPCArgs (1/2) #26039

Merged

rpc: Return fee and prevout (utxos) to getrawtransaction #23319

Merged

DrahtBot added the Needs rebase label Dec 13, 2022

maflcko closed this Dec 15, 2022

maflcko deleted the 2006-rpcHelpRefactor branch December 15, 2022 13:12

bitcoin locked and limited conversation to collaborators Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

Uh oh!

maflcko commented Jun 12, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Jun 13, 2020 •

edited

Loading

Uh oh!

laanwj commented Jun 16, 2020

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jun 30, 2020

Uh oh!

ryanofsky Jun 30, 2020

Uh oh!

maflcko Dec 15, 2022

Uh oh!

DrahtBot commented Dec 13, 2022

Uh oh!

luke-jr commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

Uh oh!

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

Uh oh!

Conversation

maflcko commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

laanwj commented Jun 16, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 13, 2022

Uh oh!

luke-jr commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

Uh oh!

maflcko commented Jun 12, 2020 •

edited

Loading

DrahtBot commented Jun 13, 2020 •

edited

Loading