Rename account to label where appropriate by ryanofsky · Pull Request #11536 · bitcoin/bitcoin

ryanofsky · 2017-10-20T18:09:26Z

Rename account to label where appropriate

This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in #7729, by getting renaming out of the way and letting that change 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.

There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).

meshcollider · 2017-10-20T18:33:04Z

Concept ACK, I think this should be part of #11497?

ryanofsky · 2017-10-20T18:54:34Z

Concept ACK, I think this should be part of #11497?

I don't mind combining prs though I don't see what advantage it brings in this case. This PR is backwards compatible and can be merged without waiting for #7729. #11497 is also confused about which account instances need to be removed vs renamed (as you pointed out in one of your review comments), and this PR clarifies that.

promag · 2017-10-20T23:55:59Z

src/qt/paymentserver.cpp

-    QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant);
-    std::string strAccount = account.toStdString();
-    std::set<CTxDestination> refundAddresses = wallet->GetAccountAddresses(strAccount);
+    QString label = tr("Refund from %1").arg(recipient.authenticatedMerchant);


Only std::string is used, so:

std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();

Changed in be9665a

promag · 2017-10-21T00:00:51Z

src/wallet/rpcwallet.cpp

 }

+// Deprecated account RPCs.
+UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); }


Missing IsDeprecatedRPCEnabled("accounts") and throw? Same below.

This doesn't actually use -deprecatedrpc though, that's done in #11497?

I made the question because in the release notes says these are deprecated.

Added "to be" in 52177d1 since after #11031, "deprecated" might now mean disabled instead of just slated for removal.

laanwj · 2017-10-21T06:57:08Z

src/wallet/wallet.h

 /** 
- * Account information.
- * Stored in wallet with key "acc"+string account name.
+ * Label information.


This structure is account-specific. It's going away when account support is dropped, as there is no state per label, it's just an identifier. So I suggest keeping it at the current name.

Reverted in 603759b

laanwj · 2017-10-21T06:59:03Z

src/rpc/protocol.h

    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


I think usually when renaming errors we keep the old name around as an alias - people are processing this directly to RPC wrappers, and old client code would still use the old name.

Added alias in acc1ee8

Just for curiosity, why isn't RPC_INVALID_PARAMETER used instead?

#11536 (comment)

Just for curiosity, why isn't RPC_INVALID_PARAMETER used instead?

I don't know the history, but I could imagine this was added because it was useful for some client to be able to distinguish this specific error from more generic errors. Similar to how RPC_WALLET_NOT_SPECIFIED was added for bitcoin-cli in #10931.

laanwj · 2017-10-21T07:00:10Z

src/rpc/client.cpp

    { "settxfee", 0, "amount" },
    { "getreceivedbyaddress", 1, "minconf" },
    { "getreceivedbyaccount", 1, "minconf" },
+    { "getreceivedbylabel", 1, "minconf" },


You don't actually introduce these methods in this PR.

Edit: oh you do, github was hiding things again.

laanwj · 2017-10-21T07:02:27Z

Concept ACK - needs qa test for the new RPCs.

laanwj · 2017-10-21T07:04:31Z

src/wallet/rpcwallet.cpp

+UniValue getaddressesbyaccount(const JSONRPCRequest& request) { return getaddressesbylabel(request); }
+UniValue getreceivedbyaccount(const JSONRPCRequest& request) { return getreceivedbylabel(request); }
+UniValue listreceivedbyaccount(const JSONRPCRequest& request) { return listreceivedbylabel(request); }
+UniValue setaccount(const JSONRPCRequest& request) { return setlabel(request); }


This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?

This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?

The intention of this PR is to not introduce any differences between accounts and labels yet, leaving that for #7729 and any future followups. I have a rebased version of #7729 at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).

ryanofsky

Added 7 commits b105fb9 -> 68b48f6 (pr/acct.1 -> pr/acct.2, compare)
Squashed 68b48f6 -> fec503a (pr/acct.2 -> pr/acct.3)

needs qa test for the new RPCs.

Added wallet-labels test in fec503a

ryanofsky · 2017-10-21T12:02:24Z

src/qt/paymentserver.cpp

-    QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant);
-    std::string strAccount = account.toStdString();
-    std::set<CTxDestination> refundAddresses = wallet->GetAccountAddresses(strAccount);
+    QString label = tr("Refund from %1").arg(recipient.authenticatedMerchant);


Changed in be9665a

ryanofsky · 2017-10-21T12:02:40Z

src/rpc/protocol.h

    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


Added alias in acc1ee8

ryanofsky · 2017-10-21T12:03:07Z

src/wallet/rpcwallet.cpp

 }

+// Deprecated account RPCs.
+UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); }


Added "to be" in 52177d1 since after #11031, "deprecated" might now mean disabled instead of just slated for removal.

ryanofsky · 2017-10-21T12:03:25Z

src/wallet/wallet.h

 /** 
- * Account information.
- * Stored in wallet with key "acc"+string account name.
+ * Label information.


Reverted in 603759b

ryanofsky · 2017-10-21T13:28:07Z

src/wallet/rpcwallet.cpp

+UniValue getaddressesbyaccount(const JSONRPCRequest& request) { return getaddressesbylabel(request); }
+UniValue getreceivedbyaccount(const JSONRPCRequest& request) { return getreceivedbylabel(request); }
+UniValue listreceivedbyaccount(const JSONRPCRequest& request) { return listreceivedbylabel(request); }
+UniValue setaccount(const JSONRPCRequest& request) { return setlabel(request); }


This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?

The intention of this PR is to not introduce any differences between accounts and labels yet, leaving that for #7729 and any future followups. I have a rebased version of #7729 at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).

jonasschnelli · 2017-10-23T01:20:35Z

Thanks!
Concept ACK.

maflcko

Concept ACK fec503a

maflcko · 2017-10-23T09:06:44Z

src/wallet/rpcwallet.cpp

            "[\n"
            "  {\n"
            "    \"involvesWatchonly\" : true,   (bool) Only returned if imported addresses were involved in transaction\n"
            "    \"account\" : \"accountname\",  (string) The account name of the receiving account\n"


Should be updated, since this is an alias for "label". (Same for other occurrences in this file)

Good catch, updated in 864a837.

ryanofsky

Added 2 commits fec503a -> 864a837 (pr/acct.3 -> pr/acct.4, compare)
Squashed 864a837 -> 62df168 (pr/acct.4 -> pr/acct.5)

ryanofsky · 2017-10-23T14:53:29Z

src/wallet/rpcwallet.cpp

            "[\n"
            "  {\n"
            "    \"involvesWatchonly\" : true,   (bool) Only returned if imported addresses were involved in transaction\n"
            "    \"account\" : \"accountname\",  (string) The account name of the receiving account\n"


Good catch, updated in 864a837.

Sjors · 2017-11-09T15:27:41Z

doc/release-notes.md

-
 - `dumpwallet` no longer allows overwriting files. This is a security measure
  as well as prevents dangerous user mistakes.
+- `getnewaddress` and `addmultisigaddress` RPC `account` named parameters have


Do you want to keep an address alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.

#11536 (comment)

Do you want to keep an address alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.

Thanks. Somehow I missed this comment earlier. Added in 7ae567f

maflcko · 2017-11-10T16:59:18Z

Needs rebase

ryanofsky · 2017-12-01T20:28:15Z

Needs rebase

Just so this isn't the last comment., I have been periodically rebasing this PR. The PR is just a big rename so it gets out of date pretty frequently.

ryanofsky

Rebased 62df168 -> a8e9ac9 (pr/acct.5 -> pr/acct.6) due to conflicts.
Rebased a8e9ac9 -> 981ac92 (pr/acct.6 -> pr/acct.7) due to more conflicts.
Added 1 commit 981ac92 -> 7ae567f (pr/acct.7 -> pr/acct.8, compare) implementing Sjors suggestion.
Squashed 7ae567f -> 8ec5890 (pr/acct.8 -> pr/acct.9)

ryanofsky · 2018-01-02T16:54:14Z

src/rpc/protocol.h

    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


#11536 (comment)

Just for curiosity, why isn't RPC_INVALID_PARAMETER used instead?

I don't know the history, but I could imagine this was added because it was useful for some client to be able to distinguish this specific error from more generic errors. Similar to how RPC_WALLET_NOT_SPECIFIED was added for bitcoin-cli in #10931.

ryanofsky · 2018-01-04T18:54:52Z

Rebased again 8ec5890 -> 5bb0a7b (pr/acct.9 -> pr/acct.10) due to conflict with #12062.

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.

This is a separate commit because changing the test at the same time as renaming it breaks git (and github) rename detection.

@Sjors

4c317d8 Document RPC method aliasing (Russell Yanofsky) Pull request description: Suggested by @Sjors in #11536 (comment) Tree-SHA512: 7bf16238e41b6c6c078e9103d8eac2ac76739a2c16b4f964be49bfde1f20f31a1fb30badf1faaa6ddc301a74f0d785d19567069b50de78c502144479143cb38c

jnewbery

One comment inline.

Why does this PR not add the getlabel and getaddressesbylabel rpcs?

I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py -> wallet_labels.py

jnewbery · 2018-03-19T15:34:06Z

doc/release-notes.md

 - The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
+- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
+  parameters have been renamed to `label` with no change in behavior.
+- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and


Nit: language is perhaps a little clumsy: ...been added to replace to be deprecated.... Perhaps prefer something like:

Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and `setlabel` RPCs have been added to replace `getaccountaddress`, `getreceivedbyaccount`, `listreceivedbyaccount`, and `setaccount` RPCs, which are now deprecated. There is no change in behavior between the new RPCs and deprecated RPCs.

Nit: language is perhaps a little clumsy

Updated in 843ee06

ryanofsky

Added 1 commits e2966ce -> 843ee06 (pr/acct.26 -> pr/acct.27, compare)
Squashed 843ee06 -> d2527bd (pr/acct.27 -> pr/acct.28)

Why does this PR not add the getlabel and getaddressesbylabel rpcs?

#7729 adds these with different behavior than the existing account rpcs. The idea is for this PR to just clean up stale "account" terminology and for #7729 to add new functionality for making labels work better.

I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py -> wallet_labels.py

Sure, moved rename to second commit.

ryanofsky · 2018-03-19T18:07:06Z

doc/release-notes.md

 - The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
+- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
+  parameters have been renamed to `label` with no change in behavior.
+- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and


Nit: language is perhaps a little clumsy

Updated in 843ee06

jnewbery · 2018-03-19T19:06:51Z

test/functional/wallet_labels.py

+    - getaddressesbyaccount
+    - listaddressgroupings
+    - setlabel
+    - sendfrom (with account arguments)


This (and the line below) can be updated to (with label arguments)

This (and the line below) can be updated to (with label arguments)

This is actually intentional. sendfrom and move are account-specific and deprecated and won't take label arguments. Idea is that we support receiving at addresses but not really sending from addresses.

ok, makes sense. I was confused by the code lower down in the test case:

# Check that sendfrom label reduces listaccounts balances. for i, label in enumerate(labels): to_label = labels[(i+1) % len(labels)] node.sendfrom(label.name, to_label.receive_address, amount_to_send)

jnewbery · 2018-03-19T19:13:10Z

Tested ACK d2527bd. Thanks for moving the commits around.

Sorry - I've added one more nit in the test file.

d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in #7729, by getting renaming out of the way and letting that change 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. --- There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff

Suggested by Sjors Provoost <[email protected]> in bitcoin#11536 (comment)

Summary: Suggested by Sjors Provoost <[email protected]> in bitcoin/bitcoin#11536 (comment) This is a backport of Core PR12700 Test Plan: make Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4169

@Sjors

4c317d8 Document RPC method aliasing (Russell Yanofsky) Pull request description: Suggested by @Sjors in bitcoin#11536 (comment) Tree-SHA512: 7bf16238e41b6c6c078e9103d8eac2ac76739a2c16b4f964be49bfde1f20f31a1fb30badf1faaa6ddc301a74f0d785d19567069b50de78c502144479143cb38c

d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate 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 that change 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. --- There is a rebased version of bitcoin#7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see bitcoin#7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff

promag reviewed Oct 21, 2017

View reviewed changes

laanwj reviewed Oct 21, 2017

View reviewed changes

ryanofsky force-pushed the pr/acct branch from b105fb9 to fec503a Compare October 21, 2017 16:43

ryanofsky commented Oct 21, 2017

View reviewed changes

ryanofsky mentioned this pull request Oct 21, 2017

rpc: introduce 'label' API for wallet #7729

Closed

maflcko reviewed Oct 23, 2017

View reviewed changes

ryanofsky force-pushed the pr/acct branch from fec503a to 62df168 Compare October 23, 2017 14:55

ryanofsky commented Oct 23, 2017

View reviewed changes

Sjors reviewed Nov 9, 2017

View reviewed changes

ryanofsky force-pushed the pr/acct branch 6 times, most recently from e958b9e to 981ac92 Compare December 1, 2017 04:07

ryanofsky mentioned this pull request Dec 6, 2017

SegWit wallet support #11403

Merged

laanwj added the Wallet label Dec 12, 2017

ryanofsky commented Jan 2, 2018

View reviewed changes

ryanofsky force-pushed the pr/acct branch 2 times, most recently from 8ec5890 to 5bb0a7b Compare January 4, 2018 16:54

ryanofsky added 2 commits March 19, 2018 12:05

Rename wallet_accounts.py test

d2527bd

This is a separate commit because changing the test at the same time as renaming it breaks git (and github) rename detection.

ryanofsky force-pushed the pr/acct branch from d596f03 to e2966ce Compare March 19, 2018 16:18

jnewbery reviewed Mar 19, 2018

View reviewed changes

ryanofsky force-pushed the pr/acct branch from e2966ce to d2527bd Compare March 19, 2018 18:38

ryanofsky commented Mar 19, 2018

View reviewed changes

jnewbery reviewed Mar 19, 2018

View reviewed changes

laanwj merged commit d2527bd into bitcoin:master Mar 22, 2018

ryanofsky mentioned this pull request Mar 22, 2018

scripted-diff: Rename wallet database classes #11851

Merged

2a5A1Ghu1 mentioned this pull request Apr 5, 2018

doc: add qrencode to brew install instructions (#1) #12889

Closed

jnewbery mentioned this pull request Apr 11, 2018

Remove wallet 'account' API #12952

Closed

6 tasks

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Jul 31, 2018

Document RPC method aliasing

bea19ae

Suggested by Sjors Provoost <[email protected]> in bitcoin#11536 (comment)

HashUnlimited mentioned this pull request Jul 31, 2018

Document RPC method aliasing chaincoin/chaincoin#69

Merged

Fuzzbawls mentioned this pull request Jan 8, 2020

Timeline: Removal of outdated internal accounting system PIVX-Project/PIVX#1248

Closed

Fuzzbawls mentioned this pull request Jun 25, 2020

[Tracking] Remove wallet 'account' API PIVX-Project/PIVX#1706

Closed

11 tasks

kwvg mentioned this pull request Mar 9, 2021

merge #17165: Remove BIP70 support dashpay/dash#4023

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ryanofsky commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 23, 2017

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 20, 2017 •

edited

Loading

meshcollider commented Oct 20, 2017 •

edited

Loading

ryanofsky commented Oct 20, 2017 •

edited

Loading

laanwj Oct 21, 2017 •

edited

Loading

ryanofsky Oct 21, 2017 •

edited

Loading

ryanofsky Oct 21, 2017 •

edited

Loading

maflcko left a comment •

edited

Loading

ryanofsky left a comment •

edited

Loading