Page MenuHomePhabricator

[qt] Multiwallet support for GUI (Core PR12610)
ClosedPublic

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

Details

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

Co-authored-by: Jason B. Cox <contact@jasonbcox.com>

Test Plan

ninja check && ninja check-functional
./src/qt/bitcoin-qt -testnet -wallet=wallet1 -wallet=wallet2
Verify the following:

  1. Visually verify that the wallet dropdown exists in the upper right-hand corner.
  2. Selecting a wallet from the dropdown displays the transactions only associated with that wallet.
  3. Requesting payment on a specified wallet generates an address for that wallet.
  4. Visually verify that Debug window now has a dropdown containing both wallet options.
  5. Selecting a particular wallet from the Debug dropdown and trying RPC commands only provides output relevant to the selected wallet (I used transaction IDs only known to one of the wallets to verify).

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

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)Nov 27 2018, 18:41
jasonbcox marked 2 inline comments as done.Nov 27 2018, 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.Nov 27 2018, 19:03

Rebase + feedback changes

jasonbcox planned changes to this revision.Nov 27 2018, 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)Dec 3 2018, 20:41
jasonbcox requested review of this revision.Dec 3 2018, 20:45
Fabien requested changes to this revision.Dec 4 2018, 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.Dec 4 2018, 22:04
jasonbcox marked an inline comment as done.Dec 5 2018, 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.Dec 5 2018, 16:43
deadalnix requested changes to this revision.Dec 6 2018, 01:47
deadalnix added inline comments.
src/qt/bitcoingui.cpp
77 ↗(On Diff #6284)

There is

appToolBar(0),

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

This revision now requires changes to proceed.Dec 6 2018, 01:47
jasonbcox requested review of this revision.Dec 28 2018, 22:10
jasonbcox added inline comments.
src/qt/bitcoingui.cpp
77 ↗(On Diff #6284)

I intentionally didn't include this line since we backported D1959 out of order. Instead of appToolBar(0), we now have QToolBar *appToolBar = nullptr; instead of QToolBar *appToolBar; in src/qt/bitcoingui.h

deadalnix requested changes to this revision.Dec 30 2018, 18:57

The test plan do not cover the usage of multiple wallet using the GUI, at best that the GUI is able to boot.

src/qt/rpcconsole.cpp
15 ↗(On Diff #6284)

These were qt headers files and this is why they were separated. This code split qt include from others, the PR you imported from use qt/ explicitely, both of which are objectively best than what you did.

This revision now requires changes to proceed.Dec 30 2018, 18:57
jasonbcox edited the test plan for this revision. (Show Details)Jan 5 2019, 17:38
jasonbcox updated this revision to Diff 6515.Jan 5 2019, 17:50
jasonbcox marked an inline comment as done.

Fix imports + rebase

jasonbcox planned changes to this revision.Jan 5 2019, 17:50
jasonbcox edited the test plan for this revision. (Show Details)Jan 5 2019, 23:35
jasonbcox requested review of this revision.Jan 5 2019, 23:39
jasonbcox edited the test plan for this revision. (Show Details)
deadalnix accepted this revision.Jan 7 2019, 01:06
This revision is now accepted and ready to land.Jan 7 2019, 01:06
jasonbcox edited the summary of this revision. (Show Details)Jan 7 2019, 05:26
This revision was automatically updated to reflect the committed changes.