Page MenuHomePhabricator

[qt] Multiwallet support for GUI (Core PR12610)
Needs RevisionPublic

Authored by jasonbcox on Oct 29 2018, 22:24.

Details

Reviewers
deadalnix
schancel
Fabien
Group Reviewers
Restricted Project
Summary

Completes T440

Original commit messages below:

PR12610:

Merge #12610: Multiwallet for the GUI

779c5f984 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli)
dc6f150f3 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli)
4826ca4b8 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli)
cfa4133ce GUI: RPCConsole: Log wallet changes (Luke Dashjr)
b6d04fc7c Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr)
12d8d2681 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli)
d1ec34a76 Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr)
d49cc70e6 Qt: Add wallet selector to debug console (Jonas Schnelli)
d558f44c5 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr)
85d531971 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr)
e449f9a9e Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr)
3dba3c3ac Qt: Load all wallets into WalletModels (Luke Dashjr)

Pull request description:

This is an overhaul of #11383 (plus some additions).
It avoids unnecessary coupling of httpserver/jsonrpc and the wallet as well as it avoids pointer pure passing (and pointer deletion) of `CWallet` (plus other minor design changes).

Additionally it adds the wallet name to the sendconfirmation and request dialog (in case multiwallet is active)

Tree-SHA512: 3d06e18badbc5d1821e488bf1dae463bb0be544cf11b2b618e025812bfdd13c5f39604bb93b4c705313930e7dc4e66f4848b9469ba14871bade58e7a027246a1

Includes PR12795, since PR12610 introduced a bug:

Do not truncate .dat extension for wallets in gui

See https://github.com/bitcoin/bitcoin/pull/12610#issuecomment-376278677 for details

Test Plan

ninja check && ninja check-functional
./src/qt/bitcoin-qt -testnet -wallet=wallet1 -wallet=wallet2

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr12610
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4271
Build 6607: Bitcoin ABC Teamcity Staging
Build 6606: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Oct 29 2018, 22:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 29 2018, 22:24
schancel retitled this revision from Backports of Core PRs: PR12610 PR12795 to [qt] Multiwallet support for GUI (Core PR12610).Oct 30 2018, 16:43
schancel edited the summary of this revision. (Show Details)

I can't review this. QT is currently broken on MacOS X mojave. I need to look into it more.

deadalnix requested changes to this revision.Nov 5 2018, 23:53

Some small changes, mostly it's good.

src/qt/bitcoin.cpp
457 ↗(On Diff #5571)

Not blaming you, as this come from the patch, but someone should really learn about unique_ptr

src/qt/bitcoingui.cpp
1029 ↗(On Diff #5571)

dito

src/qt/bitcoingui.h
213 ↗(On Diff #5571)

Keep passing amount by value. Passing them by ref don't make sense.

src/qt/walletmodel.cpp
724 ↗(On Diff #5571)

Is the removin of the ".dat" handling code on purpose ?

src/qt/walletview.h
136 ↗(On Diff #5571)

Keep passing amount by value

This revision now requires changes to proceed.Nov 5 2018, 23:53
jasonbcox edited the summary of this revision. (Show Details)Tue, Nov 27, 18:41
jasonbcox marked 2 inline comments as done.Tue, Nov 27, 18:46
jasonbcox added inline comments.
src/qt/bitcoin.cpp
457 ↗(On Diff #5571)

Ya agreed. We can fix this in a future diff once these backports are completed.

src/qt/walletmodel.cpp
724 ↗(On Diff #5571)

Yes. There was a small note in the diff summary, but I realize it was not clear as to why that change was included as part of this backport. Updated it now.

jasonbcox updated this revision to Diff 6139.Tue, Nov 27, 19:03

Rebase + feedback changes

jasonbcox planned changes to this revision.Tue, Nov 27, 19:04

Need to test bitcoin-qt still. Having some issues running Qt on WSL.

jasonbcox edited the test plan for this revision. (Show Details)Mon, Dec 3, 20:41
jasonbcox requested review of this revision.Mon, Dec 3, 20:45
Fabien requested changes to this revision.Tue, Dec 4, 22:04
Fabien added a subscriber: Fabien.

Otherwise LGTM

src/qt/bitcoingui.cpp
582 ↗(On Diff #6139)

braces

This revision now requires changes to proceed.Tue, Dec 4, 22:04
jasonbcox marked an inline comment as done.Wed, Dec 5, 00:36
jasonbcox added inline comments.
src/qt/bitcoingui.cpp
582 ↗(On Diff #6139)

there's lots of these, which we should fix after this deluge of backports. otherwise it just adds to the merge conflict mayhem.

Fabien accepted this revision as: Fabien.Wed, Dec 5, 16:43
deadalnix requested changes to this revision.Thu, Dec 6, 01:47
deadalnix added inline comments.
src/qt/bitcoingui.cpp
77

There is

appToolBar(0),

missing. Which begs the question: there is some previous backport that is missing.

This revision now requires changes to proceed.Thu, Dec 6, 01:47