Merge #14350: Add WalletLocation class
ClosedPublic
Actions

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

nakihito created this revision.Dec 17 2019, 00:31

Owners added a reviewer: Restricted Owners Package.Dec 17 2019, 00:31

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 17 2019, 00:31

nakihito planned changes to this revision.Dec 17 2019, 00:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito edited the summary of this revision. (Show Details)Dec 17 2019, 00:32

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8581: Diff 14889.Dec 17 2019, 00:43

nakihito requested review of this revision.Dec 17 2019, 00:55

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

nakihito added inline comments.Dec 17 2019, 20:32

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.