leather-io / extension

Leather browser extension
https://leather.io
MIT License
305 stars 144 forks source link

Loading state for balances on home screen #4787

Closed kyranjamie closed 5 months ago

kyranjamie commented 10 months ago

Currently, we show the user's balance as 0 while we're querying the API.

Balance API query in flight Balance loaded
2024-01-09-000048@2x 2024-01-09-000047@2x

How should the UI handle this loading state?

314159265359879 commented 10 months ago

I would like to see some kind of loading animation.

If the balance can't be loaded/determined, I would like to see a meaningful error: The balance can't load because the server isn't accessible from your location, or The balance isn't loading because the service to check your BTC balance isn't available currently.

fabric-8 commented 10 months ago

Looked at what other wallets are doing here — I've seen 2 things:

  1. Whatever number has been shown earlier is cached and shown. As soon as the latest number is available it just updates
  2. Full on skeleton loading state for the entire view until the data is ready

Option 2 is obv. much more heavy-handed, but would offer the benefit to for example also take that as an opportunity to load everything else, ie FTs, but would also increse the scope. Is something along the lines of option 1 possible for?

pete-watters commented 9 months ago

Reading this and I don't think option 1 of showing stale balances really cuts it.

I think option 2 is OK, to show a skeleton while the data is loading however if no balances can be fetched we need to alert the user there is an API issue.

We are dealing with money here so we can't really show stale data and if the data is unavailable we can't have people panicking they have lost there money.

I would even consider this a P1 bug with how things are currently

314159265359879 commented 8 months ago

Should we show different details/explainer depending on the error being hit? We should show a different error when the "too many history entries" error is hit? because there is no known workaround for it for example.

@markmhendrickson Can this be made part of prep for Nakamoto?

markmhendrickson commented 8 months ago

@fabric-8 I believe you subsequently explored some designs for this issue, correct? If so, mind referencing in a comment?

Can this be made part of prep for Nakamoto?

@314159265359879 is it relevant to Nakamoto for any particular reason? It seems like an important issue to resolve soon but not Nakamoto-related to me.

314159265359879 commented 8 months ago

With such a huge upgrade it would be nice if we can rely on the balance data to be accurate. Prevent new users from panicking when 0 balance is erroneously displayed. With the Nakamoto release balances can change faster because transactions can be confirmed quicker.

fabric-8 commented 8 months ago

@markmhendrickson I think this slack thread is the best thing to point to for now, however in the light of general statefulness around home balances I started putting together this little matrix here so we can create a holistic, systemic solution — Mind taking a look and letting me know wether I'm missing something or the appraoch should be different?

Also, just to see if I am understanding this correctly:

Correct?

markmhendrickson commented 8 months ago

This matrix / approach to mapping out the states looks accurate to me. When it comes to "no cache vs. cache available", we can perhaps use the verbs "load" vs. "refresh" (for data) respectively to distinguish the underlying context.

In other words, when there's no cache, we need to "load data" and when there is cache, we need to "refresh data".

Also, instead of saying "overall balance" here, let's get more specific per our balance types to keep track of things. I believe we can best refer to the "overall balance for an account" as the "aggregate total balance" since we want to be clear that it's the aggregate amount of total assets in the account.

Note: My original comment here indicated "aggregate available balance" instead of "total" but then I changed my mind. This is perhaps worth a discussion if we feel "available" might actually be more appropriate for displaying at the top of accounts cc @314159265359879

States apply to both, individual fungible token balances and overall balance

Correct – and also to both the primary value (i.e. crypto e.g. 3 BTC) amount and the secondary value (i.e. fiat e.g. $100) amount within each displayed balance.

If any individual balance returns an error, it'll also "error" the overall balance as it won't be correct anymore

I actually think it may be best to treat the aggregate balance as inclusive of individual balances on a "as available" basis, in that if any of the individual ones return an error or simply can't be queried, that shouldn't stop the aggregate balance from calculating and displaying with the other individual balances it can determine.

The overall balance can run into an error on it's own without individual fungible token balances being affected

Correct, though I'm not sure if the aggregate balance would encounter such independent errors based on data availability concerns alone, since it wouldn't be querying anything directly / apart from summing individual balances.

fabric-8 commented 7 months ago

Updated guidance for the skeleton loader since loading / refetch have been implemented by @alter-eggo can be found here: https://www.figma.com/file/2MLHeIeL6XPVi3Tc2DfFCr/%E2%9D%96-Leather-%E2%80%93-Design-System?type=design&node-id=15390%3A116940&mode=design&t=HO5sYzbkOR8pZ14K-1

fbwoolf commented 5 months ago

The status of this issue is Closed so closing the issue.