Page MenuHomePhabricator

Rename account to label where appropriate

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



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

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 3770
Build 5614: Bitcoin ABC Buildbot (legacy)
Build 5613: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix added inline comments.
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...

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 !

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.

deadalnix requested changes to this revision.Nov 4 2018, 18:58
deadalnix added inline comments.
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:

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.

141 ↗(On Diff #5634)


154 ↗(On Diff #5634)

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

593 ↗(On Diff #5634)


890 ↗(On Diff #5634)


893 ↗(On Diff #5634)

This got changed to be passed by ref.

1 ↗(On Diff #5634)

This file got renamed.

This revision now requires changes to proceed.Nov 4 2018, 18:58
schancel added inline comments.
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.

1 ↗(On Diff #5634)

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

Finish removing address_type from getnewaddress.

Pull in previous fixes per Fabian.

deadalnix requested changes to this revision.Nov 5 2018, 23:19
deadalnix added inline comments.
708 ↗(On Diff #5634)

Thanks, good to know.

45 ↗(On Diff #5652)

Good catch :)

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 marked an inline comment as done.

Fix pushKVs

1505 ↗(On Diff #5652)

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

This revision is now accepted and ready to land.Nov 25 2018, 17:16
deadalnix added inline comments.
119 ↗(On Diff #6065)

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

1654 ↗(On Diff #6065)


This revision is now accepted and ready to land.Nov 29 2018, 00:08
3656 ↗(On Diff #6065)

Argument name should be label

This revision was automatically updated to reflect the committed changes.