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:

    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
    
  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

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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
    -       "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"")?

  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

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  21. luke-jr commented at 12: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: 2026-05-01 03:14 UTC

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