Page MenuHomePhabricator

Merge #14350: Add WalletLocation class
ClosedPublic

Authored by nakihito on Dec 17 2019, 00:31.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGINGc6620b59cba1: Merge #14350: Add WalletLocation class
rABCc6620b59cba1: Merge #14350: Add WalletLocation class
Summary

65f3672f3b82a6fa30e5171f85bc8d8a29e0797e wallet: Refactor to use WalletLocation (João Barbosa)
01a4c095c87500650663341533f000c6b613e9da wallet: Add WalletLocation utility class (João Barbosa)

Pull request description:

Advantages of this change:
 - avoid resolving wallet absolute path and name repetitively and in multiple places;
 - avoid calling `GetWalletDir` in multiple places;
 - extract these details from the actual wallet implementation.

The `WalletLocation` class can be a way to represent a wallet not yet loaded that exists in the wallet directory.

Tree-SHA512: 71ec09786e038499710e7acafe92d66ab9883fc894964e267443ae9c10a6872a10995c3987a169c436a4e793dae96b28fb97bd7f78483c4b72ac930fa23f8686

Backport of Core PR14350
https://github.com/bitcoin/bitcoin/pull/14350/

Completes T596

Test Plan
make check
test_runner.py wallet_*

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

Owners added a reviewer: Restricted Owners Package.Dec 17 2019, 00:31
Fabien requested changes to this revision.Dec 17 2019, 11:02
Fabien added inline comments.
src/wallet/test/wallet_test_fixture.cpp
11 ↗(On Diff #14889)

Missing #include <wallet/rpcwallet.h>, any reason for that ?

src/wallet/test/walletdb_tests.cpp
20 ↗(On Diff #14889)

Nit: This is an opportunity to replace by std::make_unique

src/wallet/wallet.h
23 ↗(On Diff #14889)

Why not removing it like the original PR ?

887 ↗(On Diff #14889)

Nit: making the initializers ordering consistent with the arguments ordering makes the code easier to read.

This revision now requires changes to proceed.Dec 17 2019, 11:02
src/wallet/test/wallet_test_fixture.cpp
11 ↗(On Diff #14889)

It didn't seem necessary at first, but I realized that it was because I couldn't remove this include from wallet.h. I'll add it here for the sake of merge conflicts, though.

src/wallet/wallet.h
23 ↗(On Diff #14889)

Its required for the PBST changes that we recently added.

887 ↗(On Diff #14889)

Making these change causes numerous -Wreorder warnings.

This revision is now accepted and ready to land.Dec 17 2019, 20:57