Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

listreceivedbyaddress Filter Address #9503

Closed
wants to merge 2 commits into from
Closed

listreceivedbyaddress Filter Address #9503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c60de5d
Add address filtering to listreceivedbyaddress
JeremyRubin Jan 9, 2017
e6f053a
Add tests of listreceivedbyaddress address filtering
JeremyRubin Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions qa/rpc-tests/receivedby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,24 @@ def run_test(self):
assert_array_result(self.nodes[1].listreceivedbyaddress(11),{"address":addr},{ },True)

#Empty Tx
addr = self.nodes[1].getnewaddress()
empty_addr = self.nodes[1].getnewaddress()
assert_array_result(self.nodes[1].listreceivedbyaddress(0,True),
{"address":addr},
{"address":addr, "account":"", "amount":0, "confirmations":0, "txids":[]})
{"address":empty_addr},
{"address":empty_addr, "account":"", "amount":0, "confirmations":0, "txids":[]})

#Test Address filtering
#Only on addr
expected = {"address":addr, "account":"", "amount":Decimal("0.1"), "confirmations":10, "txids":[txid,]}
res = self.nodes[1].listreceivedbyaddress(0, True, True, addr)
assert_array_result(res, {"address":addr}, expected)
if len(res) != 1:
raise AssertionError("listreceivedbyaddress expected only 1 result")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you do

assert_equal(len(res), 1)

instead, here? That way the resulting len(res) would be visible in the output.


#Not on addr
other_addr = self.nodes[0].getnewaddress() # note on node[0]! just a random addr
res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr)
if res != []:
raise AssertionError("Should not have listed any transactions, got\n%s"%res)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can use assert_equal here too.


'''
getreceivedbyaddress Test
Expand Down
43 changes: 36 additions & 7 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,17 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if(params[2].get_bool())
filter = filter | ISMINE_WATCH_ONLY;

bool fFilterAddress = false;
CTxDestination filterAddress = CNoDestination();
if (!fByAccounts && params.size() > 3) {
filterAddress = CBitcoinAddress(params[3].get_str()).Get();
CTxDestination nulladdress = CNoDestination();
if (filterAddress == nulladdress) {
throw JSONRPCError(RPC_WALLET_ERROR, "only_address parameter was invalid");
}
fFilterAddress = true;
}

// Tally
map<CBitcoinAddress, tallyitem> mapTally;
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
Expand All @@ -1164,6 +1175,10 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
if (!ExtractDestination(txout.scriptPubKey, address))
continue;

if (fFilterAddress && !(filterAddress == address)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not fFilterAddress && filterAddress != address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a != b) is not the same as !(a == b). I don't think != is defined here iirc.

Copy link
Member

@kallewoof kallewoof Mar 1, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the same only in the sense that the != operator may not be defined, right? Semantically they're identical. And yeah, I see now. CNoDestination has no operator!=.

I wonder if it would be worth adding the one liner

friend bool operator!=(const CNoDestination &a, const CNoDestination &b) { return false; }

to make this line look less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except it's not a one liner, it needs to be added for all sorts of classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a CNoDestination, it's a CTxDestination, which is a boost variant. Some supported versions of boost do not support operator!= for variants.

Copy link
Member

@kallewoof kallewoof Mar 1, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought if all variants accepted the operator the variant would accept it, in which case adding it to CNoDestination would be enough, but maybe I'm off on that one.
Edit: re-read your response; okay, didn't know that. Gotcha.

continue;
}

isminefilter mine = IsMine(*pwalletMain, address);
if(!(mine & filter))
continue;
Expand All @@ -1180,10 +1195,23 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
// Reply
UniValue ret(UniValue::VARR);
map<string, tallyitem> mapAccountTally;
BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CAddressBookData)& item, pwalletMain->mapAddressBook)

// Create mapAddressBook iterator
// If we aren't filtering, go from begin() to end()
std::map<CTxDestination, CAddressBookData>::const_iterator start = pwalletMain->mapAddressBook.begin();
std::map<CTxDestination, CAddressBookData>::const_iterator end = pwalletMain->mapAddressBook.end();
// If we are filtering, find() the applicable entry
if (fFilterAddress) {
start = pwalletMain->mapAddressBook.find(filterAddress);
if (start != end) {
end = std::next(start);
}
}

for(std::map<CTxDestination, CAddressBookData>::const_iterator item_it = start; item_it != end; ++item_it)
{
const CBitcoinAddress& address = item.first;
const string& strAccount = item.second.name;
const CBitcoinAddress& address = item_it->first;
const string& strAccount = item_it->second.name;
map<CBitcoinAddress, tallyitem>::iterator it = mapTally.find(address);
if (it == mapTally.end() && !fIncludeEmpty)
continue;
Expand Down Expand Up @@ -1253,15 +1281,15 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request)
if (!EnsureWalletIsAvailable(request.fHelp))
return NullUniValue;

if (request.fHelp || request.params.size() > 3)
if (request.fHelp || request.params.size() > 4)
throw runtime_error(
"listreceivedbyaddress ( minconf include_empty include_watchonly)\n"
"listreceivedbyaddress (minconf include_empty include_watchonly only_address)\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: standard elsewhere is to show optionals as "( opt1 opt2 ... )", not "(opt1 opt2 ...)" (i.e. instead of removing starting space, add ending space)

"\nList balances by receiving address.\n"
"\nArguments:\n"
"1. minconf (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n"
"2. include_empty (bool, optional, default=false) Whether to include addresses that haven't received any payments.\n"
"3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n"

"4. only_address (string, optional) If present, only return information on this address. Otherwise, return all information.\n"
"\nResult:\n"
"[\n"
" {\n"
Expand All @@ -1283,6 +1311,7 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request)
+ HelpExampleCli("listreceivedbyaddress", "")
+ HelpExampleCli("listreceivedbyaddress", "6 true")
+ HelpExampleRpc("listreceivedbyaddress", "6, true, true")
+ HelpExampleRpc("listreceivedbyaddress", "6, true, true, \"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\"")
);

LOCK2(cs_main, pwalletMain->cs_wallet);
Expand Down Expand Up @@ -2940,7 +2969,7 @@ static const CRPCCommand commands[] =
{ "wallet", "listaddressgroupings", &listaddressgroupings, false, {} },
{ "wallet", "listlockunspent", &listlockunspent, false, {} },
{ "wallet", "listreceivedbyaccount", &listreceivedbyaccount, false, {"minconf","include_empty","include_watchonly"} },
{ "wallet", "listreceivedbyaddress", &listreceivedbyaddress, false, {"minconf","include_empty","include_watchonly"} },
{ "wallet", "listreceivedbyaddress", &listreceivedbyaddress, false, {"minconf","include_empty","include_watchonly", "only_address"} },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: abide by surrounding style of no space after comma

{ "wallet", "listsinceblock", &listsinceblock, false, {"blockhash","target_confirmations","include_watchonly"} },
{ "wallet", "listtransactions", &listtransactions, false, {"account","count","skip","include_watchonly"} },
{ "wallet", "listunspent", &listunspent, false, {"minconf","maxconf","addresses","include_unsafe"} },
Expand Down