rpc: document returned error fields as optional if applicable #19644

1. 
   theStack commented at 4:49 pm on August 2, 2020: member

   The following RPCs return error fields (named "error" or "errors") that are optional, but don’t show up as optional in the help text yet:

   • analyzepsbt
   • estimatesmartfee
   • signrawtransactionwithkey
   • signrawtransactionwithwallet

   The following RPC has the errors field already marked as optional, but doesn’t match the usual format in the description (like "if there are any" in parantheses):

   • estimaterawfee

   This PR adds the missing optional flags and adapts the description strings. Inspired by a recent PR #19634 by justinmoon.

   The instances were found via git grep "RPCResult.*\"error". Note that there is one RPC so far where the return error is not optional (i.e. in case of no error, the field is included in the result, but is just empty), namely bumpfee.

2. rpc: document returned error fields as optional if applicable

   Affects the following RPCs:
   - analyzepsbt
   - estimatesmartfee
   - signrawtransactionwithkey
   - signrawtransactionwithwallet
   
   For the RPC estimaterawfee, the description message was adapted
   to match the other optional ones.

   f110b7c722
3. DrahtBot added the label Mining on Aug 2, 2020
4. DrahtBot added the label RPC/REST/ZMQ on Aug 2, 2020
5. DrahtBot added the label Wallet on Aug 2, 2020
6. 
   adaminsky commented at 8:16 pm on August 5, 2020: contributor

   ACK f110b7c I successfully built and ran unit tests and the following functional tests: rpc_psbt.py, rpc_estimatefee.py, rpc_signrawtransaction.py, rpc_rawtransaction.py. The code changes look good.

   I may be missing something, but it seems like the error field for bumpfee should be optional (maybe a different PR) since I see no use for always including it.

7. 
   theStack commented at 12:43 pm on August 6, 2020: member

   @adaminsky: Thanks for reviewing!

   I may be missing something, but it seems like the error field for bumpfee should be optional (maybe a different PR) since I see no use for always including it.

   I agree that the behaviour considering presence of error fields should be the same for all RPCs. Generally one has to be very careful though in changing the return format, as this could potentially break the compatibility to RPC clients. If they e.g. did their success check to the bumpfee call by explicitely testing that the return error field is "" (empty string), this would not work anymore.

8. 
   laanwj commented at 4:46 pm on August 6, 2020: member

   ACK f110b7c722eb150816a26cab161ac2b8c0f58609, new documentation looks consistent with actual behavior
9. 
   achow101 commented at 4:14 pm on August 13, 2020: member

   ACK f110b7c722eb150816a26cab161ac2b8c0f58609
10. 
    meshcollider commented at 9:57 pm on August 13, 2020: contributor

    utACK f110b7c722eb150816a26cab161ac2b8c0f58609
11. meshcollider merged this on Aug 13, 2020
12. meshcollider closed this on Aug 13, 2020

    ---

13. sidhujag referenced this in commit 166c599bf8 on Aug 14, 2020
14. theStack deleted the branch on Dec 1, 2020
15. Fabcien referenced this in commit 1241ef50e9 on Sep 10, 2021
16. DrahtBot locked this on Feb 15, 2022

Contributors
theStack adaminsky laanwj achow101 meshcollider

Labels
Wallet RPC/REST/ZMQ Mining