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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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");
this string is changing yet I don't see associated test strings changing. Increase coverage?
also good time to add braces to this conditional :)
AmountFromValue already checks the range, so dropping it here and adding a test with a negative value.
even better :) CreateTransaction checks as well, heh
utACK. Since we're touching a bunch of lines modernizing the variable namings to snake_case is probably apt if you don't mind
Rebased and snake-cased.
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")
now that I mention it, I'm a little shocked we don't have coverage on this?
We do for the utility function via createrawtransaction tests.
you mean 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]);
This (order N, so in aggregate, quadratic) lookup would be unnecessary if the loop would iterate over key,value pairs in address_amounts. But I don't think univalue offers this functionality?
There's 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
int i = 0;
for (const std::string& address: address_amounts.getKeys()) {
CAmount amount = AmountFromValue(address_amounts[i++]);
...
Ah, I didn't notice this operator: 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)
Please add braces here as required by the style guidelines, or put the 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().
Done
Nice cleanup, overall!
Concept ACK, looks good
need rebase
Rebased, added safety {} 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
- 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)
Do you intend to pass coin_control by value? In the original code it's a const CCoinControl & which avoids a copy.
Fixed
Is it necessary to merge the old 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.
Rebased and added a helper function 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;
subtract_fee = true;
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
-- 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`:
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
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?
return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Some suggestions below. Feel free to ignore the style nits.
Thanks, I'll address the above nits if this PR needs touching.
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Nits can be left for followup