Page MenuHomePhabricator
Differential D1986

Rename account to label where appropriate
ClosedPublic
Actions

Authored by schancel on Oct 29 2018, 23:59.

Details

Reviewers
jasonbcox
Fabien
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGING90348f6ecce9: Rename account to label where appropriate
rABC90348f6ecce9: Rename account to label where appropriate
Summary

This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
https://github.com/bitcoin/bitcoin/pull/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.

Backport Core PR11536
Depends on D1980 and D2001

Test Plan
make check && ./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr11536
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3768
Build 5610: Bitcoin ABC Buildbot (legacy)
Build 5609: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schancel added a child revision: D1992: Rename wallet functional test files to match bitcoin core.Oct 31 2018, 23:16
deadalnix added a subscriber: deadalnix.Nov 1 2018, 01:32
deadalnix added inline comments.
doc/release-notes.md
28 ↗(On Diff #5603)

I don't think it makes a lot of sense to create a category backport as it is completely not actionable by anyone reading this.

If the list gets big, then maybe we want to split into RPC/dependencies/whatever...

Fabien added inline comments.Nov 1 2018, 14:46
src/wallet/rpcwallet.cpp
1411 ↗(On Diff #5596)

Was looking the wrong call. No issue there.

3669 ↗(On Diff #5596)

You're right, I missed it because the getaccountaddress and getlabeladdress are in reversed order in core.

I did not catch the duplicated getaccount !

jasonbcox added inline comments.Nov 1 2018, 21:05
doc/release-notes.md
28 ↗(On Diff #5603)

For non-RPC changes, this is true, but for RPC commands backported from Core, it's directly relevant, as businesses with patches or special infra to handle ABC not supporting expected RPC commands in Core can now migrate since they know the behavior is now the same/similar.

Fabien mentioned this in D1994: Update release notes to include several missing items.Nov 2 2018, 16:48
schancel updated this revision to Diff 5634.Nov 2 2018, 22:25

Rebase on D2001

schancel edited the summary of this revision. (Show Details)Nov 2 2018, 22:28
schancel added a parent revision: D2001: [qa] Improve wallet-accounts test.
teamcity added a comment.Nov 2 2018, 22:29

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 2 2018, 22:36

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3755: Diff 5634.Nov 2 2018, 22:37
deadalnix requested changes to this revision.Nov 4 2018, 18:58
deadalnix added inline comments.
doc/release-notes.md
28 ↗(On Diff #5634)

This would benefit from being just one list. If we have enough items that we need categories, we'll make category. Catogories should be done based on what users will want to dig (change in the RPC API, change in build dependencies, new wallet features, new mining feature, etc...) rather than the source.

See for instance: https://releases.llvm.org/7.0.0/docs/ReleaseNotes.html

src/qt/paymentserver.cpp
708 ↗(On Diff #5634)

This clearly doesn't match the original PR, why is that ?

717 ↗(On Diff #5634)
std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();

Is missing.

src/wallet/rpcwallet.cpp
141 ↗(On Diff #5634)

Braces

154 ↗(On Diff #5634)

The address type clearly wasn't there before. Something is wrong here.

593 ↗(On Diff #5634)

???

src/wallet/wallet.cpp
890 ↗(On Diff #5634)

dito

src/wallet/wallet.h
893 ↗(On Diff #5634)

This got changed to be passed by ref.

test/functional/wallet-accounts.py
1 ↗(On Diff #5634)

This file got renamed.

This revision now requires changes to proceed.Nov 4 2018, 18:58
schancel marked 3 inline comments as done.Nov 5 2018, 19:06
schancel added inline comments.
src/qt/paymentserver.cpp
708 ↗(On Diff #5634)

The code was changed in Peter Wuille's segwit wallet support patches. This is a synthesized diff to make the same effective change without the SegWit changes.

717 ↗(On Diff #5634)

No, it's above where it's needed.

test/functional/wallet-accounts.py
1 ↗(On Diff #5634)

There's another diff to update these. It needs to be done in a second diff.

schancel updated this revision to Diff 5645.Nov 5 2018, 19:13

Update per feedback

teamcity added a comment.Nov 5 2018, 19:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 5 2018, 19:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3765: Diff 5645.Nov 5 2018, 19:23
schancel updated this revision to Diff 5649.Nov 5 2018, 20:33

Finish removing address_type from getnewaddress.

teamcity added a comment.Nov 5 2018, 20:35

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 5 2018, 20:44

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3768: Diff 5649.Nov 5 2018, 20:44
schancel updated this revision to Diff 5650.Nov 5 2018, 21:12

Pull in previous fixes per Fabian.

schancel updated this revision to Diff 5651.Nov 5 2018, 21:13

Rebase

teamcity added a comment.Nov 5 2018, 21:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 5 2018, 21:15

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B3769: Diff 5650.Nov 5 2018, 21:22
teamcity added a comment.Nov 5 2018, 21:22

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

teamcity added a comment.Nov 5 2018, 21:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3770: Diff 5651.Nov 5 2018, 21:24
schancel updated this revision to Diff 5652.Nov 5 2018, 21:30

Remove clang-format

teamcity added a comment.Nov 5 2018, 21:32

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 5 2018, 21:40

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3771: Diff 5652.Nov 5 2018, 21:40
Fabien accepted this revision as: Fabien.Nov 5 2018, 21:53
deadalnix requested changes to this revision.Nov 5 2018, 23:19
deadalnix added inline comments.
src/qt/paymentserver.cpp
708 ↗(On Diff #5634)

Thanks, good to know.

src/rpc/client.cpp
45 ↗(On Diff #5652)

Good catch :)

src/wallet/rpcwallet.cpp
588 ↗(On Diff #5652)

You should remove that empty line.

1505 ↗(On Diff #5652)

I think you fubared things up here. Shouldn't pushKV be used ?

3271 ↗(On Diff #5652)

Shouldn't this be PushKV ?

This revision now requires changes to proceed.Nov 5 2018, 23:19
schancel updated this revision to Diff 5660.Nov 6 2018, 02:12
schancel marked an inline comment as done.

Fix pushKVs

teamcity added a comment.Nov 6 2018, 02:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

schancel added inline comments.Nov 6 2018, 02:15
src/wallet/rpcwallet.cpp
1505 ↗(On Diff #5652)

Yes. I fixed them and then unfixed it when rebasing twice. 😢

teamcity added a comment.Nov 6 2018, 02:17

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3777: Diff 5660!Nov 6 2018, 02:17
jasonbcox accepted this revision.Nov 12 2018, 22:14
schancel updated this revision to Diff 6065.Nov 23 2018, 19:39

Rebase

teamcity added a comment.Nov 23 2018, 19:40

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B4092: Diff 6065.Nov 23 2018, 19:49
teamcity added a comment.Nov 23 2018, 19:49

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

schancel removed a reviewer: deadalnix.Nov 25 2018, 17:16
This revision is now accepted and ready to land.Nov 25 2018, 17:16
schancel requested review of this revision.Nov 25 2018, 17:16
deadalnix accepted this revision.Nov 29 2018, 00:08
deadalnix added inline comments.
src/rpc/protocol.h
119 ↗(On Diff #6065)

Trailing coma would avoid having to modify unrelated line when changing the enum.

src/wallet/rpcwallet.cpp
1654 ↗(On Diff #6065)

Keep

This revision is now accepted and ready to land.Nov 29 2018, 00:08
Fabien added inline comments.Nov 29 2018, 00:44
src/wallet/rpcwallet.cpp
3656 ↗(On Diff #6065)

Argument name should be label

schancel updated this revision to Diff 6175.Nov 29 2018, 20:13

Rebase, fix nits

teamcity added a comment.Nov 29 2018, 20:15

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B4175: Diff 6175.Nov 29 2018, 20:24
teamcity added a comment.Nov 29 2018, 20:24

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Closed by commit rABC90348f6ecce9: Rename account to label where appropriate (authored by Russell Yanofsky <russ@yanofsky.org>, committed by schancel). · Explain WhyNov 29 2018, 21:01
This revision was automatically updated to reflect the committed changes.
nakihito mentioned this in D3308: Merge #9894: remove 'label' filter for rpc command help.Jun 14 2019, 21:05
deadalnix mentioned this in D3624: Rename wallet_accounts.py test.Jul 10 2019, 00:21
deadalnix mentioned this in rABC6509f482538a: Rename wallet_accounts.py test.Jul 10 2019, 13:33
nakihito mentioned this in D3997: Merge #11050: Avoid treating null RPC arguments different from missing arguments.Sep 5 2019, 19:20
deadalnix mentioned this in rSTAGING6509f482538a: Rename wallet_accounts.py test.Sep 19 2019, 01:09
Fabien mentioned this in D5899: Pass some of wallet/rpcwallet RPC results and examples to RPCHelpMan.Apr 30 2020, 07:37

Revision Contents
Changeset List

PathSize
doc/
8 lines
src/
qt/
9 lines
rpc/
6 lines
6 lines
wallet/
308 lines
9 lines
24 lines
test/
functional/
2 lines
71 lines
174 lines

Diff 5649

View Options

doc/release-notes.md

Loading...
View Options

src/qt/paymentserver.cpp

Loading...
View Options

src/rpc/client.cpp

Loading...
View Options

src/rpc/protocol.h

Loading...
View Options

src/wallet/rpcwallet.cpp

Loading...
View Options

src/wallet/wallet.h

Loading...
View Options

src/wallet/wallet.cpp

Loading...
View Options

test/functional/import-rescan.py

Loading...
View Options

test/functional/receivedby.py

Loading...
View Options

test/functional/wallet-accounts.py

Loading...