LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #11050: Avoid treating null RPC arguments different from missing arguments

Browse source 
745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky)
fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky)
e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky)
e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky)

Pull request description:

  This is a followup to #10783.

  - The first commit doesn't change behavior at all, just simplifies code.
  - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors.
  - The third commit updates developer notes after the cleanup.
  - The forth commit does some additional code cleanup in `getbalance`.

  Followup changes that should happen in future PRs:

  - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133850525
  - [ ] Add braces around if statements. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133851133
  - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133829303

Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9
This commit is contained in:
Wladimir J. van der Laan 2017-08-22 09:24:31 +02:00
commit 7ed57d3d7c
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
7 changed files with 71 additions and 61 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
doc/developer-notes.md
View file

@ -572,16 +572,14 @@ A few guidelines for introducing and reviewing new RPC interfaces:
is specified as-is in BIP22.
- Missing arguments and 'null' should be treated the same: as default values. If there is no
default value, both cases should fail in the same way.
default value, both cases should fail in the same way. The easiest way to follow this
guideline is detect unspecified arguments with `params[x].isNull()` instead of
`params.size() <= x`. The former returns true if the argument is either null or missing,
while the latter returns true if is missing, and false if it is null.
- *Rationale*: Avoids surprises when switching to name-based arguments. Missing name-based arguments
are passed as 'null'.
- *Exception*: Many legacy exceptions to this exist, one of the worst ones is
`getbalance` which follows a completely different code path based on the
number of arguments. We are still in the process of cleaning these up. Do not introduce
new ones.
- Try not to overload methods on argument type. E.g. don't make `getblock(true)` and `getblock("hash")`
do different things.

4
src/rpc/blockchain.cpp
View file

@ -1489,11 +1489,11 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
const CBlockIndex* pindex;
int blockcount = 30 * 24 * 60 * 60 / Params().GetConsensus().nPowTargetSpacing; // By default: 1 month
if (request.params.size() > 0 && !request.params[0].isNull()) {
if (!request.params[0].isNull()) {
blockcount = request.params[0].get_int();
}
bool havehash = request.params.size() > 1 && !request.params[1].isNull();
bool havehash = !request.params[1].isNull();
uint256 hash;
if (havehash) {
hash = uint256S(request.params[1].get_str());

2
src/rpc/mining.cpp
View file

@ -842,7 +842,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
unsigned int conf_target = ParseConfirmTarget(request.params[0]);
bool conservative = true;
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
FeeEstimateMode fee_mode;
if (!FeeModeFromString(request.params[1].get_str(), fee_mode)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");

6
src/rpc/misc.cpp
View file

@ -552,7 +552,7 @@ UniValue getmemoryinfo(const JSONRPCRequest& request)
+ HelpExampleRpc("getmemoryinfo", "")
);
std::string mode = (request.params.size() < 1 || request.params[0].isNull()) ? "stats" : request.params[0].get_str();
std::string mode = request.params[0].isNull() ? "stats" : request.params[0].get_str();
if (mode == "stats") {
UniValue obj(UniValue::VOBJ);
obj.push_back(Pair("locked", RPCLockedMemoryInfo()));
@ -603,11 +603,11 @@ UniValue logging(const JSONRPCRequest& request)
}
uint32_t originalLogCategories = logCategories;
if (request.params.size() > 0 && request.params[0].isArray()) {
if (request.params[0].isArray()) {
logCategories |= getCategoryMask(request.params[0]);
}
if (request.params.size() > 1 && request.params[1].isArray()) {
if (request.params[1].isArray()) {
logCategories &= ~getCategoryMask(request.params[1]);
}

12
src/rpc/net.cpp
View file

@ -193,7 +193,7 @@ UniValue getpeerinfo(const JSONRPCRequest& request)
UniValue addnode(const JSONRPCRequest& request)
{
std::string strCommand;
if (request.params.size() == 2)
if (!request.params[1].isNull())
strCommand = request.params[1].get_str();
if (request.fHelp || request.params.size() != 2 ||
(strCommand != "onetry" && strCommand != "add" && strCommand != "remove"))
@ -258,7 +258,7 @@ UniValue disconnectnode(const JSONRPCRequest& request)
bool success;
const UniValue &address_arg = request.params[0];
const UniValue &id_arg = request.params.size() < 2 ? NullUniValue : request.params[1];
const UniValue &id_arg = request.params[1];
if (!address_arg.isNull() && id_arg.isNull()) {
/* handle disconnect-by-address */
@ -311,7 +311,7 @@ UniValue getaddednodeinfo(const JSONRPCRequest& request)
std::vector<AddedNodeInfo> vInfo = g_connman->GetAddedNodeInfo();
if (request.params.size() == 1 && !request.params[0].isNull()) {
if (!request.params[0].isNull()) {
bool found = false;
for (const AddedNodeInfo& info : vInfo) {
if (info.strAddedNode == request.params[0].get_str()) {
@ -490,7 +490,7 @@ UniValue getnetworkinfo(const JSONRPCRequest& request)
UniValue setban(const JSONRPCRequest& request)
{
std::string strCommand;
if (request.params.size() >= 2)
if (!request.params[1].isNull())
strCommand = request.params[1].get_str();
if (request.fHelp || request.params.size() < 2 ||
(strCommand != "add" && strCommand != "remove"))
@ -534,11 +534,11 @@ UniValue setban(const JSONRPCRequest& request)
throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: IP/Subnet already banned");
int64_t banTime = 0; //use standard bantime if not specified
if (request.params.size() >= 3 && !request.params[2].isNull())
if (!request.params[2].isNull())
banTime = request.params[2].get_int64();
bool absolute = false;
if (request.params.size() == 4 && request.params[3].isTrue())
if (request.params[3].isTrue())
absolute = true;
isSubnet ? g_connman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute) : g_connman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);

12
src/rpc/rawtransaction.cpp
View file

@ -336,14 +336,14 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
CMutableTransaction rawTx;
if (request.params.size() > 2 && !request.params[2].isNull()) {
if (!request.params[2].isNull()) {
int64_t nLockTime = request.params[2].get_int64();
if (nLockTime < 0 || nLockTime > std::numeric_limits<uint32_t>::max())
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range");
rawTx.nLockTime = nLockTime;
}
bool rbfOptIn = request.params.size() > 3 ? request.params[3].isTrue() : false;
bool rbfOptIn = request.params[3].isTrue();
for (unsigned int idx = 0; idx < inputs.size(); idx++) {
const UniValue& input = inputs[idx];
@ -732,7 +732,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
bool fGivenKeys = false;
CBasicKeyStore tempKeystore;
if (request.params.size() > 2 && !request.params[2].isNull()) {
if (!request.params[2].isNull()) {
fGivenKeys = true;
UniValue keys = request.params[2].get_array();
for (unsigned int idx = 0; idx < keys.size(); idx++) {
@ -754,7 +754,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
#endif
// Add previous txouts given in the RPC call:
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
UniValue prevTxs = request.params[1].get_array();
for (unsigned int idx = 0; idx < prevTxs.size(); idx++) {
const UniValue& p = prevTxs[idx];
@ -825,7 +825,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
#endif
int nHashType = SIGHASH_ALL;
if (request.params.size() > 3 && !request.params[3].isNull()) {
if (!request.params[3].isNull()) {
static std::map<std::string, int> mapSigHashValues = {
{std::string("ALL"), int(SIGHASH_ALL)},
{std::string("ALL|ANYONECANPAY"), int(SIGHASH_ALL|SIGHASH_ANYONECANPAY)},
@ -919,7 +919,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
const uint256& hashTx = tx->GetHash();
CAmount nMaxRawTxFee = maxTxFee;
if (request.params.size() > 1 && request.params[1].get_bool())
if (!request.params[1].isNull() && request.params[1].get_bool())
nMaxRawTxFee = 0;
CCoinsViewCache &view = *pcoinsTip;

86
src/wallet/rpcwallet.cpp
View file

@ -280,7 +280,7 @@ UniValue setaccount(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
std::string strAccount;
if (request.params.size() > 1)
if (!request.params[1].isNull())
strAccount = AccountFromValue(request.params[1]);
// Only add the account if the address is yours.
@ -462,26 +462,26 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
// Wallet comments
CWalletTx wtx;
if (request.params.size() > 2 && !request.params[2].isNull() && !request.params[2].get_str().empty())
if (!request.params[2].isNull() && !request.params[2].get_str().empty())
wtx.mapValue["comment"] = request.params[2].get_str();
if (request.params.size() > 3 && !request.params[3].isNull() && !request.params[3].get_str().empty())
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
wtx.mapValue["to"] = request.params[3].get_str();
bool fSubtractFeeFromAmount = false;
if (request.params.size() > 4 && !request.params[4].isNull()) {
if (!request.params[4].isNull()) {
fSubtractFeeFromAmount = request.params[4].get_bool();
}
CCoinControl coin_control;
if (request.params.size() > 5 && !request.params[5].isNull()) {
if (!request.params[5].isNull()) {
coin_control.signalRbf = request.params[5].get_bool();
}
if (request.params.size() > 6 && !request.params[6].isNull()) {
if (!request.params[6].isNull()) {
coin_control.m_confirm_target = ParseConfirmTarget(request.params[6]);
}
if (request.params.size() > 7 && !request.params[7].isNull()) {
if (!request.params[7].isNull()) {
if (!FeeModeFromString(request.params[7].get_str(), coin_control.m_fee_mode)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
}
@ -768,18 +768,31 @@ UniValue getbalance(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);
if (request.params.size() == 0)
return ValueFromAmount(pwallet->GetBalance());
const UniValue& account_value = request.params[0];
const UniValue& minconf = request.params[1];
const UniValue& include_watchonly = request.params[2];
const std::string& account_param = request.params[0].get_str();
if (account_value.isNull()) {
if (!minconf.isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER,
"getbalance minconf option is only currently supported if an account is specified");
}
if (!include_watchonly.isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER,
"getbalance include_watchonly option is only currently supported if an account is specified");
}
return ValueFromAmount(pwallet->GetBalance());
}
const std::string& account_param = account_value.get_str();
const std::string* account = account_param != "*" ? &account_param : nullptr;
int nMinDepth = 1;
if (!request.params[1].isNull())
nMinDepth = request.params[1].get_int();
if (!minconf.isNull())
nMinDepth = minconf.get_int();
isminefilter filter = ISMINE_SPENDABLE;
if(!request.params[2].isNull())
if(request.params[2].get_bool())
if(!include_watchonly.isNull())
if(include_watchonly.get_bool())
filter = filter | ISMINE_WATCH_ONLY;
return ValueFromAmount(pwallet->GetLegacyBalance(filter, nMinDepth, account));
@ -838,11 +851,11 @@ UniValue movecmd(const JSONRPCRequest& request)
CAmount nAmount = AmountFromValue(request.params[2]);
if (nAmount <= 0)
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
if (request.params.size() > 3)
if (!request.params[3].isNull())
// unused parameter, used to be nMinDepth, keep type-checking it though
(void)request.params[3].get_int();
std::string strComment;
if (request.params.size() > 4)
if (!request.params[4].isNull())
strComment = request.params[4].get_str();
if (!pwallet->AccountMove(strFrom, strTo, nAmount, strComment)) {
@ -899,14 +912,14 @@ UniValue sendfrom(const JSONRPCRequest& request)
if (nAmount <= 0)
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
int nMinDepth = 1;
if (request.params.size() > 3)
if (!request.params[3].isNull())
nMinDepth = request.params[3].get_int();
CWalletTx wtx;
wtx.strFromAccount = strAccount;
if (request.params.size() > 4 && !request.params[4].isNull() && !request.params[4].get_str().empty())
if (!request.params[4].isNull() && !request.params[4].get_str().empty())
wtx.mapValue["comment"] = request.params[4].get_str();
if (request.params.size() > 5 && !request.params[5].isNull() && !request.params[5].get_str().empty())
if (!request.params[5].isNull() && !request.params[5].get_str().empty())
wtx.mapValue["to"] = request.params[5].get_str();
EnsureWalletIsUnlocked(pwallet);
@ -986,23 +999,23 @@ UniValue sendmany(const JSONRPCRequest& request)
CWalletTx wtx;
wtx.strFromAccount = strAccount;
if (request.params.size() > 3 && !request.params[3].isNull() && !request.params[3].get_str().empty())
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
wtx.mapValue["comment"] = request.params[3].get_str();
UniValue subtractFeeFromAmount(UniValue::VARR);
if (request.params.size() > 4 && !request.params[4].isNull())
if (!request.params[4].isNull())
subtractFeeFromAmount = request.params[4].get_array();
CCoinControl coin_control;
if (request.params.size() > 5 && !request.params[5].isNull()) {
if (!request.params[5].isNull()) {
coin_control.signalRbf = request.params[5].get_bool();
}
if (request.params.size() > 6 && !request.params[6].isNull()) {
if (!request.params[6].isNull()) {
coin_control.m_confirm_target = ParseConfirmTarget(request.params[6]);
}
if (request.params.size() > 7 && !request.params[7].isNull()) {
if (!request.params[7].isNull()) {
if (!FeeModeFromString(request.params[7].get_str(), coin_control.m_fee_mode)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
}
@ -1105,7 +1118,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);
std::string strAccount;
if (request.params.size() > 2)
if (!request.params[2].isNull())
strAccount = AccountFromValue(request.params[2]);
// Construct using pay-to-script-hash:
@ -1711,10 +1724,10 @@ UniValue listaccounts(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);
int nMinDepth = 1;
if (request.params.size() > 0)
if (!request.params[0].isNull())
nMinDepth = request.params[0].get_int();
isminefilter includeWatchonly = ISMINE_SPENDABLE;
if(request.params.size() > 1)
if(!request.params[1].isNull())
if(request.params[1].get_bool())
includeWatchonly = includeWatchonly | ISMINE_WATCH_ONLY;
@ -2363,19 +2376,18 @@ UniValue lockunspent(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);
if (request.params.size() == 1)
RPCTypeCheck(request.params, {UniValue::VBOOL});
else
RPCTypeCheck(request.params, {UniValue::VBOOL, UniValue::VARR});
RPCTypeCheckArgument(request.params[0], UniValue::VBOOL);
bool fUnlock = request.params[0].get_bool();
if (request.params.size() == 1) {
if (request.params[1].isNull()) {
if (fUnlock)
pwallet->UnlockAllCoins();
return true;
}
RPCTypeCheckArgument(request.params[1], UniValue::VARR);
UniValue outputs = request.params[1].get_array();
for (unsigned int idx = 0; idx < outputs.size(); idx++) {
const UniValue& output = outputs[idx];
@ -2672,19 +2684,19 @@ UniValue listunspent(const JSONRPCRequest& request)
);
int nMinDepth = 1;
if (request.params.size() > 0 && !request.params[0].isNull()) {
if (!request.params[0].isNull()) {
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
nMinDepth = request.params[0].get_int();
}
int nMaxDepth = 9999999;
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
RPCTypeCheckArgument(request.params[1], UniValue::VNUM);
nMaxDepth = request.params[1].get_int();
}
std::set<CBitcoinAddress> setAddress;
if (request.params.size() > 2 && !request.params[2].isNull()) {
if (!request.params[2].isNull()) {
RPCTypeCheckArgument(request.params[2], UniValue::VARR);
UniValue inputs = request.params[2].get_array();
for (unsigned int idx = 0; idx < inputs.size(); idx++) {
@ -2699,7 +2711,7 @@ UniValue listunspent(const JSONRPCRequest& request)
}
bool include_unsafe = true;
if (request.params.size() > 3 && !request.params[3].isNull()) {
if (!request.params[3].isNull()) {
RPCTypeCheckArgument(request.params[3], UniValue::VBOOL);
include_unsafe = request.params[3].get_bool();
}
@ -3114,7 +3126,7 @@ UniValue generate(const JSONRPCRequest& request)
int num_generate = request.params[0].get_int();
uint64_t max_tries = 1000000;
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
max_tries = request.params[1].get_int();
}