Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2008-rpcAssertNames changing 3 files +295 −193
  1. MarcoFalke commented at 4:31 pm on August 31, 2020: member

    This is split out from #18531 to just touch some RPC methods. Description from the main pr:

    Motivation

    RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the CRPCCommands and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section “bugs found” for a list of bugs that have been found as a result of the changes here.

    Changes

    The changes here add an assert in the CRPCCommand constructor that the RPCArg names are identical to the ones in the CRPCCommand.

    Future work

    Here or follow up, makes sense to also assert type of returned UniValue?

    Sure, but let’s not get ahead of ourselves. I am going to submit any further works as follow-ups, including:

    • Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
    • Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
    • Auto-formatting and sanity checking the RPCExamples with RPCMan
    • Checking passed-in json in self-check. Removing redundant checks
    • Checking returned json against documentation to avoid regressions or false documentation
    • Compile the RPC documentation at compile-time to ensure it doesn’t change at runtime and is completely static

    Bugs found

    • The assert identified issue #18607
    • The changes itself fixed bug #19250
  2. Assert that RPCArg names are equal to CRPCCommand ones (blockchain) fa80c81487
  3. Assert that RPCArg names are equal to CRPCCommand ones (rawtransaction) fa6bb0ce5d
  4. MarcoFalke added the label Refactoring on Aug 31, 2020
  5. MarcoFalke added the label RPC/REST/ZMQ on Aug 31, 2020
  6. DrahtBot commented at 4:54 pm on August 31, 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:

    • #19806 (validation: UTXO snapshot activation by jamesob)
    • #19792 (rpc: Add dumpcoinstats by fjahr)
    • #19572 (ZMQ: Create “sequence” notifier, enabling client-side mempool tracking by instagibbs)
    • #19544 (refactor: Add ParseBool to rpc/util by fjahr)
    • #19476 (rpc: Add mempoolchanges by promag)
    • #19463 (Prune locks by luke-jr)
    • #18689 (rpc: allow dumptxoutset to dump human-readable data by pierreN)
    • #14053 (Add address-based index (attempt 4?) by marcinja)

    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.

  7. fjahr commented at 10:02 pm on August 31, 2020: member
    utACK fa6bb0ce5dba33970e2c1e47ea4d0d2c0718eccb
  8. tryphe commented at 5:54 am on September 16, 2020: contributor
    utACK fa6bb0ce5dba33970e2c1e47ea4d0d2c0718eccb. Reducing data duplication is nice. Code changes are minimal and concise.
  9. tryphe commented at 5:58 am on September 16, 2020: contributor
    Other thoughts: perhaps leave a commented example of how to add the old style RPC, if someone wants to add their own RPC without documentation.
  10. MarcoFalke merged this on Sep 22, 2020
  11. MarcoFalke closed this on Sep 22, 2020

  12. MarcoFalke deleted the branch on Sep 22, 2020
  13. MarcoFalke commented at 4:43 pm on September 22, 2020: member

    Other thoughts: perhaps leave a commented example of how to add the old style RPC, if someone wants to add their own RPC without documentation.

    You can skip adding documentation by passing empty strings, though the argument names need to be specified (both with the legacy constructor and the new one). Only difference is that the types will have to be specified as well. If it is worth it to allow arbitrary types for a quick hack, it might be useful to overwrite the Check method (e.g. with a noop), but I’ll leave that for a follow-up.

  14. sidhujag referenced this in commit f769a0d8b6 on Sep 23, 2020
  15. deadalnix referenced this in commit bf542dca1e on Oct 12, 2021
  16. deadalnix referenced this in commit d9ac1af5e4 on Oct 12, 2021
  17. DrahtBot locked this on Feb 15, 2022

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-07-03 13:13 UTC

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