rpc: Fix named arguments in documentation #18607

1. 
   MarcoFalke commented at 3:42 pm on April 12, 2020: member

   This fixes a bug found with #18531:

   • Currently the named argument for generateblock is documented as address/descriptor, but the server only accepts a named argument of address. Fix it by changing the name to output for both the documentation and the server code. Also, add tests to prove the server understands the new name output.

   • Unrelated to that, there have been a bunch of aliases in the server code that are not present in the source code of the documentation. Fix that by adding the alias to the source code of the documentation. Only the first alias is displayed in the rendered documentation. Also, add tests to prove the server actually understands all aliases.

2. MarcoFalke force-pushed on Apr 12, 2020
3. DrahtBot added the label Mining on Apr 12, 2020
4. DrahtBot added the label RPC/REST/ZMQ on Apr 12, 2020
5. DrahtBot added the label Tests on Apr 12, 2020
6. rpc: Rename first arg of generateblock RPC to "output" fa86a4bbfc
7. MarcoFalke force-pushed on Apr 12, 2020
8. MarcoFalke removed the label Mining on Apr 12, 2020
9. 
   DrahtBot commented at 10:09 pm on April 12, 2020: member

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

   Conflicts

   No conflicts as of last run.

10. MarcoFalke removed the label Tests on Apr 13, 2020
11. in src/rpc/util.cpp:428 in fa8a3e04a8 outdated

    421@@ -419,8 +422,12 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
    422 {
    423     std::set<std::string> named_args;
    424     for (const auto& arg : m_args) {
    425+        std::vector<std::string> names;
    426+        boost::split(names, arg.m_names, boost::is_any_of("|"));
    427         // Should have unique named arguments
    428-        CHECK_NONFATAL(named_args.insert(arg.m_name).second);
    429+        for (const std::string& name : names) {
    

    ---

    ---

    

    pierreN commented at 9:20 pm on April 15, 2020:

    Since you create accessors for m_names (GetName, GetFirstName), it would make sense to make m_names private, no?

    Plus this would allow you to have a function GetNames similar to:

    0std::vector<std::string> RPCArg::GetNames() const
    1{
    2    std::vector<std::string> names;
    3    boost::split(names, m_names, boost::is_any_of("|"));
    4    return names;
    5}
    

    and do here more cleanly for (const std::string& name : arg.GetNames()) {

    (note that Fallback, m_fallback, m_description, m_oneline_description can also be made private without any additional code change)

    ---

    

    MarcoFalke commented at 12:38 pm on April 16, 2020:

    RPCArg is a struct, because it was meant to be a stupid class to only hold data and not have complicated member functions. I see that with the name aliases this is no longer true. I think it can be made a class and the GetNames utility can be added. Though, I will leave this pull as is for now, because currently GetNames is only used in this one place. And just in-lining it will yield the least amount of LOC written.
12. in src/rpc/util.cpp:519 in fa8a3e04a8 outdated

    512@@ -506,6 +513,18 @@ std::string RPCHelpMan::ToString() const
    513     return ret;
    514 }
    515 
    516+std::string RPCArg::GetFirstName() const
    517+{
    518+    auto ret = m_names.substr(0, m_names.find("|"));
    519+    return ret;
    

    ---

    ---

    

    pierreN commented at 9:23 pm on April 15, 2020:

    For style, I believe this could be a one-liner, no? return m_names.substr(0, m_names.find("|")); (or have a better variable name than ret)

    But this is personal taste (and you know the codebase style better than I do :)

    ---

    

    MarcoFalke commented at 12:46 pm on April 16, 2020:

    Thanks, fixed and force pushed.
13. pierreN approved
14. 
    pierreN commented at 9:24 pm on April 15, 2020: contributor

    For what it’s worth, tested ACK.

    I just left 2 comments below if I were to be extremely picky.

15. MarcoFalke force-pushed on Apr 16, 2020
16. rpc: Document all aliases for second arg of getblock fa5b1f067f
17. rpc: Document all aliases for first arg of listtransactions fa168d7542
18. MarcoFalke force-pushed on Apr 16, 2020
19. 
    MarcoFalke commented at 12:46 pm on April 16, 2020: member

    Addressed feedback by @pierreN
20. 
    MarcoFalke commented at 12:59 pm on April 16, 2020: member

    And thanks for testing @pierreN . If you want to get included in the merge commit message with your ACK, you can write

    Tested ACK ${commit_hash_of_the_commit_you_tested} ${optional_description_of_how and_what_you_tested}

21. 
    pierreN commented at 2:19 pm on April 16, 2020: contributor

    Thanks!

    Tested ACK fa168d7 tests, help messages

22. 
    MarcoFalke commented at 4:13 pm on April 17, 2020: member

    Rendered diff for reference:

     0$ git diff --ignore-all-space 
     1diff --git a/generateblock b/generateblock
     2index 5586a32..55aca89 100644
     3--- a/generateblock
     4+++ b/generateblock
     5@@ -1,9 +1,9 @@
     6-generateblock "address/descriptor" ["rawtx/txid",...]
     7+generateblock "output" ["rawtx/txid",...]
     8 
     9 Mine a block with a set of ordered transactions immediately to a specified address or descriptor (before the RPC call returns)
    10 
    11 Arguments:
    12-1. address/descriptor    (string, required) The address or descriptor to send the newly generated bitcoin to.
    13+1. output               (string, required) The address or descriptor to send the newly generated bitcoin to.
    14 2. transactions         (json array, required) An array of hex strings which are either txids or raw transactions.
    15                         Txids must reference transactions currently in the mempool.
    16                         All transactions must be valid and in valid order, otherwise the block will be rejected.
    

23. MarcoFalke merged this on Apr 17, 2020
24. MarcoFalke closed this on Apr 17, 2020

    ---

25. sidhujag referenced this in commit 645c5f7960 on Apr 17, 2020
26. MarcoFalke deleted the branch on Apr 17, 2020
27. MarcoFalke referenced this in commit 804ca26629 on Jul 15, 2020
28. sidhujag referenced this in commit ea73ed0a45 on Jul 16, 2020
29. MarcoFalke referenced this in commit 31760bb7c9 on Aug 14, 2020
30. sidhujag referenced this in commit b761dae549 on Aug 16, 2020
31. sidhujag referenced this in commit 010a3815db on Aug 16, 2020
32. MarcoFalke referenced this in commit 89a8299a14 on Aug 31, 2020
33. sidhujag referenced this in commit 4c2ef86224 on Aug 31, 2020
34. MarcoFalke referenced this in commit d692d192cd on Sep 22, 2020
35. sidhujag referenced this in commit f769a0d8b6 on Sep 23, 2020
36. MarcoFalke referenced this in commit 5e14fafb31 on Sep 23, 2020
37. sidhujag referenced this in commit 63306f71f5 on Sep 23, 2020
38. MarcoFalke referenced this in commit 71d068db40 on Nov 19, 2020
39. sidhujag referenced this in commit 92bd89791a on Nov 19, 2020
40. jasonbcox referenced this in commit ec1d2d93d8 on Dec 1, 2020
41. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke DrahtBot pierreN

Labels
RPC/REST/ZMQ