Multiwallet for the GUI #12610

jonasschnelli · 2018-03-06T05:24:35Z

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)

Sjors

I still think it's better to use the menu rather than add a dropbox, but as long as it's easy to change later it doesn't matter. I left some comments to better separate UI from model to make such a change easier. That already improved in this PR compared to #11383.

Nit: "Loading wallet" should be "Loading wallets" if there's more than 1.

Sjors · 2018-03-06T15:34:05Z

src/qt/bitcoingui.cpp

The first item in the menu is now created in the UI file, while new ones are created programatically. Can you make them all programmatically or maybe find a way to clone elements? Can be done in a future PR too.

Sjors · 2018-03-06T15:41:36Z

src/qt/walletmodel.cpp

Why is this a method on WalletModel? Otherwise you could use m_wallet_models->count(). Maybe we need a WalletsModel class to hold on to a series of wallets?

I think the best place for wallets manager functions like this is currently the WalletModel, long term, we may want to have a WalletsManager (see #12587).

Sjors · 2018-03-06T16:00:46Z

src/qt/bitcoingui.cpp

Seems to need a line break above:

It's fixed right? At least there's \n in the Amount: above.

luke-jr · 2018-03-06T18:53:15Z

This is strictly an inferior design to #11383, which already has ACKs and is ready for merging as-is.

The minor other improvements can be done on top of that.

Sjors · 2018-03-06T22:16:46Z

@luke-jr it's not strictly better; this PR has a cleaner separation of model and view. I can't speak to the other differences.

laanwj · 2018-03-12T16:58:03Z

Let's stop arguing just for sake of arguing and help move this forward. If this is not the perfect design I'm sure it can be improved in a later PR. I'll test it later.

luke-jr · 2018-03-12T17:05:41Z

Why does stop arguing mean merging this PR? #11383 is already reviewed and ready to merge...

promag · 2018-03-14T14:50:06Z

Should rebase to have external wallet support (#11687).

promag

Tested ACK.

Receiving addresses and Sending addresses dialogs could have the wallet name in the dialog title.

promag · 2018-03-14T15:04:45Z

src/qt/sendcoinsdialog.cpp

Not sure about the underline, it's already bold.

I think its okay since the used wallet is a pretty critical information. Overhauling the send-confirmation dialog is something that would be possible in the long run.

promag · 2018-03-14T15:07:40Z

src/qt/bitcoingui.cpp

nit, remove extra space before {.

promag · 2018-03-14T15:08:26Z

src/qt/bitcoingui.cpp

It's fixed right? At least there's \n in the Amount: above.

promag · 2018-03-15T00:57:09Z

Maybe running with -disablewallet should not show the wallet selector? Currently looks like:

jonasschnelli · 2018-03-18T08:45:47Z

Added a commit that fixes the -disablewallet case (also won't show the RPCConsole wallet selector if only one wallet is present).

fanquake · 2018-03-21T15:54:49Z

Testing 3b156cf on top of master (4ad3b3c). macOS 10.13.3, Qt 5.10.1
Wallet Selector:

In the debug console if I try and use the help command with either wallet selected I get an error. Selecting (none) and using help works fine:

jnewbery

Tested ACK 3b156cf01fda530cca127079d630c295d839efc1, although I'm not very familiar with the qt code.

To aid review, the commits could be tidied up a little (some code is added and removed in a later commit) and squashed a bit.

A couple of comments inline.

jnewbery · 2018-03-21T19:26:51Z

src/qt/walletmodel.cpp

I'm not sure that we should strip .dat from the wallet name now that #11687 is merged. Perhaps @ryanofsky has an opinion on this?

Also note that new default wallets will have the name "" (the empty string). Perhaps QT should display something different in this case?

EDIT: for now, I think the only way we can have the default wallet with its name set to "" is if we start bitcoin with no wallet arguments, in which case multiwallet isn't active and the dropdown won't appear. However, in future we'll have dynamic wallet loading/unloading, in which case we could have the default "" wallet plus named wallets.

Good point. For sure something to deal with later.

jnewbery · 2018-03-21T20:20:00Z

src/qt/rpcconsole.cpp

I think this comment is outdated and can be removed

promag · 2018-03-26T09:27:18Z

Tested ACK 3b156cf.

Running with -disablewallet works as expected and @fanquake point is fixed. Agree with @jnewbery that commits could be tidied up a little, but I don't think that it should block this anymore.

…t at all until needed

jonasschnelli · 2018-03-26T11:43:19Z

Force push fixed the outdated commit.
Can be verified by comparing 3b156cf01fda530cca127079d630c295d839efc1 against 779c5f9.

779c5f9 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli) dc6f150 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli) 4826ca4 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli) cfa4133 GUI: RPCConsole: Log wallet changes (Luke Dashjr) b6d04fc Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr) 12d8d26 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli) d1ec34a Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr) d49cc70 Qt: Add wallet selector to debug console (Jonas Schnelli) d558f44 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr) 85d5319 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr) e449f9a Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr) 3dba3c3 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

instagibbs · 2018-03-26T19:11:17Z

argh, sorry for the late review

this logic here means that anytime we do a wallet match on rpc debug console, it will not find the wallet unless it has no .dat extension: 3dba3c3#diff-8c9d79ba40bf702f01685008550ac100R491

This also means you can load two wallets(eg wallet.dat and wallet), and they will look the same in the drop down selector.

imo if we're supporting arbitrary filename extensions we should just display the entire name

ryanofsky · 2018-03-26T19:20:47Z

imo if we're supporting arbitrary filename extensions we should just display the entire name

Agree, it would be good to drop the special .dat extension handling (which is now in WalletModel::getWalletName().

jnewbery · 2018-03-26T20:31:23Z

I also agree that we should remove the special .dat extension handling

luke-jr · 2018-03-26T22:33:04Z

Totally un-user-friendly. If we merged #11383 without the ugly regressions herein, it wouldn't have been a problem...

Summary: Completes T440 Original commit messages below: ------------------------------- PR12610: Merge #12610: Multiwallet for the GUI 779c5f9 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli) dc6f150 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli) 4826ca4 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli) cfa4133 GUI: RPCConsole: Log wallet changes (Luke Dashjr) b6d04fc Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr) 12d8d26 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli) d1ec34a Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr) d49cc70 Qt: Add wallet selector to debug console (Jonas Schnelli) d558f44 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr) 85d5319 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr) e449f9a Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr) 3dba3c3 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 bitcoin/bitcoin#12610 (comment) 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). Reviewers: deadalnix, schancel, #bitcoin_abc, Fabien Reviewed By: deadalnix, #bitcoin_abc, Fabien Subscribers: Fabien, teamcity Differential Revision: https://reviews.bitcoinabc.org/D1979

779c5f9 Qt: hide RPCConsole wallet selector when no wallets are present (Jonas Schnelli) dc6f150 Qt: show wallet name in request dlg in case of multiwallet (Jonas Schnelli) 4826ca4 Qt: show wallet name in send confirmation dlg in case of multiwallet (Jonas Schnelli) cfa4133 GUI: RPCConsole: Log wallet changes (Luke Dashjr) b6d04fc Qt: Get wallet name from WalletModel rather than passing it around (Luke Dashjr) 12d8d26 Qt: When multiple wallets are used, include in notifications the name (Jonas Schnelli) d1ec34a Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding it at all until needed (Luke Dashjr) d49cc70 Qt: Add wallet selector to debug console (Jonas Schnelli) d558f44 Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/ (Luke Dashjr) 85d5319 Qt: Ensure UI updates only come from the currently selected walletView (Luke Dashjr) e449f9a Qt: Add a combobox to toolbar to select from multiple wallets (Luke Dashjr) 3dba3c3 Qt: Load all wallets into WalletModels (Luke Dashjr) Pull request description: This is an overhaul of bitcoin#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

luke-jr added 4 commits March 6, 2018 12:03

Qt: Load all wallets into WalletModels

3dba3c3

Qt: Add a combobox to toolbar to select from multiple wallets

e449f9a

Qt: Ensure UI updates only come from the currently selected walletView

85d5319

Bugfix: RPC: Add missing UnregisterHTTPHandler for /wallet/

d558f44

jonasschnelli added the GUI label Mar 6, 2018

jonasschnelli force-pushed the 2018/03/multiwallet branch from cfca2c5 to 98cce65 Compare March 6, 2018 07:57

Sjors reviewed Mar 6, 2018

View reviewed changes

promag reviewed Mar 14, 2018

View reviewed changes

Sjors mentioned this pull request Mar 14, 2018

Refactor: separate gui from wallet and node #10244

Merged

This was referenced Mar 16, 2018

GUI unresponsive during slow operations #10504

Closed

Qt GUI Feature Request: Load wallet file #7675

Closed

MacOSX: wallet.dat content should be stored in Mac OSX Keychain #2545

Closed

jonasschnelli force-pushed the 2018/03/multiwallet branch from 98cce65 to 3b156cf Compare March 18, 2018 08:39

jnewbery reviewed Mar 21, 2018

View reviewed changes

jonasschnelli and others added 7 commits March 26, 2018 18:40

Qt: Add wallet selector to debug console

d49cc70

Qt: QComboBox::setVisible doesn't work in toolbars, so defer adding i…

d1ec34a

…t at all until needed

Qt: When multiple wallets are used, include in notifications the name

12d8d26

Qt: Get wallet name from WalletModel rather than passing it around

b6d04fc

GUI: RPCConsole: Log wallet changes

cfa4133

Qt: show wallet name in send confirmation dlg in case of multiwallet

4826ca4

Qt: show wallet name in request dlg in case of multiwallet

dc6f150

Qt: hide RPCConsole wallet selector when no wallets are present

779c5f9

jonasschnelli force-pushed the 2018/03/multiwallet branch from 3b156cf to 779c5f9 Compare March 26, 2018 11:42

jonasschnelli merged commit 779c5f9 into bitcoin:master Mar 26, 2018

jonasschnelli mentioned this pull request Mar 26, 2018

Basic Multiwallet GUI support #11383

Closed

instagibbs mentioned this pull request Mar 26, 2018

multiwallet in GUI .dat gets dropped, leads to bug and ambiguity #12794

Closed

ryanofsky mentioned this pull request Mar 27, 2018

Add BitcoinApplication & RPCConsole tests #11625

Merged

jnewbery mentioned this pull request Apr 10, 2018

[wallet] loadwallet RPC - load wallet at runtime #10740

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Multiwallet for the GUI #12610

Multiwallet for the GUI #12610

Uh oh!

Conversation

jonasschnelli commented Mar 6, 2018

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Mar 6, 2018

Uh oh!

Sjors commented Mar 6, 2018

Uh oh!

laanwj commented Mar 12, 2018

Uh oh!

luke-jr commented Mar 12, 2018

Uh oh!

promag commented Mar 14, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 15, 2018

Uh oh!

jonasschnelli commented Mar 18, 2018

Uh oh!

fanquake commented Mar 21, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 26, 2018

Uh oh!

jonasschnelli commented Mar 26, 2018

Uh oh!

instagibbs commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 26, 2018

Uh oh!

jnewbery commented Mar 26, 2018

Uh oh!

luke-jr commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

instagibbs commented Mar 26, 2018 •

edited

Loading