refactor: consolidate sendmany and sendtoaddress code by Sjors · Pull Request #18202 · bitcoin/bitcoin

Sjors · 2020-02-24T19:13:09Z

I consolidated code between these two RPC calls, since sendtoaddress is essentially sendmany with 1 destination.

Unless I overlooked something, the only behaviour change is that some sendtoaddress error codes changed from -4 to -6. The release note mentions this.

Salvaged from #18201.

DrahtBot · 2020-02-24T20:52:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)

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.

Sjors · 2020-03-27T11:23:36Z

Rebased and added a helper function ParseRecipients() so that SendMoney is now a lot shorter.

instagibbs · 2020-04-24T20:10:30Z

reACK a6e1761

even shorter nice

Sjors · 2020-05-04T10:53:56Z

Rebased after #16426

Sjors · 2020-05-04T18:37:41Z

Rebased again after #18699

The only new behavior is some error codes are changed from -4 to -6.

fjahr

Code review ACK 08fc6f6

Nice refactor.

fjahr · 2020-06-22T19:08:14Z

+        for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
+            const UniValue& addr = subtract_fee_outputs[idx];
+            if (addr.get_str() == address) {
+                subtract_fee = true;


Suggested change

subtract_fee = true;

subtract_fee = true;

break;

jonatack

ACK 08fc6f6

Some suggestions below. Feel free to ignore the style nits.

jonatack · 2020-06-23T07:16:47Z

+---------------------
+
+- To make RPC `sendtoaddress` more consistent with `sendmany` the following error
+    `sendtoaddress` codes were changed from `-4` to `-6`:


08fc6f6 suggestion

-- To make RPC `sendtoaddress` more consistent with `sendmany` the following error - `sendtoaddress` codes were changed from `-4` to `-6`: +- To make RPC `sendtoaddress` more consistent with `sendmany`, the following + `sendtoaddress` error codes are changed from `-4` to `-6`:

jonatack · 2020-06-23T09:27:11Z

-    if (nValue > curBalance)
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds");
+        bool subtract_fee = false;
+        for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {


while retouching this code, could update to the current convention

Suggested change

for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {

for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); ++idx) {

jonatack · 2020-06-23T09:31:34Z

+    std::vector<CRecipient> recipients;
+    ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
+
+    return SendMoney(pwallet, coin_control, recipients, mapValue);


can use move for mapValue here as well?

Suggested change

return SendMoney(pwallet, coin_control, recipients, mapValue);

return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));

Sjors · 2020-06-23T09:43:38Z

Thanks, I'll address the above nits if this PR needs touching.

meshcollider

Code review & functional test run ACK 08fc6f6

Nits can be left for followup

Update the Namecoin-specific wallet code (e.g. sending transactions with name inputs, sendtomany) for the upstream refactor in bitcoin/bitcoin#18202.

Summary: The only new behavior is some error codes are changed from -4 to -6. This is a backport of [[bitcoin/bitcoin#18202 | core#18202]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9993

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from 7061c2c to 44ec825 Compare February 24, 2020 19:13

DrahtBot added Build system Docs GUI Refactoring RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Feb 24, 2020

fanquake removed Build system Docs GUI Tests Utils/log/libs labels Feb 25, 2020

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from 771ca9f to c23298f Compare March 27, 2020 11:22

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from c23298f to a6e1761 Compare March 27, 2020 12:58

This was referenced Apr 5, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

rpc: replace raw pointers with shared_ptrs #18592

Closed

This was referenced Apr 17, 2020

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

wallet: Avoid translating RPC errors #18699

Merged

Sjors requested review from instagibbs and laanwj April 23, 2020 17:11

Sjors requested a review from sipa April 26, 2020 11:59

DrahtBot added the Needs rebase label May 1, 2020

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from a6e1761 to ed2fc36 Compare May 4, 2020 10:51

DrahtBot added Needs rebase and removed Needs rebase labels May 4, 2020

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from ed2fc36 to 443a740 Compare May 4, 2020 18:37

DrahtBot removed the Needs rebase label May 4, 2020

[rpc] refactor: consolidate sendmany and sendtoaddress code

08fc6f6

The only new behavior is some error codes are changed from -4 to -6.

Sjors force-pushed the 2020/02/refactor_sendmany_sendtoaddress branch from 443a740 to 08fc6f6 Compare June 19, 2020 09:29

fjahr approved these changes Jun 22, 2020

View reviewed changes

jonatack reviewed Jun 23, 2020

View reviewed changes

meshcollider reviewed Jul 12, 2020

View reviewed changes

meshcollider merged commit 4db44ac into bitcoin:master Jul 12, 2020

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Jul 13, 2020

Merge branch 'auxpow'

e720687

Update the Namecoin-specific wallet code (e.g. sending transactions with name inputs, sendtomany) for the upstream refactor in bitcoin/bitcoin#18202.

Sjors deleted the 2020/02/refactor_sendmany_sendtoaddress branch July 14, 2020 15:36

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
	for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); ++idx) {

	return SendMoney(pwallet, coin_control, recipients, mapValue);
	return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));

Uh oh!

Conversation

Sjors commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Mar 27, 2020

Uh oh!

instagibbs commented Apr 24, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jun 23, 2020

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Sjors commented Feb 24, 2020 •

edited

Loading

DrahtBot commented Feb 24, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading