refactor: consolidate sendmany and sendtoaddress code #18202
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7061c2c to
44ec825
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Feb 24, 2020
Merged
|
Rebased and added a helper function |
c23298f to
a6e1761
Compare
This was referenced Apr 5, 2020
This was referenced Apr 17, 2020
|
reACK a6e1761 even shorter nice |
a6e1761 to
ed2fc36
Compare
|
Rebased after #16426 |
ed2fc36 to
443a740
Compare
|
Rebased again after #18699 |
The only new behavior is some error codes are changed from -4 to -6.
443a740 to
08fc6f6
Compare
fjahr
approved these changes
Jun 22, 2020
| 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; |
There was a problem hiding this comment.
Suggested change
| subtract_fee = true; | |
| subtract_fee = true; | |
| break; |
jonatack
reviewed
Jun 23, 2020
| --------------------- | ||
|
|
||
| - To make RPC `sendtoaddress` more consistent with `sendmany` the following error | ||
| `sendtoaddress` codes were changed from `-4` to `-6`: |
There was a problem hiding this comment.
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`:| 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++) { |
There was a problem hiding this comment.
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) { |
| std::vector<CRecipient> recipients; | ||
| ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); | ||
|
|
||
| return SendMoney(pwallet, coin_control, recipients, mapValue); |
There was a problem hiding this comment.
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)); |
|
Thanks, I'll address the above nits if this PR needs touching. |
There was a problem hiding this comment.
Code review & functional test run ACK 08fc6f6
Nits can be left for followup
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 12, 2020
… code 08fc6f6 [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost) Pull request description: 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 bitcoin#18201. ACKs for top commit: fjahr: Code review ACK 08fc6f6 jonatack: ACK 08fc6f6 meshcollider: Code review & functional test run ACK 08fc6f6 Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
domob1812
added a commit
to domob1812/namecoin-core
that referenced
this pull request
Jul 13, 2020
Update the Namecoin-specific wallet code (e.g. sending transactions with name inputs, sendtomany) for the upstream refactor in bitcoin/bitcoin#18202.
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Aug 31, 2021
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I consolidated code between these two RPC calls, since
sendtoaddressis essentiallysendmanywith 1 destination.Unless I overlooked something, the only behaviour change is that some
sendtoaddresserror codes changed from-4to-6. The release note mentions this.Salvaged from #18201.