Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Add dynamic wallets support #13100

Closed
wants to merge 6 commits into
base: master
from

Conversation

@promag
Copy link
Member

promag commented Apr 26, 2018

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

Fixes #7675.

@fanquake fanquake added the GUI label Apr 27, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Apr 27, 2018

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
image

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch Apr 27, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Apr 27, 2018

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

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Apr 27, 2018

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

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Apr 27, 2018

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

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Apr 27, 2018

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

And it crash when I quit Qt

Checked that this isn't an issue on Linux.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Apr 27, 2018

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

This comment has been minimized.

Copy link
Member Author

promag commented Apr 27, 2018

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

src/qt/bitcoingui.cpp Outdated
@@ -701,6 +707,20 @@ void BitcoinGUI::openClicked()
}
}

void BitcoinGUI::openWalletClicked()
{
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),

This comment has been minimized.

@ken2812221

ken2812221 Apr 29, 2018

Member

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

This comment has been minimized.

@promag

promag May 2, 2018

Author Member

Done.

This comment has been minimized.

@ghost

@jnewbery jnewbery referenced this pull request Apr 30, 2018

Open

Dynamic wallet load / create / unload #13059

8 of 10 tasks complete
src/interfaces/node.cpp Outdated
{
#ifdef ENABLE_WALLET
try {
std::string dummy_warning;

This comment has been minimized.

@jnewbery

jnewbery Apr 30, 2018

Member

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

This comment has been minimized.

@promag

promag Apr 30, 2018

Author Member

Yeah, I would like to move to WalletManager

This comment has been minimized.

@promag

promag May 2, 2018

Author Member

@jnewbery done in #9888dc2.

src/interfaces/node.cpp Outdated
wallet->postInitProcess();
return { MakeWallet(*wallet), std::string() };
} catch (...) {
return { std::unique_ptr<Wallet>(), "ops" };

This comment has been minimized.

@jnewbery

jnewbery Apr 30, 2018

Member

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

This comment has been minimized.

@promag

promag Apr 30, 2018

Author Member

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

This comment has been minimized.

@promag

promag May 2, 2018

Author Member

Removed, changed the return type.

src/qt/bitcoingui.cpp Outdated
QFileDialog::ShowDirsOnly
| QFileDialog::DontResolveSymlinks);
if (fileName.isEmpty()) return;
const auto& result = m_node.loadWallet(fileName.toStdString());

This comment has been minimized.

@jnewbery

jnewbery Apr 30, 2018

Member

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

This comment has been minimized.

@promag

promag May 2, 2018

Author Member

No an issue now, return value is not necessary.

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented May 2, 2018

Concept ACK

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch May 2, 2018

src/qt/bitcoingui.cpp Outdated
if (fileName.isEmpty()) return;

std::vector<std::string> errors;
m_node.loadWallet(QDir::toNativeSeparators(fileName).toStdString(), errors);

This comment has been minimized.

@promag

promag May 3, 2018

Author Member

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

This comment has been minimized.

@jnewbery

jnewbery May 3, 2018

Member

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

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 3, 2018

Please update description to say that this fixes #7675

@jnewbery jnewbery added this to In progress in Multiwallet support May 3, 2018

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch May 4, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 4, 2018

Please update description to say that this fixes #7675

Done. Also rebased.

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch 2 times, most recently May 23, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 23, 2018

Rebased on master.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 23, 2018

wallet_multiwallet.py fails

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch May 23, 2018

@MarcoFalke MarcoFalke changed the title Add menu entry to open wallet gui: Add menu entry to open wallet May 23, 2018

@promag promag force-pushed the promag:2018-04-ui-open-wallet branch May 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 24, 2018

Rebased.

src/qt/bitcoingui.cpp Outdated
void BitcoinGUI::openWalletClicked()
{
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),
QString(),

This comment has been minimized.

@jnewbery

jnewbery May 24, 2018

Member

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

src/qt/bitcoingui.cpp Outdated
{
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"),
QString(),
QFileDialog::ShowDirsOnly

This comment has been minimized.

@jnewbery

jnewbery May 24, 2018

Member

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

This comment has been minimized.

@promag

promag May 24, 2018

Author Member

Right.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 25, 2018

@jnewbery wrote:

Longer term [...] to have bitcoind/qt not create a default wallet at startup if one didn't exist. This PR is the final blocker for that to happen.

That would be nice indeed. I find it quite annoying that you can't start bitcoind without any wallet loaded. And I have too many wallet.dat files floating around that I need to carefully check are actually safe to delete :-)

I'll test again after rebase.

Re #11485 listing wallets, this could be added later. It's nice to just show a list of wallet names that can be loaded, avoiding the file dialog.

For this PR it should be enough if the file dialog opens in walletdir, or datadir if that doesn't exist. (the latter is impossible now, so maybe a source code comment warning would suffice).

Creating a new wallet should probably create a data and/or wallet dir, though it's easier if #13761 is merged first.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Sep 25, 2018

@promag - what's the status here? This PR seems to have stalled.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Sep 25, 2018

@jnewbery I'm rebasing on #14291. Current implementation is more suitable to external wallets.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Sep 25, 2018

@Sjors @jnewbery this is what it looks like:

screen shot 2018-09-25 at 12 43 04

For this PR it should be enough if the file dialog opens in walletdir, or datadir if that doesn't exist.

If we handle wallets in the -walletdir differently, then, for external wallets, I think we should point the file dialog to QDesktopServices::DocumentsLocation or whatever.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 25, 2018

That looks pretty nice. I'd move Open External Wallet to the bottom and just call it Other.... Indeed it makes sense that doesn't use walletdir.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 25, 2018

Doesn't build on linux/win gitian:

  CXXLD    test/test_bitcoin.exe
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/bitcoin-qt.exe
libbitcoin_server.a(libbitcoin_server_a-node.o): In function `getWalletDir':
/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src/interfaces/node.cpp:226: undefined reference to `GetWalletDir[abi:cxx11]()'
collect2: error: ld returned 1 exit status
Makefile:3904: recipe for target 'qt/bitcoin-qt.exe' failed
make[2]: *** [qt/bitcoin-qt.exe] Error 1
make[2]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Makefile:10188: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Makefile:774: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1
Failed run an application inside container
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/bitcoin-qt
libbitcoin_server.a(libbitcoin_server_a-node.o): In function `interfaces::(anonymous namespace)::NodeImpl::getWalletDir()':
/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src/interfaces/node.cpp:226: undefined reference to `GetWalletDir[abi:cxx11]()'
collect2: error: ld returned 1 exit status
Makefile:3904: recipe for target 'qt/bitcoin-qt' failed
make[2]: *** [qt/bitcoin-qt] Error 1
make[2]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src'
Makefile:10188: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-pc-linux-gnu/src'
Makefile:774: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1
Failed run an application inside container
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 20, 2018

Needs rebase
@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Nov 3, 2018

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

@MeshCollider MeshCollider added the Wallet label Nov 9, 2018

@fanquake fanquake added this to the 0.18.0 milestone Nov 10, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 10, 2018

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:

  • #14350 (by yourself, WalletLocation needed in various places)
  • #14437 (you need to pass interfaces::Chain& chain to CreateWallet, which needs it for Verify)
@@ -330,6 +332,15 @@ void BitcoinGUI::createActions()
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);

This comment has been minimized.

@luke-jr

luke-jr Jan 2, 2019

Member

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

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 2, 2019

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 4, 2019

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

@promag promag referenced this pull request Jan 4, 2019

Merged

gui: Add WalletController #15101

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member Author

promag commented Jan 17, 2019

@jnewbery please see #15101 and then #15153.

jonasschnelli added a commit that referenced this pull request Jan 18, 2019

Merge #15101: gui: Add WalletController
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

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 18, 2019

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

@promag promag closed this Jan 18, 2019

@promag promag deleted the promag:2018-04-ui-open-wallet branch Jan 18, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 18, 2019

Thanks for updating that @jnewbery.

@jonasschnelli jonasschnelli removed this from In progress in Multiwallet support Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.