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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
350- if (nValue > curBalance)
351- throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds");
352+ CScript scriptPubKey = GetScriptForDestination(dest);
353+ CAmount nAmount = AmountFromValue(address_amounts[address]);
354+ if (nAmount <= 0)
355+ throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
AmountFromValue already checks the range, so dropping it here and adding a test with a negative value.
CreateTransaction checks as well, heh
308@@ -309,6 +309,9 @@ def run_test(self):
309 assert_equal(tx_obj['amount'], Decimal('-0.0001'))
310
311 # General checks for errors from incorrect inputs
312+ # This will raise an exception because the amount is negative
313+ assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "-1")
createrawtransaction tests.
AmountFromValue? I meant specifically for sendtoaddress. Anyways, a test is a win.
343+ throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address);
344+ }
345+ destinations.insert(dest);
346+
347+ CScript script_pub_key = GetScriptForDestination(dest);
348+ CAmount amount = AmountFromValue(address_amounts[address]);
address_amounts. But I don’t think univalue offers this functionality?
getKeys() and getValues() but not both. A future refactor could convert from UniValue to std::map<CTxDestination, CAmount>, but you have to loop over the UniValue at least once. So if it’s really a performance bottleneck for big transactions, someone would need to patch UniValue to add e.g. bool getStringDictionary(std::map<std::string,std::string>& dict)
Can also do
0int i = 0;
1for (const std::string& address: address_amounts.getKeys()) {
2 CAmount amount = AmountFromValue(address_amounts[i++]);
3 ...
const UniValue& operator[](size_t index) const;
351- if (nValue <= 0)
352- throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid amount");
353+ bool subtract_fee = false;
354+ for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
355+ const UniValue& addr = subtract_fee_outputs[idx];
356+ if (addr.get_str() == address)
subtrace_fee... on the same line.
I was to suggest using ranged for here but that doesn’t work for univalue as there’s no begin() and end().
{} and a std::move(mapValue).
0@@ -0,0 +1,8 @@
1+Low-level RPC Changes
2+---------------------
3+
4+- To make RPC `sendtoaddress` more consisent with `sendmany` the following error
0- To make RPC `sendtoaddress` more consistent with `sendmany` the following error
326@@ -327,36 +327,53 @@ static UniValue setlabel(const JSONRPCRequest& request)
327 return NullUniValue;
328 }
329
330-
331-static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue)
332+UniValue SendMoney(interfaces::Chain::Lock& locked_chain, CWallet * const pwallet, CCoinControl coin_control, UniValue& address_amounts, mapValue_t map_value, UniValue& subtract_fee_outputs)
const CCoinControl & which avoids a copy.
SendMoney function into the new one you’re creating? It seems preferable to create an intermediary function between SendMoney and its RPC call sites rather than merging all the RPC parsing into SendMoney itself.
ParseRecipients() so that SendMoney is now a lot shorter.
reACK https://github.com/bitcoin/bitcoin/pull/18202/commits/a6e1761823c8cd11a6e9338eaedd9301c9b36756
even shorter nice
The only new behavior is some error codes are changed from -4 to -6.
348- throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds");
349+ bool subtract_fee = false;
350+ for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
351+ const UniValue& addr = subtract_fee_outputs[idx];
352+ if (addr.get_str() == address) {
353+ subtract_fee = true;
0 subtract_fee = true;
1 break;
Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Nice refactor.
0@@ -0,0 +1,8 @@
1+Low-level RPC Changes
2+---------------------
3+
4+- To make RPC `sendtoaddress` more consistent with `sendmany` the following error
5+ `sendtoaddress` codes were changed from `-4` to `-6`:
08fc6f6c suggestion
0-- To make RPC `sendtoaddress` more consistent with `sendmany` the following error
1- `sendtoaddress` codes were changed from `-4` to `-6`:
2+- To make RPC `sendtoaddress` more consistent with `sendmany`, the following
3+ `sendtoaddress` error codes are changed from `-4` to `-6`:
345+ CAmount amount = AmountFromValue(address_amounts[i++]);
346
347- if (nValue > curBalance)
348- throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds");
349+ bool subtract_fee = false;
350+ for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
while retouching this code, could update to the current convention
0 for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); ++idx) {
460+ }
461+
462+ std::vector<CRecipient> recipients;
463+ ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
464+
465+ return SendMoney(pwallet, coin_control, recipients, mapValue);
can use move for mapValue here as well?
0 return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Some suggestions below. Feel free to ignore the style nits.
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Nits can be left for followup