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

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 !

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.

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

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

Fix pushKVs

src/wallet/rpcwallet.cpp
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.
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
src/wallet/rpcwallet.cpp
3656 ↗(On Diff #6065)

Argument name should be label

This revision was automatically updated to reflect the committed changes.