gui: Add dynamic wallets support by promag · Pull Request #13100 · bitcoin/bitcoin

promag · 2018-04-26T23:01:22Z

This PR adds a menu entry Open Wallet to support loading existing wallets in the UI.

ken2812221 · 2018-04-27T01:33:37Z

~~If the wallet folder name contains Chinese, it will create a new folder that has same wallet file with a weird name on Windows, maybe the encoding issue~~ (Not related)

Also, the seperator is incorrect on Windows

promag · 2018-04-27T08:36:44Z

@ken2812221 does the problem exists if you use the RPC command?

ken2812221 · 2018-04-27T08:49:38Z

~~It shows wallet not found on RPC.~~ (Not related)

promag · 2018-04-27T08:55:12Z

@ken2812221 but it does exists right? What if you launch the application with -wallet=d:\...?

ken2812221 · 2018-04-27T09:14:04Z

-wallet=d:\錢包 does works, but the name displayed in Qt is incorrect

And it crash when I quit Qt

Checked that this isn't an issue on Linux.

ryanofsky · 2018-04-27T12:47:40Z

Maybe windows or boost is using big5 encoding. which ends in a right square bracket ]

>>> '錢包'.encode('big5')
b'\xbf\xfa\xa5]'
>>> '錢包'.encode('big5').decode('utf8', 'replace')
'���]'

promag · 2018-04-27T14:08:45Z

@ken2812221 thanks for the review and bug report. Let's move the bug discussion to #13103.

ken2812221 · 2018-04-29T10:56:27Z

src/qt/bitcoingui.cpp

Use QDir::toNativeSeparators to convert / to \ on Windows

jnewbery · 2018-04-30T18:12:05Z

src/interfaces/node.cpp

This seems almost exactly duplicated from loadwallet() in rpcwallet.cpp. Does it make sense to factor it out to avoid repetition?

Yeah, I would like to move to WalletManager

@jnewbery done in #9888dc2.

jnewbery · 2018-04-30T18:14:24Z

src/interfaces/node.cpp

I don't understand why you're passing back an error string if it's always the string ops

Ops, it's unfinished. Not sure if it should return the pair.

Removed, changed the return type.

jnewbery · 2018-04-30T18:15:01Z

src/qt/bitcoingui.cpp

The use of auto here makes it difficult to understand the result.second below.

No an issue now, return value is not necessary.

meshcollider · 2018-05-02T09:12:30Z

Concept ACK

promag · 2018-05-03T09:37:52Z

src/qt/bitcoingui.cpp

@jnewbery @laanwj ATM the wallet is loaded in the UI thread, should I refactor to load it in the background?

I think that's a good future enhancement. Not necessary in the initial version IMO.

jnewbery · 2018-05-03T20:03:56Z

Please update description to say that this fixes #7675

promag · 2018-05-04T22:06:22Z

Please update description to say that this fixes #7675

Done. Also rebased.

promag · 2018-05-23T11:57:17Z

Rebased on master.

jnewbery · 2018-05-23T13:29:34Z

wallet_multiwallet.py fails

promag · 2018-05-24T13:21:43Z

Rebased.

jnewbery · 2018-05-24T14:51:14Z

src/qt/bitcoingui.cpp

Suggestion: start in the data directory for the current network (mainnet/testnet/regtest) instead of the home directory, since that's where the wallet files and folders will usually be (and because it may be difficult to traverse down into .bitcoin).

jnewbery · 2018-05-24T14:51:46Z

src/qt/bitcoingui.cpp

Why only show directories and not symlinks? Wallets can be .dat files.

DrahtBot · 2018-10-20T09:52:28Z

Needs rebase

meshcollider · 2018-11-03T06:21:16Z

It'd be nice to get this in to 0.18, is that feasible? Needs a rebase

Overall this is looking good, concept ACK with some code review, I'll review more once rebased/the comments above have been addressed and the WIP fixup commit is done

Sjors · 2018-12-10T17:42:47Z

For a followup, once #11082 is merged, we should add a wallet= entry to it once a wallet is created.

Noteable changes that may cause a headache during rebase:

Add WalletLocation class #14350 (by yourself, WalletLocation needed in various places)
Refactor: Start to separate wallet from node #14437 (you need to pass interfaces::Chain& chain to CreateWallet, which needs it for Verify)

luke-jr · 2019-01-02T11:10:31Z

src/qt/bitcoingui.cpp

    openAction = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("Open &URI..."), this);
    openAction->setStatusTip(tr("Open a bitcoin: URI or payment request"));

+    m_new_wallet_action = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("New &Wallet"), this);


If we're going to use icons, I think they ought to be different...

luke-jr · 2019-01-02T11:13:30Z

Why is there a separate File and Wallet menu in the screenshot above? They should be the same menu...

jonasschnelli · 2019-01-04T06:33:13Z

@promag: care to rebase/pick this up again?

Sjors · 2019-01-17T16:10:30Z

A lot of this has been moved to smaller pull requests. E.g. there's now a separate PR for just the open wallet feature #15153. I suppose "unloading" a wallet would be fairly trivial on top of that.

For my own work on #14912 I'd love to have a (proof of concept) "create wallet" PR to build on top of. Ideally it would a dialog, with a check box for the read-only feature (and later for the empty wallet feature in #14938).

jnewbery · 2019-01-17T16:20:29Z

Feature freeze is on Feb 15th. I'd really love to have this in v0.18.

@promag - any chance of a rebase so we can start testing & reviewing?

promag · 2019-01-17T16:55:18Z

@jnewbery please see #15101 and then #15153.

0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of #13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094

jnewbery · 2019-01-18T20:37:59Z

Can this PR be closed? I think all changes are covered by other PRs, and issue #13059 tracks the total progress.

promag · 2019-01-18T20:40:38Z

Thanks for updating that @jnewbery.

0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094

fanquake added the GUI label Apr 27, 2018

promag force-pushed the 2018-04-ui-open-wallet branch from ce11cae to f61aff5 Compare April 27, 2018 08:30

promag mentioned this pull request Apr 27, 2018

Invalid wallet path with Chinese characters in windows #13103

Closed

ken2812221 reviewed Apr 29, 2018

View reviewed changes

jnewbery mentioned this pull request Apr 30, 2018

Dynamic wallet load / create / unload #13059

Closed

10 tasks

jnewbery reviewed Apr 30, 2018

View reviewed changes

jnewbery mentioned this pull request Apr 30, 2018

ui: Support wallets loaded dynamically #13097

Merged

promag force-pushed the 2018-04-ui-open-wallet branch from f61aff5 to 462bf7f Compare May 2, 2018 16:28

promag commented May 3, 2018

View reviewed changes

promag force-pushed the 2018-04-ui-open-wallet branch from 462bf7f to 0bdb5c6 Compare May 4, 2018 22:05

promag force-pushed the 2018-04-ui-open-wallet branch 2 times, most recently from 8c9cf75 to 9170254 Compare May 23, 2018 11:56

promag force-pushed the 2018-04-ui-open-wallet branch from 9170254 to 3e88848 Compare May 23, 2018 13:50

maflcko changed the title ~~Add menu entry to open wallet~~ gui: Add menu entry to open wallet May 23, 2018

promag force-pushed the 2018-04-ui-open-wallet branch from 3e88848 to a27b300 Compare May 24, 2018 13:20

jnewbery reviewed May 24, 2018

View reviewed changes

ghost approved these changes Jun 6, 2018

View reviewed changes

This was referenced Oct 9, 2018

Refactor: Start to separate wallet from node #14437

Merged

Add BIP70 deprecation warning and allow building GUI without BIP70 support #14451

Merged

DrahtBot added the Needs rebase label Oct 20, 2018

meshcollider added the Wallet label Nov 9, 2018

fanquake added this to the 0.18.0 milestone Nov 10, 2018

luke-jr reviewed Jan 2, 2019

View reviewed changes

promag mentioned this pull request Jan 3, 2019

Support creating an empty wallet #14938

Closed

promag mentioned this pull request Jan 4, 2019

gui: Add WalletController #15101

Merged

promag closed this Jan 18, 2019

promag deleted the 2018-04-ui-open-wallet branch January 18, 2019 20:40

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

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

Uh oh!

Uh oh!

ken2812221 commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Apr 27, 2018

Uh oh!

ken2812221 commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Apr 27, 2018

Uh oh!

ken2812221 commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Apr 27, 2018

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 Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

meshcollider commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 3, 2018

Uh oh!

promag commented May 4, 2018

Uh oh!

promag commented May 23, 2018

Uh oh!

jnewbery commented May 23, 2018

Uh oh!

promag commented May 24, 2018

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!

DrahtBot commented Oct 20, 2018

Uh oh!

meshcollider commented Nov 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jan 2, 2019

promag commented Apr 26, 2018 •

edited

Loading

ken2812221 commented Apr 27, 2018 •

edited

Loading

ken2812221 commented Apr 27, 2018 •

edited

Loading

ken2812221 commented Apr 27, 2018 •

edited

Loading

ryanofsky commented Apr 27, 2018 •

edited

Loading

promag Apr 30, 2018 •

edited

Loading

promag May 2, 2018 •

edited

Loading

meshcollider commented Nov 3, 2018 •

edited

Loading