Send RPC bug fix and touch-ups #19969

pull Sjors wants to merge 4 commits into bitcoin:master from Sjors:2020/09/send_touchups changing 4 files +39 −32

Sjors commented at 1:31 PM on September 17, 2020: member

Followup based on #16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough).

I marked the RPC as experimental so we can tweak it a bit over the next release cycle.
Mark send RPC experimental efc9b85e6f
Sjors cross-referenced this on Sep 17, 2020 from issue The ultimate send RPC by Sjors
fanquake added the label RPC/REST/ZMQ on Sep 17, 2020
[rpc] send: fix parsing replaceable option 0fc1c685e1
Sjors renamed this:
~~Send RPC touch-ups~~
Send RPC bug fix and touch-ups
on Sep 17, 2020
[rpc] send: various touch-ups d813d26f06
rpc: add brackets to ConstructTransaction f7b331ea85
Sjors force-pushed on Sep 17, 2020
fjahr commented at 11:14 PM on September 17, 2020: contributor

utACK f7b331ea85d45c7337e527b6e77a45da7a689b7d
DrahtBot commented at 1:25 PM on September 19, 2020: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20013 (rpc: normalize parameter names by kallewoof)
- #16546 (External signer support - Wallet Box edition by Sjors)
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.
DrahtBot cross-referenced this on Sep 19, 2020 from issue External signer support - Wallet Box edition by Sjors

in src/wallet/rpcwallet.cpp:3880 in f7b331ea85

3873 | @@ -3874,9 +3874,10 @@ static UniValue listlabels(const JSONRPCRequest& request)
3874 |  static RPCHelpMan send()
3875 |  {
3876 |      return RPCHelpMan{"send",
3877 | +        "\nEXPERIMENTAL warning: this call may be changed in future releases.\n"
3878 |          "\nSend a transaction.\n",
3879 |          {
3880 | -            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "a json array with outputs (key-value pairs), where none of the keys are duplicated.\n"
3881 | +            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "A JSON array with outputs (key-value pairs), where none of the keys are duplicated.\n"

MarcoFalke commented at 7:28 AM on September 20, 2020:

            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "Outputs (key-value pairs), where none of the keys are duplicated.\n"

The whole point of RPCHelpMan is to deal with types and formatting, so that the documentation strings can be minimal.

If you repeat the types, it will be rendered twice:

1. outputs                               (json array, required) a json array with outputs (key-value pairs), where none of the keys are duplicated.

Sjors commented at 2:29 PM on September 21, 2020:

Will fix if there's other issues (or just later)

MarcoFalke commented at 7:29 AM on September 20, 2020: member

left a style nit
meshcollider commented at 3:39 AM on September 25, 2020: contributor

utACK f7b331ea85d45c7337e527b6e77a45da7a689b7d
DrahtBot cross-referenced this on Sep 25, 2020 from issue rpc: normalize parameter names by kallewoof
kallewoof commented at 4:35 AM on September 28, 2020: member

ACK f7b331ea85d45c7337e527b6e77a45da7a689b7d

I find it weird that even though I used "1 sat/b" it complains about failing to do fee estimation. It shouldn't need fee estimation at all. Anyway, that's outside the scope of this nit-fix PR.

in doc/release-notes-16378.md:4 in f7b331ea85

   0 | @@ -1,5 +1,6 @@
   1 |  RPC
   2 |  ---
   3 |  - A new `send` RPC with similar syntax to `walletcreatefundedpsbt`, including
   4 | -  support for coin selection and a custom fee rate. Using the new `send` method
   5 | -  is encouraged: `sendmany` and `sendtoaddress` may be deprecated in a future release.
   6 | +  support for coin selection and a custom fee rate. The `send` RPC is experimental

fanquake commented at 6:51 AM on September 29, 2020:

Using it is encouraged once it's no longer experimental: This sentence seems unnecessary, and somewhat confusing. There's no reason not to use it (i.e it being unsafe) as long as you know the interface might change in the future, and that is made clear here and in the help output. Will no-doubt be changed in the wiki pre-release.

fanquake commented at 6:55 AM on September 29, 2020: member

Will merge this now. Going forward please pick a single prefix style for your commits and use it consistently. If anything it can make it slightly easier to generate release notes from commit logs etc. The prevailing style seems to be foo: bar.
fanquake merged this on Sep 29, 2020
fanquake closed this on Sep 29, 2020
fanquake cross-referenced this on Sep 29, 2020 from issue Followup from #19969 by fanquake
sidhujag referenced this in commit 4793d4ff1f on Sep 29, 2020
Sjors deleted the branch on Sep 29, 2020
fanquake cross-referenced this on Sep 30, 2020 from issue doc: Add 19501 release notes by MarcoFalke
deadalnix referenced this in commit be507501c5 on Oct 29, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#16378 The ultimate send RPC #20032 Followup from #19969 #20043 doc: Add 19501 release notes

Send RPC bug fix and touch-ups #19969

Conflicts