Move SendMoney() to rpcwallet.cpp. #5399

pull paveljanik wants to merge 2 commits into bitcoin:master from paveljanik:movesendmoney changing 3 files +37 −48

paveljanik commented at 7:13 PM on December 1, 2014: contributor

This is proposed variant to #5393. SendMoney() moved from wallet.cpp/.h to rpcwallet.cpp. RPC errors are not localized. SendMoney directly throws JSONRPCErrors instead of returning strings and guessing the proper error code. SendMoney also checks for available funds so RPC_WALLET_INSUFFICIENT_FUNDS (-6) is returned properly (fixes #3007).

in src/rpcwallet.cpp:None in ccf44cbe68 outdated

 820 | @@ -786,15 +821,7 @@ Value sendfrom(const Array& params, bool fHelp)
 821 |  
 822 |      EnsureWalletIsUnlocked();
 823 |  
 824 | -    // Check funds
 825 | -    CAmount nBalance = GetAccountBalance(strAccount, nMinDepth, ISMINE_SPENDABLE);

laanwj commented at 5:43 AM on December 2, 2014:

You removed the account balance check!

laanwj commented at 5:44 AM on December 2, 2014: member

Thanks a lot utACK apart from above comment, I'd like to get rid of accounts as much as you do, but suddenly losing that check is going to result in confusion at least.
laanwj added the label Wallet on Dec 2, 2014
Move SendMoney() to rpcwallet.cpp. b93173dee9
paveljanik force-pushed on Dec 2, 2014
paveljanik commented at 7:33 AM on December 2, 2014: contributor

The check was removed on purpose. Before this change, SendMoney did NOT throw anything, it just returned strings and then the calling code threw generic RPC error. This was the cause of the issue #3007. Now we do not return anything but we directly throw JSONRPCError. This means it doesn't make sense to check GetBalance() twice. Once outside this function in the calling code and inside for sure in the SendMoney(). BUT if I look correctly, the GetBalance check is for an account, so yes, I have to bring it back again - good catch! I have to read better next time. Thank you.
laanwj commented at 8:11 AM on December 2, 2014: member

Indeed, that check is account-specific. It is necessary in addition to the global balance check
jtimon commented at 11:28 AM on December 2, 2014: contributor

utACK. Maybe the function should take pwalletMain explicitly as a parameter instead of using the extern global variable directly? Not important but seems more flexible for future refactors (or if someone implements multi-wallet support or something).
laanwj commented at 8:49 AM on December 3, 2014: member

@jtimon I like the idea of passing the wallet as an argument, although indeed the need for that is cosmetic right now.

in src/rpcwallet.cpp:None in b93173dee9 outdated

 309 | @@ -309,6 +310,42 @@ Value getaddressesbyaccount(const Array& params, bool fHelp)
 310 |      return ret;
 311 |  }
 312 |  
 313 | +void SendMoney(const CTxDestination &address, CAmount nValue, CWalletTx& wtxNew)
 314 | +{
 315 | +    // Check amount
 316 | +    if (nValue <= 0)
 317 | +        throw JSONRPCError(RPC_WALLET_ERROR, "Invalid amount");

laanwj commented at 3:41 PM on December 5, 2014:

~~Shouldn't this be RPC_WALLET_INSUFFICIENT_FUNDS too?~~ Ooops, no. Maybe RPC_INVALID_PARAMETER.

paveljanik commented at 3:59 PM on December 5, 2014:

I moved the code, not changed :-) I think RPC_INVALID_PARAMETER is better, yes.

in src/rpcwallet.cpp:None in b93173dee9 outdated

 341 | +        throw JSONRPCError(RPC_WALLET_ERROR, strError);
 342 | +    }
 343 | +    if (!pwalletMain->CommitTransaction(wtxNew, reservekey))
 344 | +        throw JSONRPCError(RPC_WALLET_ERROR, "Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here.");
 345 | +
 346 | +    return;

laanwj commented at 3:43 PM on December 5, 2014:

No need for a return statement at end of function if you return nothing :)

paveljanik commented at 4:08 PM on December 5, 2014: contributor

Ad return: 1. it is also used in other functions - see e.g. void CWallet::EraseFromWallet(const uint256 &hash). 2. I'm used to it. But sure I can remove it, if you want to.
Use RPC_INVALID_PARAMETER instead of RPC_WALLET_ERROR for invalid amount.
No return at the end of void function.
4be639eaec
Diapolo commented at 2:19 PM on December 9, 2014: none

I'm sorry, didn't even bother to look :-(... now we've got this twice ^^ (#5452). And pretty similar also...
laanwj commented at 2:35 PM on December 9, 2014: member

@diapolo This one was first, and 'Ive already reviewed this one, so please rebase to this.
Diapolo commented at 2:37 PM on December 9, 2014: none

No problem, if this is merged I'm rebasing my pull :).
laanwj merged this on Dec 10, 2014
laanwj closed this on Dec 10, 2014
laanwj referenced this in commit 34468066ff on Dec 10, 2014
paveljanik deleted the branch on Dec 10, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#3007 sendtoaddress() returns wrong error code for message 'Insufficient funds'#5393 Check for available funds before call to SendMoney.