Details

Reviewers

deadalnix
schancel
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGINGf957f5b34ce8: [qt] Multiwallet support for GUI (Core PR12610)
rABCf957f5b34ce8: [qt] Multiwallet support for GUI (Core PR12610)

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:

Visually verify that the wallet dropdown exists in the upper right-hand corner.
Selecting a wallet from the dropdown displays the transactions only associated with that wallet.
Requesting payment on a specified wallet generates an address for that wallet.
Visually verify that Debug window now has a dropdown containing both wallet options.
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

Branch

pr12610

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4271
Build 6607: Bitcoin ABC Buildbot (legacy)
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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3711: Diff 5571.Oct 29 2018, 22:34

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.

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.

Rebase + feedback changes

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4149: Diff 6139.Nov 27 2018, 19:12

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

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.

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4271: Diff 6284.Dec 5 2018, 00:46

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

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

Fix imports + rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox planned changes to this revision.Jan 5 2019, 17:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4418: Diff 6515.Jan 5 2019, 17:58

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

Closed by commit rABCf957f5b34ce8: [qt] Multiwallet support for GUI (Core PR12610) (authored by Jonas Schnelli <dev@jonasschnelli.ch>, committed by jasonbcox). · Explain WhyJan 7 2019, 05:27

This revision was automatically updated to reflect the committed changes.

nakihito mentioned this in D4254: ui: Support wallets unloaded dynamically.Oct 15 2019, 02:55

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

Details

Original commit messages below:

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 6284

src/httprpc.cpp

src/qt/bitcoin.cpp

src/qt/bitcoingui.h

src/qt/bitcoingui.cpp

src/qt/forms/debugwindow.ui

src/qt/receivecoinsdialog.cpp

src/qt/receiverequestdialog.h

src/qt/receiverequestdialog.cpp

src/qt/rpcconsole.h

src/qt/rpcconsole.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletframe.h

src/qt/walletframe.cpp

src/qt/walletmodel.h

src/qt/walletmodel.cpp

src/qt/walletview.h

src/qt/walletview.cpp

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

Details

Original commit messages below:

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 6284

src/httprpc.cpp

src/qt/bitcoin.cpp

src/qt/bitcoingui.h

src/qt/bitcoingui.cpp

src/qt/forms/debugwindow.ui

src/qt/receivecoinsdialog.cpp

src/qt/receiverequestdialog.h

src/qt/receiverequestdialog.cpp

src/qt/rpcconsole.h

src/qt/rpcconsole.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletframe.h

src/qt/walletframe.cpp

src/qt/walletmodel.h

src/qt/walletmodel.cpp

src/qt/walletview.h

src/qt/walletview.cpp

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

Revision Contents
Changeset List