Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) #19994

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2009-rpcAssertNames changing 3 files +381 −254
  1. MarcoFalke commented at 3:59 pm on September 22, 2020: member

    This is the last part 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. DrahtBot added the label RPC/REST/ZMQ on Sep 22, 2020
  3. DrahtBot added the label Wallet on Sep 22, 2020
  4. MarcoFalke force-pushed on Sep 22, 2020
  5. MarcoFalke removed the label Wallet on Sep 22, 2020
  6. MarcoFalke added the label Refactoring on Sep 22, 2020
  7. Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) fa14f57fbc
  8. MarcoFalke force-pushed on Sep 22, 2020
  9. DrahtBot commented at 6:57 pm on September 22, 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:

    • #19771 (net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection by luke-jr)
    • #19501 (send* RPCs in the wallet returns the “fee reason” by stackman27)
    • #19443 (rpc: Add options argument to listtransactions, with paginatebypointer options by kristapsk)
    • #19349 (doc: Include wallet path to relevant RPC calls by D4nte)
    • #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
    • #17167 (Allow whitelisting outgoing connections by luke-jr)
    • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

    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.

  10. fjahr commented at 7:37 pm on September 22, 2020: member
    tACK fa14f57fbc3c1fa2b9eea5df687f0fb36d452bd5
  11. ryanofsky approved
  12. ryanofsky commented at 5:33 pm on September 23, 2020: member
    Code review ACK fa14f57fbc3c1fa2b9eea5df687f0fb36d452bd5. Just straightforward replacements except code moved in addnode, and displatching updated in bumpfee_helper
  13. MarcoFalke merged this on Sep 23, 2020
  14. MarcoFalke closed this on Sep 23, 2020

  15. MarcoFalke deleted the branch on Sep 23, 2020
  16. sidhujag referenced this in commit 63306f71f5 on Sep 23, 2020
  17. Fabcien referenced this in commit c2cf6cba84 on Oct 14, 2021
  18. Fabcien referenced this in commit c258007216 on Oct 14, 2021
  19. 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-11-22 00:12 UTC

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