Skip to content

Commit

Permalink
Rename account to label where appropriate
Browse files Browse the repository at this point in the history
This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
bitcoin#7729, by getting renaming out of the
way and letting it focus on semantics.

The difference between accounts and labels is that labels apply only to
addresses, while accounts apply to both addresses and transactions
(transactions have "from" and "to" accounts). The code associating accounts
with transactions is clumsy and unreliable so we would like get rid of it.
  • Loading branch information
ryanofsky committed Feb 1, 2018
1 parent 41363fe commit 44eb4b4
Show file tree
Hide file tree
Showing 11 changed files with 169 additions and 145 deletions.
12 changes: 12 additions & 0 deletions doc/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ frequently tested on them.
Notable changes
===============

Low-level RPC changes
=====================
- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
parameters have been renamed to `label` with no change in behavior.
- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and
`setlabel` RPCs have been added to replace to be deprecated
`getaccountaddress`, `getreceivedbyaccount`, `listreceivedbyaccount`, and
`setaccount` RPCs with no differences in behavior.
- Wallet `listreceivedbylabel`, `listreceivedbyaccount` and `listunspent` RPCs
add `label` fields to returned JSON objects that previously only had
`account` fields.

Credits
=======

Expand Down
5 changes: 2 additions & 3 deletions src/qt/paymentserver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,6 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
payment.add_transactions(transaction.data(), transaction.size());

// Create a new refund address, or re-use:
QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant);
std::string strAccount = account.toStdString();
CPubKey newKey;
if (wallet->GetKeyFromPool(newKey)) {
// BIP70 requests encode the scriptPubKey directly, so we are not restricted to address
Expand All @@ -646,7 +644,8 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
const OutputType change_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
wallet->LearnRelatedScripts(newKey, change_type);
CTxDestination dest = GetDestinationForKey(newKey, change_type);
wallet->SetAddressBook(dest, strAccount, "refund");
std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();
wallet->SetAddressBook(dest, label, "refund");

CScript s = GetScriptForDestination(dest);
payments::Output* refund_to = payment.add_refund_to();
Expand Down
4 changes: 4 additions & 0 deletions src/rpc/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,16 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "settxfee", 0, "amount" },
{ "getreceivedbyaddress", 1, "minconf" },
{ "getreceivedbyaccount", 1, "minconf" },
{ "getreceivedbylabel", 1, "minconf" },
{ "listreceivedbyaddress", 0, "minconf" },
{ "listreceivedbyaddress", 1, "include_empty" },
{ "listreceivedbyaddress", 2, "include_watchonly" },
{ "listreceivedbyaccount", 0, "minconf" },
{ "listreceivedbyaccount", 1, "include_empty" },
{ "listreceivedbyaccount", 2, "include_watchonly" },
{ "listreceivedbylabel", 0, "minconf" },
{ "listreceivedbylabel", 1, "include_empty" },
{ "listreceivedbylabel", 2, "include_watchonly" },
{ "getbalance", 1, "minconf" },
{ "getbalance", 2, "include_watchonly" },
{ "getblockhash", 0, "height" },
Expand Down
5 changes: 4 additions & 1 deletion src/rpc/protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ enum RPCErrorCode
//! Wallet errors
RPC_WALLET_ERROR = -4, //!< Unspecified problem with wallet (key not found etc.)
RPC_WALLET_INSUFFICIENT_FUNDS = -6, //!< Not enough funds in wallet or account
RPC_WALLET_INVALID_ACCOUNT_NAME = -11, //!< Invalid account name
RPC_WALLET_INVALID_LABEL_NAME = -11, //!< Invalid label name
RPC_WALLET_KEYPOOL_RAN_OUT = -12, //!< Keypool ran out, call keypoolrefill first
RPC_WALLET_UNLOCK_NEEDED = -13, //!< Enter the wallet passphrase with walletpassphrase first
RPC_WALLET_PASSPHRASE_INCORRECT = -14, //!< The wallet passphrase entered was incorrect
Expand All @@ -85,6 +85,9 @@ enum RPCErrorCode
RPC_WALLET_ALREADY_UNLOCKED = -17, //!< Wallet is already unlocked
RPC_WALLET_NOT_FOUND = -18, //!< Invalid wallet specified
RPC_WALLET_NOT_SPECIFIED = -19, //!< No wallet specified (error when there are multiple wallets loaded)

//! Backwards compatible aliases
RPC_WALLET_INVALID_ACCOUNT_NAME = RPC_WALLET_INVALID_LABEL_NAME,
};

UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
Expand Down
202 changes: 104 additions & 98 deletions src/wallet/rpcwallet.cpp

Large diffs are not rendered by default.

22 changes: 11 additions & 11 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -820,12 +820,12 @@ bool CWallet::AccountMove(std::string strFrom, std::string strTo, CAmount nAmoun
return true;
}

bool CWallet::GetAccountDestination(CTxDestination &dest, std::string strAccount, bool bForceNew)
bool CWallet::GetLabelDestination(CTxDestination &dest, const std::string& label, bool bForceNew)
{
CWalletDB walletdb(*dbw);

CAccount account;
walletdb.ReadAccount(strAccount, account);
walletdb.ReadAccount(label, account);

if (!bForceNew) {
if (!account.vchPubKey.IsValid())
Expand All @@ -851,8 +851,8 @@ bool CWallet::GetAccountDestination(CTxDestination &dest, std::string strAccount

LearnRelatedScripts(account.vchPubKey, g_address_type);
dest = GetDestinationForKey(account.vchPubKey, g_address_type);
SetAddressBook(dest, strAccount, "receive");
walletdb.WriteAccount(strAccount, account);
SetAddressBook(dest, label, "receive");
walletdb.WriteAccount(label, account);
} else {
dest = GetDestinationForKey(account.vchPubKey, g_address_type);
}
Expand Down Expand Up @@ -2163,7 +2163,7 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons
for (const CTxOut& out : wtx.tx->vout) {
if (outgoing && IsChange(out)) {
debit -= out.nValue;
} else if (IsMine(out) & filter && depth >= minDepth && (!account || *account == GetAccountName(out.scriptPubKey))) {
} else if (IsMine(out) & filter && depth >= minDepth && (!account || *account == GetLabelName(out.scriptPubKey))) {
balance += out.nValue;
}
}
Expand Down Expand Up @@ -3250,7 +3250,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
return CWalletDB(*dbw).EraseName(EncodeDestination(address));
}

const std::string& CWallet::GetAccountName(const CScript& scriptPubKey) const
const std::string& CWallet::GetLabelName(const CScript& scriptPubKey) const
{
CTxDestination address;
if (ExtractDestination(scriptPubKey, address) && !scriptPubKey.IsUnspendable()) {
Expand All @@ -3260,9 +3260,9 @@ const std::string& CWallet::GetAccountName(const CScript& scriptPubKey) const
}
}
// A scriptPubKey that doesn't have an entry in the address book is
// associated with the default account ("").
const static std::string DEFAULT_ACCOUNT_NAME;
return DEFAULT_ACCOUNT_NAME;
// associated with the default label ("").
const static std::string DEFAULT_LABEL_NAME;
return DEFAULT_LABEL_NAME;
}

/**
Expand Down Expand Up @@ -3618,15 +3618,15 @@ std::set< std::set<CTxDestination> > CWallet::GetAddressGroupings()
return ret;
}

std::set<CTxDestination> CWallet::GetAccountAddresses(const std::string& strAccount) const
std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) const
{
LOCK(cs_wallet);
std::set<CTxDestination> result;
for (const std::pair<CTxDestination, CAddressBookData>& item : mapAddressBook)
{
const CTxDestination& address = item.first;
const std::string& strName = item.second.name;
if (strName == strAccount)
if (strName == label)
result.insert(address);
}
return result;
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
int64_t IncOrderPosNext(CWalletDB *pwalletdb = nullptr);
DBErrors ReorderTransactions();
bool AccountMove(std::string strFrom, std::string strTo, CAmount nAmount, std::string strComment = "");
bool GetAccountDestination(CTxDestination &dest, std::string strAccount, bool bForceNew = false);
bool GetLabelDestination(CTxDestination &dest, const std::string& label, bool bForceNew = false);

void MarkDirty();
bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true);
Expand Down Expand Up @@ -1010,7 +1010,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
std::set< std::set<CTxDestination> > GetAddressGroupings();
std::map<CTxDestination, CAmount> GetAddressBalances();

std::set<CTxDestination> GetAccountAddresses(const std::string& strAccount) const;
std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;

isminetype IsMine(const CTxIn& txin) const;
/**
Expand Down Expand Up @@ -1040,7 +1040,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

bool DelAddressBook(const CTxDestination& address);

const std::string& GetAccountName(const CScript& scriptPubKey) const;
const std::string& GetLabelName(const CScript& scriptPubKey) const;

void Inventory(const uint256 &hash) override
{
Expand Down
2 changes: 1 addition & 1 deletion test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
'feature_segwit.py',
# vv Tests less than 2m vv
'wallet_basic.py',
'wallet_accounts.py',
'wallet_labels.py',
'p2p_segwit.py',
'wallet_dump.py',
'rpc_listtransactions.py',
Expand Down
2 changes: 1 addition & 1 deletion test/functional/wallet_import_rescan.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def check(self, txid=None, amount=None, confirmations=None):

if txid is not None:
tx, = [tx for tx in txs if tx["txid"] == txid]
assert_equal(tx["account"], self.label)
assert_equal(tx["label"], self.label)
assert_equal(tx["address"], self.address["address"])
assert_equal(tx["amount"], amount)
assert_equal(tx["category"], "receive")
Expand Down
File renamed without changes.
54 changes: 27 additions & 27 deletions test/functional/wallet_listreceivedby.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,19 @@ def run_test(self):
self.sync_all()
assert_array_result(self.nodes[1].listreceivedbyaddress(),
{"address": addr},
{"address": addr, "account": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]})
{"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]})
# With min confidence < 10
assert_array_result(self.nodes[1].listreceivedbyaddress(5),
{"address": addr},
{"address": addr, "account": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]})
{"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]})
# With min confidence > 10, should not find Tx
assert_array_result(self.nodes[1].listreceivedbyaddress(11), {"address": addr}, {}, True)

# Empty Tx
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": addr, "label": "", "amount": 0, "confirmations": 0, "txids": []})

self.log.info("getreceivedbyaddress Test")

Expand All @@ -74,46 +74,46 @@ def run_test(self):
# Trying to getreceivedby for an address the wallet doesn't own should return an error
assert_raises_rpc_error(-4, "Address not found in wallet", self.nodes[0].getreceivedbyaddress, addr)

self.log.info("listreceivedbyaccount + getreceivedbyaccount Test")
self.log.info("listreceivedbylabel + getreceivedbylabel Test")

# set pre-state
addrArr = self.nodes[1].getnewaddress()
account = self.nodes[1].getaccount(addrArr)
received_by_account_json = [r for r in self.nodes[1].listreceivedbyaccount() if r["account"] == account][0]
balance_by_account = self.nodes[1].getreceivedbyaccount(account)
label = self.nodes[1].getaccount(addrArr)
received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel() if r["label"] == label][0]
balance_by_label = self.nodes[1].getreceivedbylabel(label)

txid = self.nodes[0].sendtoaddress(addr, 0.1)
self.sync_all()

# listreceivedbyaccount should return received_by_account_json because of 0 confirmations
assert_array_result(self.nodes[1].listreceivedbyaccount(),
{"account": account},
received_by_account_json)
# listreceivedbylabel should return received_by_label_json because of 0 confirmations
assert_array_result(self.nodes[1].listreceivedbylabel(),
{"label": label},
received_by_label_json)

# getreceivedbyaddress should return same balance because of 0 confirmations
balance = self.nodes[1].getreceivedbyaccount(account)
assert_equal(balance, balance_by_account)
balance = self.nodes[1].getreceivedbylabel(label)
assert_equal(balance, balance_by_label)

self.nodes[1].generate(10)
self.sync_all()
# listreceivedbyaccount should return updated account balance
assert_array_result(self.nodes[1].listreceivedbyaccount(),
{"account": account},
{"account": received_by_account_json["account"], "amount": (received_by_account_json["amount"] + Decimal("0.1"))})
# listreceivedbylabel should return updated received list
assert_array_result(self.nodes[1].listreceivedbylabel(),
{"label": label},
{"label": received_by_label_json["label"], "amount": (received_by_label_json["amount"] + Decimal("0.1"))})

# getreceivedbyaddress should return updates balance
balance = self.nodes[1].getreceivedbyaccount(account)
assert_equal(balance, balance_by_account + Decimal("0.1"))
# getreceivedbylabel should return updated receive total
balance = self.nodes[1].getreceivedbylabel(label)
assert_equal(balance, balance_by_label + Decimal("0.1"))

# Create a new account named "mynewaccount" that has a 0 balance
self.nodes[1].getaccountaddress("mynewaccount")
received_by_account_json = [r for r in self.nodes[1].listreceivedbyaccount(0, True) if r["account"] == "mynewaccount"][0]
# Create a new label named "mynewlabel" that has a 0 balance
self.nodes[1].getlabeladdress("mynewlabel")
received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel(0, True) if r["label"] == "mynewlabel"][0]

# Test includeempty of listreceivedbyaccount
assert_equal(received_by_account_json["amount"], Decimal("0.0"))
# Test includeempty of listreceivedbylabel
assert_equal(received_by_label_json["amount"], Decimal("0.0"))

# Test getreceivedbyaccount for 0 amount accounts
balance = self.nodes[1].getreceivedbyaccount("mynewaccount")
# Test getreceivedbylabel for 0 amount labels
balance = self.nodes[1].getreceivedbylabel("mynewlabel")
assert_equal(balance, Decimal("0.0"))

if __name__ == '__main__':
Expand Down

0 comments on commit 44eb4b4

Please sign in to comment.