rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2006-rpcHelpRefactor changing 14 files +68 −71
  1. maflcko commented at 11:21 pm on June 12, 2020: member

    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:

     0diff --git a/createwallet b/createwallet
     1index 539552e..677a15c 100644
     2--- a/createwallet
     3+++ b/createwallet
     4@@ -6,7 +6,7 @@ Arguments:
     5 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.
     6 2. disable_private_keys    (boolean, optional, default=false) Disable the possibility of private keys (only watchonlys are possible in this mode).
     7 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.
     8-4. passphrase              (string) Encrypt the wallet with this passphrase.
     9+4. passphrase              (string, optional) Encrypt the wallet with this passphrase.
    10 5. avoid_reuse             (boolean, optional, default=false) Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind.
    11 6. descriptors             (boolean, optional, default=false) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation
    12 
    13diff --git a/getblocktemplate b/getblocktemplate
    14index 24368b1..9f38068 100644
    15--- a/getblocktemplate
    16+++ b/getblocktemplate
    17@@ -11,8 +11,8 @@ For full specification, see BIPs 22, 23, 9, and 145:
    18 Arguments:
    19 1. template_request         (json object, optional, default={}) Format of the template
    20      {
    21-       "mode": "str",       (string, optional) This must be set to "template", "proposal" (see BIP 23), or omitted
    22-       "capabilities": [    (json array, optional) A list of strings
    23+       "mode": "str",       (string) This must be set to "template", "proposal" (see BIP 23), or omitted
    24+       "capabilities": [    (json array) The capabilities, can be omitted
    25          "support",         (string) client side supported feature, 'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'
    26          ...
    27        ],
    28diff --git a/scantxoutset b/scantxoutset
    29index 1f166fb..b09e533 100644
    30--- a/scantxoutset
    31+++ b/scantxoutset
    32@@ -21,7 +21,7 @@ Arguments:
    33                                  "start" for starting a scan
    34                                  "abort" for aborting the current scan (returns true when abort was successful)
    35                                  "status" for progress report (in %) of the current scan
    36-2. scanobjects                   (json array) Array of scan objects. Required for "start" action
    37+2. scanobjects                   (json array, optional) Array of scan objects. Required for "start" action
    38                                  Every scan object is either a string descriptor or an object:
    39      [
    40        "descriptor",             (string) An output descriptor
    
  2. maflcko added the label Refactoring on Jun 12, 2020
  3. maflcko added the label RPC/REST/ZMQ on Jun 12, 2020
  4. maflcko removed the label Refactoring on Jun 12, 2020
  5. maflcko added the label Docs on Jun 12, 2020
  6. maflcko force-pushed on Jun 12, 2020
  7. DrahtBot commented at 2:47 am on June 13, 2020: contributor

    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.

  8. laanwj commented at 5:48 pm on June 16, 2020: member
    0-       "mode": "str",       (string, optional) This must be set to "template", "proposal" (see BIP 23), or omitted
    1-       "capabilities": [    (json array, optional) A list of strings
    2+       "mode": "str",       (string) This must be set to "template", "proposal" (see BIP 23), or omitted
    3+       "capabilities": [    (json array) The capabilities, can be omitted
    

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

  9. in src/rpc/util.cpp:748 in fab2f3bb64 outdated
    583     if (m_fallback.which() == 1) {
    584         ret += ", optional, default=" + boost::get<std::string>(m_fallback);
    585     } else {
    586         switch (boost::get<RPCArg::Optional>(m_fallback)) {
    587         case RPCArg::Optional::OMITTED: {
    588+            if (is_top_level_arg) {
    


    ryanofsky commented at 1:57 pm on June 30, 2020:

    In commit “refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED” (fab2f3bb6446d5202b7a665cde1e7c0d02718e18)

    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.

  10. in src/rpc/util.cpp:747 in fab2f3bb64 outdated
    582     }
    583     if (m_fallback.which() == 1) {
    584         ret += ", optional, default=" + boost::get<std::string>(m_fallback);
    585     } else {
    586         switch (boost::get<RPCArg::Optional>(m_fallback)) {
    587         case RPCArg::Optional::OMITTED: {
    


    ryanofsky commented at 2:04 pm on June 30, 2020:

    In commit “refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED” (fab2f3bb6446d5202b7a665cde1e7c0d02718e18)

    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.


    maflcko commented at 4:11 pm on December 15, 2022:

    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)

  11. ryanofsky approved
  12. ryanofsky commented at 2:11 pm on June 30, 2020: contributor
    Code review ACK fab2f3bb6446d5202b7a665cde1e7c0d02718e18. Suggested more changes, but this is fine and seems to be moving in a good direction.
  13. maflcko marked this as a draft on Jun 30, 2020
  14. DrahtBot added the label Needs rebase on Dec 13, 2021
  15. refactor: Replace confusing OMITTED_NAMED_ARG with OMITTED de73738b18
  16. refactor: Introduce is_top_level_arg c7753f5088
  17. maflcko force-pushed on Dec 12, 2022
  18. DrahtBot removed the label Needs rebase on Dec 12, 2022
  19. DrahtBot added the label Needs rebase on Dec 13, 2022
  20. DrahtBot commented at 11:57 pm on December 13, 2022: contributor

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

  21. luke-jr commented at 0:32 am on December 15, 2022: member
    Concept NACK. Seems like a pointless refactor that just conflates two different meanings behind the same flag. Does it accomplish something?
  22. maflcko commented at 7:28 am on December 15, 2022: member
    No it does not. Thanks for the NACK and closing for now.
  23. maflcko closed this on Dec 15, 2022

  24. maflcko commented at 7:28 am on December 15, 2022: member
    Well, there are some bugfixes here, but I’ll submit them separately.
  25. maflcko deleted the branch on Dec 15, 2022
  26. maflcko commented at 4:12 pm on December 15, 2022: member
  27. bitcoin locked this on Dec 15, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me