wallet, bugfix: allow send with string fee_rate amounts #20573

1. 
   jonatack commented at 9:52 pm on December 4, 2020: member

   RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage.
2. wallet, bugfix: allow send to take string fee rate values ce207d6b93
3. test: add coverage for passing fee rate as a string 6fa72ceb80
4. 
   jonatack commented at 9:53 pm on December 4, 2020: member

   Found while adding tests for #20546.
5. DrahtBot added the label RPC/REST/ZMQ on Dec 4, 2020
6. DrahtBot added the label Wallet on Dec 4, 2020
7. 
   DrahtBot commented at 11:13 pm on December 4, 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:

   • #20546 (policy, wallet, refactor: check for non-representable CFeeRates by jonatack)
   • #20391 (wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack)
   • #20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)

   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.

8. 
   MarcoFalke commented at 5:29 am on December 5, 2020: member

   review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
9. in src/wallet/rpcwallet.cpp:4092 in 6fa72ceb80

   4088@@ -4089,7 +4089,7 @@ static RPCHelpMan send()
   4089                 UniValueType(), // outputs (ARR or OBJ, checked later)
   4090                 UniValue::VNUM, // conf_target
   4091                 UniValue::VSTR, // estimate_mode
   4092-                UniValue::VNUM, // fee_rate
   

   ---

   ---

   

   promag commented at 1:25 pm on December 5, 2020:

   How about removing this RPCTypeCheck (tests still pass as far as I can see)?

   ---

   

   jonatack commented at 12:02 pm on December 7, 2020:

   I followed the practice seen in the file and the codebase when grepping UniValueType() and it seems to have documentation value for future readers of the code.

   ---

   

   promag commented at 8:43 pm on December 9, 2020:

   That’s fine. I just want to point that if removing some code makes no difference then it’s because its redundant or some test is missing.
10. 
    achow101 commented at 4:47 pm on December 9, 2020: member

    Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
11. MarcoFalke added the label Needs backport (0.21) on Dec 9, 2020
12. MarcoFalke added this to the milestone 0.21.0 on Dec 9, 2020
13. 
    promag commented at 8:44 pm on December 9, 2020: member

    Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923.
14. MarcoFalke merged this on Dec 10, 2020
15. MarcoFalke closed this on Dec 10, 2020

    ---

16. MarcoFalke referenced this in commit 06c84232b3 on Dec 10, 2020
17. MarcoFalke referenced this in commit 0d3c140c4d on Dec 10, 2020
18. 
    MarcoFalke commented at 10:50 am on December 10, 2020: member

    Backported in #20612
19. MarcoFalke removed the label Needs backport (0.21) on Dec 10, 2020
20. jonatack deleted the branch on Dec 10, 2020
21. sidhujag referenced this in commit 43cc489a95 on Dec 10, 2020
22. DrahtBot locked this on Feb 15, 2022

Contributors
jonatack DrahtBot MarcoFalke promag achow101

Labels
Wallet RPC/REST/ZMQ

Milestone
0.21.0