Page MenuHomePhabricator

Rename account to label where appropriate
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schancel updated this revision to Diff 5603.Oct 31 2018, 23:04

Fix feedback

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.

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
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

schancel updated this revision to Diff 5649.Nov 5 2018, 20:33

Finish removing address_type from getnewaddress.

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

schancel updated this revision to Diff 5652.Nov 5 2018, 21:30

Remove clang-format

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

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. 😢

jasonbcox accepted this revision.Mon, Nov 12, 22:14
This revision is now accepted and ready to land.Sun, Nov 25, 17:16
schancel requested review of this revision.Sun, Nov 25, 17:16
deadalnix accepted this revision.Thu, Nov 29, 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.Thu, Nov 29, 00:08
Fabien added inline comments.Thu, Nov 29, 00:44
src/wallet/rpcwallet.cpp
3656 ↗(On Diff #6065)

Argument name should be label

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

Rebase, fix nits

Closed by commit rABC90348f6ecce9: Rename account to label where appropriate (authored by Russell Yanofsky <russ@yanofsky.org>, committed by schancel). · Explain WhyThu, Nov 29, 21:01
This revision was automatically updated to reflect the committed changes.