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

Introduce WalletModel and loadWallet functionality #417

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented Aug 23, 2024

When a user selects a wallet from the WalletSelect menu the wallet controller can now load the wallet data in and the name and balance will appear in the WalletBadge

The WalletQmlModel is introduced that hodes the interface to the backend wallet and provides the balance to the gui as a formatted string. The formatted string isn't quite as nice as specified in the figma. Instead it just uses the satoshi formatting currently provided by our GUI utils. The advanced formatting will be added as its own PR so that it can be reviewed separately.

WalletController has been renamed to WalletQmlController to not conflict with the qt widgets controller. A function to set the selected wallet has been added. This function loads or creates the backend wallet interface and a WalletQmlModel owns the interface and is set to the controller's selectedWallet property. This is how the gui will gain access to the wallet's information.

Loading encrypted wallets is not currently handled as we need additional dialogs. This will be done in a separate PR so that it can be reviewed independantly.

A handler is also added to the wallet controller to handle background loading of wallet either through the rpc interface or at startup.

Initial state and loading states of the wallet selector have not been implemented yet and will be done separately so it currently will just show 0 balance until a wallet is properly loaded.

@MarnixCroes
Copy link
Contributor

forgot to push some stuff?
some things seem to be missing and CI is failing

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

This is a sufficient starting point, @johnny9 fix CI?

@johnny9 johnny9 force-pushed the wallet-model branch 2 times, most recently from 7c99b8e to df375c0 Compare September 13, 2024 03:19
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

df375c0

  • On regtest, I generated a wallet and did generatetoaddress 200
    Then the balance was displayed like this
    S

QT:
S

  • regularly it is rather slow or not working
    (it's not because they are big unused wallets that need to load, it works fine when using QT)
screencastload.webm
  • I've had a couple times that after closing the app the Saving... window stayed on for multiple minutes while there seems to be no reason as there was no heavy usage that needed to be saved, maybe related to previous point

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

26b5ef9
the negative balance issue is fixed

  • Sometimes it does, but oftenly it doesn't work as supposed
Screencast.webm
  • the balance doesn't update when balanceChanged
  • the shutting down window shows all the time now (indefinitely, many minutes, have to manually kill it) when shutting down from terminal, doesn't happen when clicking the X of the app
  • nit, some amount can be a bit cut off, same is for the wallet name
    image

@johnny9
Copy link
Contributor Author

johnny9 commented Sep 24, 2024

update from 26b5ef9 to d155eed

  • Unloaded wallets when shutdown is requested.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d155eed

This is fine as the initial template for the models and controller, balanceChanged is not properly wired.

I would say the idea of loading is not yet finished, we should implement the functionality of loading an encrypted wallet next.

QString WalletQmlModel::balance() const
{
if (!m_wallet) {
return "0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this leaves the wallet dropdown in a weird looking state, we'd want to revisit balance anyways on subsequent updates, just noting that this should all change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the wallet badge needs more states and will be one of the next things to tackle in the wallet selector component.

@@ -126,7 +62,7 @@ Button {
CoreText {
id: balanceText
visible: root.showBalance
text: formatSatoshis(12300)
text: "₿ " + root.balance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we'll also need the sat symbol :P

@hebasto
Copy link
Member

hebasto commented Sep 27, 2024

Concept ACK.

When a user selects a wallet from the WalletSelect menu the
wallet controller can now load the wallet data in and the
name and balance will appear in the WalletBadge
@johnny9
Copy link
Contributor Author

johnny9 commented Oct 20, 2024

Update from d155eed to b41a0e4

  • Additional #ifdef to exclude functionality when --disable-wallet

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b41a0e4

it sometimes happens that the walletname shown as loaded in the navigationbar is not the same as the wallet shown as loaded/selected in the wallet select dropdown

Screencast.from.20-10-2024.10.25.24.webm

as a result, clicking another wallet doesn't seem to work as it doesn't update the selected walletname in navbar

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested manually b41a0e4 in -regtest

As soon as bitcoin-qt starts, it seems that loading the latest used wallet is quite slow and takes like 5 secs to show it up on the menu.

Also it's very slow to load/ switch to another wallet (and only non/ 0 balance wallets), for wallets with some balance (using -regtest), doesn't work at all (selected wallet doesn't change so balance either).

Last thing for now, in Qt we have the list of wallets in the file menu to select and load and when there are more then 1 wallet loaded (multi-wallet mode) there's combo box with the loaded wallets to switch between them. I think the designers or Christoph showed me some prototype where there were small icons in the list to indicate which wallet is loaded or not, perhaps it should be another icon/ button to select only the loaded wallets, unloaded or both/ all.

@johnny9
Copy link
Contributor Author

johnny9 commented Nov 25, 2024

All of the issues commented so far are going to explode the complexity of this PR beyond anything reviewable. I just wanted to create the boilerplate for the classes that the UI will interact with. Each of the above issues should be a separate PR as thate functionality gets added to the GUI.

The only thing this is intended to do is list the wallets in the folder and when one is selected it loads and shows the very basic formatted balance. The more advanced formatting, for instance, should be added and reviewed in its own PR as well as they asynchronous loading and event handling.

Edit: I went ahead and solved the issue with work being done on the gui thread. The creation is now done on QThread m_worker_thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants