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] Indicate wallet is syncing after completing installer #1370

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Oct 8, 2024

This is to resolve #1361.

The home page considers the wallet to be syncing if its height is 0. In this case, the balance will slowly blink and a "Syncing..." text will appear just below.

The home page will check the wallet's height upon each cache refresh, and will reload the home page once the syncing has completed so that the updated balance is displayed without the user needing to do anything.

Both the blinking balance and "Syncing..." text use a new Carousel widget that cycles through different child widgets at a specified rate.

EDIT: I've added an extra commit to address #1363 as that is also related to the wallet height and cache refresh.

The `amount` function can call `amount_with_size` rather than
repeating the underlying call to `render_amount`.
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 8, 2024

Here's how it looks when importing a descriptor:
Screencast from Import Descriptor

The wallet's height is taken from the cache and is used to check
if the wallet has been initially synced after creation.

For the remote backend, the cache refresh should be done with the
usual frequency while the wallet's height is 0 so that the sync
completion can be detected sooner.
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 11, 2024

I've made the following changes:

  1. Update the home page from the cache even when the home page is not currently selected. This way, the user won't need to wait on the home page for the syncing status to disappear and can browse other pages in the meanwhile.
  2. Given (1), the home page will only reload automatically once syncing is completed if the home page is currently selected. Otherwise, it will reload once the user returns to the home page.
  3. The home page's reload method will only reload data if the wallet is not currently syncing.

@edouardparis
Copy link
Member

Note: while importing descriptor with a bitcoin core, the synced state is still empty which is normal because user has to trigger a rescan but it may be confusing as the "syncing" implies that the wallet is updating to a correct state.

Copy link
Member

@edouardparis edouardparis left a comment

Choose a reason for hiding this comment

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

ACK 6f4eb79

@kloaec
Copy link
Collaborator

kloaec commented Oct 18, 2024

That's a good point, and bad UX from our import descriptor flow.

I suppose the user should ask for the rescan at import time, before going to the wallet.
We could also, instead or at the same time, put an explanation banner on every empty wallet (no tx), telling them to check the Support page for rescan? (to be written)

@edouardparis edouardparis merged commit 67c9262 into wizardsardine:master Oct 18, 2024
31 checks passed
@jp1ac4 jp1ac4 deleted the gui-check-wallet-height branch October 18, 2024 14:58
@nondiremanuel
Copy link
Collaborator

As per what was discussed offline:

We could also not show the "Syncing..." message for a local bitcoind wallet with height 0 to avoid misleading the user. In that case, the UX will be the same as currently

This should be a valid option since in the cases in which wallet height is 0 we don't expect any update from the first poll.

edouardparis added a commit that referenced this pull request Oct 22, 2024
9208cd1 gui: don't treat wallets using bitcoind as syncing (Michael Mallan)
f34bb1b gui: include node type in embedded daemon backend (Michael Mallan)

Pull request description:

  This is a follow-up to #1370.

  If the user has imported a descriptor and is using bitcoind as a local node, then they will need to perform a rescan in order
  to see past transactions. Treating the wallet as syncing in this case could mislead the user that a rescan is being performed. Therefore, it's better to keep the past behaviour here to avoid further confusion.

ACKs for top commit:
  edouardparis:
    ACK 9208cd1

Tree-SHA512: fabb3289f01c68309cd511854b0ea249e5fa1279c736bfc61c3140ca3352dfc24a1be46c977ed21c528a67c3d1f0e16521d94aaa0ee1c17d8f9ed4f3403547d0
edouardparis added a commit that referenced this pull request Oct 25, 2024
…pletes after opening

ec7556e gui(home): indicate existing wallet is syncing (Michael Mallan)
f7314aa gui(cache): include last poll timestamp (Michael Mallan)
9dd737b gui: upgrade liana dependency (Michael Mallan)

Pull request description:

  This PR uses the changes from #1376 to complete #1373.

  It builds on the changes from #1370 to consider an existing wallet (that uses a local backend) to be syncing until the first poll completes after opening the GUI, where "existing wallet" means one that has positive height (since a newly created wallet has height 0).

  Note that for an external Liana daemon,  this logic is only applied in case the last poll timestamp has already been set when starting the GUI. Otherwise, we can't be sure if this external daemon will ever set this value.

ACKs for top commit:
  edouardparis:
    ACK ec7556e

Tree-SHA512: a6db8e510a7d24ca554965512e289eb271dab41871bde9362a7af71b73c771431025b1be1092bab98b106fb520d8c6959d712909d1db9a21731f3a79c9f766a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[GUI] Indicate wallet is syncing after creation
4 participants