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

Query wallet state & transactions only when the wallet is visible #1378

Open
bajtos opened this issue Mar 4, 2024 · 6 comments
Open

Query wallet state & transactions only when the wallet is visible #1378

bajtos opened this issue Mar 4, 2024 · 6 comments

Comments

@bajtos
Copy link
Member

bajtos commented Mar 4, 2024

At the moment, Station Desktop is running a background task that's periodically updating Wallet Balance, Scheduled Rewards and the list of transactions:

https://github.com/filecoin-station/desktop/blob/main/main/wallet.js#L58

This puts a lot of load on the RPC API providers and won't scale.

It's also wasteful, because most of the time, the app is running in the tray and nobody is looking at the data fetched by refreshState.

We should rework the wallet state management to refresh the state only when we are actually rendering it.

  1. The tray menu renders Wallet Balance, which we can update when the user opens the menu.
  2. The main view renders Wallet Balance and Scheduled Rewards; we should refresh only these two fields and only when the main window is visible.
  3. The Wallet view lists all transactions. We should refresh the list of transactions only when the Wallet view is open.

/cc @juliangruber based on my discussion with @jleni

@juliangruber
Copy link
Member

juliangruber commented Mar 5, 2024

💯

I was thinking about making this dependent on the UI being shown too.

This puts a lot of load on the RPC API providers and won't scale.

For reference, we're consuming two different services:

  • JSON-RPC APIs for wallet balance and scheduled rewards
  • Block explorers (like Beryx) for list of transactions, as JSON-RPC APIs don't expose these the way we need them

It's also wasteful, because most of the time, the app is running in the tray and nobody is looking at the data fetched by refreshState.

💯

  1. The tray menu renders Wallet Balance, which we can update when the user opens the menu.

To be precise: The main menu shows wallet balance and scheduled rewards. Both are fetched from JSON-RPC APIs, not block explorers. We should still try to keep the request count down of course.

There's no room for async actions between opening and displaying the menu. We also can't update its contents as it's still showing (electron limitation).

  • We could make the updated balance show the next time the menu is opened
  • We could refresh it on a very long interval (eg 10 minutes)
  1. The main view renders Wallet Balance and Scheduled Rewards; we should refresh only these two fields and only when the main window is visible.

To be precise, also here, this is using JSON-RPC APIs, not the block explorer.

We can implement this by moving the refresh loop for these two fields to the client. Otherwise the client has to tell the backend when it's visible and when not. I think it's fair for the UI to own this, as it's purely a display matter, and not part of Station's business logic.

  1. The Wallet view lists all transactions. We should refresh the list of transactions only when the Wallet view is open.

To be precise, here this is using the block explorer, and has highest priority of fixing.

As above, I propose we move the refresh loop to the client. Wdyt?

About the implementation, we can create a react hook for the loop, that is mounted inside a hook that listens for visibilitychange events.

<onVisible>
  <updateWallet />
</onVisible>

@bajtos
Copy link
Member Author

bajtos commented Mar 6, 2024

  1. The tray menu renders Wallet Balance, which we can update when the user opens the menu.

To be precise: The main menu shows wallet balance and scheduled rewards. Both are fetched from JSON-RPC APIs, not block explorers. We should still try to keep the request count down of course.

There's no room for async actions between opening and displaying the menu. We also can't update its contents as it's still showing (electron limitation).

Oh, that's a bummer. It means we have two options:

  1. Keep showing wallet balance & scheduled rewards in the tray menu. This requires us to keep refreshing these values in the background and accept the cost of frequent JSON-RPC API calls.
  2. Remove the wallet balance & scheduled rewards from the tray menu. I consider this as a step back in the user experience, so I'd rather not do that.
  • We could make the updated balance show the next time the menu is opened

That would be extremely confusing to me. I don't open the menu often, but when I do take a look, then I want to see the current values.

  • We could refresh it on a very long interval (eg 10 minutes)

Yeah, that's an option. I see two issues though:

  • Every time our network grows 10x, we have to adjust the interval and make it even longer
  • It can be confusing to the user if we show a precise value that's actually outdated

To make it less confusing, how about rendering the values in the menu will less precision? E.g. 0.08 FIL instead of 0.081214 FIL, and 57k jobs instead of 57,414 jobs.

The less-precise value does not change so often, and therefore, it's less likely that we show a number that's no longer true.

Then we would end up with two background refresh loops:

  • one running all the time with a lower frequency to keep the menu up to date,
  • another running only while the app window is visible and updating the values more frequently

It feels like a lot of additional complexity, maybe it's not worth doing it right now?

  1. The main view renders Wallet Balance and Scheduled Rewards; we should refresh only these two fields and only when the main window is visible.

To be precise, also here, this is using JSON-RPC APIs, not the block explorer.

We can implement this by moving the refresh loop for these two fields to the client. Otherwise the client has to tell the backend when it's visible and when not. I think it's fair for the UI to own this, as it's purely a display matter, and not part of Station's business logic.

I think this depends on how we implement the refresh for the tray menu item.

IMO, we should share the state (wallet balance & scheduled rewards) between the tray menu and the app window. Since the tray menu is managed by the backend (the main process), I think it also means we must keep the implementation of the refresh RPC calls in the backend.

However, I like the idea of moving the code triggering the refresh to the front end. That way the front-end can use a different refresh interval than the tray menu.

  1. The Wallet view lists all transactions. We should refresh the list of transactions only when the Wallet view is open.

To be precise, here this is using the block explorer, and has highest priority of fixing.

As above, I propose we move the refresh loop to the client. Wdyt?

Sounds great. Maybe we can move the fetch calls to the front end, too? As long as it's still easy to write unit tests for that.

About the implementation, we can create a react hook for the loop, that is mounted inside a hook that listens for visibilitychange events.

<onVisible>
  <updateWallet />
</onVisible>

Nice! I am not that much familiar with React, but I am happy to follow your lead here.

@juliangruber
Copy link
Member

juliangruber commented Mar 7, 2024

  1. The tray menu renders Wallet Balance, which we can update when the user opens the menu.

To be precise: The main menu shows wallet balance and scheduled rewards. Both are fetched from JSON-RPC APIs, not block explorers. We should still try to keep the request count down of course.
There's no room for async actions between opening and displaying the menu. We also can't update its contents as it's still showing (electron limitation).

Oh, that's a bummer. It means we have two options:

  1. Keep showing wallet balance & scheduled rewards in the tray menu. This requires us to keep refreshing these values in the background and accept the cost of frequent JSON-RPC API calls.
  2. Remove the wallet balance & scheduled rewards from the tray menu. I consider this as a step back in the user experience, so I'd rather not do that.

Other options:

  • Put a CF caching layer in front of the 3rd party APIs (bad: decentralized)
  • Host our own JSON-RPC API and block explorer (bad: our team is too small)
  • Build some kind of lotus node into Station (good: most decentralized)

For now I believe keeping the background refresh at a low rate is our best option. Adding a kind of lotus node seems like the most promising option for the future, as it can also be exposed to modules, enabling more use cases.

  • We could make the updated balance show the next time the menu is opened

That would be extremely confusing to me. I don't open the menu often, but when I do take a look, then I want to see the current values.

+1

  • We could refresh it on a very long interval (eg 10 minutes)

Yeah, that's an option. I see two issues though:

  • Every time our network grows 10x, we have to adjust the interval and make it even longer
  • It can be confusing to the user if we show a precise value that's actually outdated

To make it less confusing, how about rendering the values in the menu will less precision? E.g. 0.08 FIL instead of 0.081214 FIL, and 57k jobs instead of 57,414 jobs.

The less-precise value does not change so often, and therefore, it's less likely that we show a number that's no longer true.

That's a really good idea 👏

Then we would end up with two background refresh loops:

  • one running all the time with a lower frequency to keep the menu up to date,
  • another running only while the app window is visible and updating the values more frequently

It feels like a lot of additional complexity, maybe it's not worth doing it right now?

I don't perceive this as a lot of additional complexity. The tasks are:

  • Refactor one loop into two
  • Run the second loop (or its tasks) conditionally
  • Adjust the rendering of menu bar items
  1. The main view renders Wallet Balance and Scheduled Rewards; we should refresh only these two fields and only when the main window is visible.

To be precise, also here, this is using JSON-RPC APIs, not the block explorer.
We can implement this by moving the refresh loop for these two fields to the client. Otherwise the client has to tell the backend when it's visible and when not. I think it's fair for the UI to own this, as it's purely a display matter, and not part of Station's business logic.

I think this depends on how we implement the refresh for the tray menu item.

IMO, we should share the state (wallet balance & scheduled rewards) between the tray menu and the app window. Since the tray menu is managed by the backend (the main process), I think it also means we must keep the implementation of the refresh RPC calls in the backend.

However, I like the idea of moving the code triggering the refresh to the front end. That way the front-end can use a different refresh interval than the tray menu.

As we saw in f4d2280, it's easy to check UI visibility on the backend. We don't have a way of checking whether the UI and the wallet are open, but I believe for now just checking if the UI is open is good enough.

  1. The Wallet view lists all transactions. We should refresh the list of transactions only when the Wallet view is open.

To be precise, here this is using the block explorer, and has highest priority of fixing.
As above, I propose we move the refresh loop to the client. Wdyt?

Sounds great. Maybe we can move the fetch calls to the front end, too? As long as it's still easy to write unit tests for that.

About the implementation, we can create a react hook for the loop, that is mounted inside a hook that listens for visibilitychange events.

<onVisible>
  <updateWallet />
</onVisible>

Nice! I am not that much familiar with React, but I am happy to follow your lead here.

To summarize, I propose this work plan:

  1. Lower precision of stats displayed in the menu
  2. Decouple the refresh loops:
    2.1 On a long 120s interval, refresh wallet balance and scheduled rewards
    2.2 When the UI is visible, on a 30s interval, refresh wallet balance and transaction list

Wdyt?

@bajtos
Copy link
Member Author

bajtos commented Mar 7, 2024

To summarize, I propose this work plan:

  1. Lower precision of stats displayed in the menu
  2. Decouple the refresh loops:
    2.1 On a long 120s interval, refresh wallet balance and scheduled rewards
    2.2 When the UI is visible, on a 30s interval, refresh wallet balance and transaction list

SGTM!

On a long 120s interval

  • The Spark round is 20 minutes long, and the scheduled rewards will be changed every 20 minutes. Depending on how much we truncate the displayed value, it will most likely change even less frequently than that.

  • Jobs counter changes every minute (depending on the delays in Spark and Voyager), but if we truncate the value to thousands, the displayed value will change less than once per day.

  • The wallet balance changes when we release rewards or the user makes a transaction. It's impossible to predict when this will happen, but it should happen very infrequently.

Given the above, I propose configuring the long interval to be 10 minutes or even 1 hour.

I would also like us to consider a few more refresh triggers:
2.3 When we open the app window, trigger an immediate refresh
2.4 When the app starts, trigger an immediate refresh

@bajtos
Copy link
Member Author

bajtos commented Mar 7, 2024

Great discussion! 😍

@juliangruber
Copy link
Member

I agree with all you said above! I'm going to add this to M4.3

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

No branches or pull requests

2 participants