Open bajtos opened 8 months ago
💯
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:
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.
💯
- 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).
- 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.
- 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>
- 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:
- 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:
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:
It feels like a lot of additional complexity, maybe it's not worth doing it right now?
- 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.
- 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.
- 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:
- 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.
- 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:
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 of0.081214 FIL
, and57k jobs
instead of57,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:
- 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 https://github.com/filecoin-station/desktop/commit/f4d2280d7468b1494cc82bd07ba02b15f463eb84, 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.
- 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:
Wdyt?
To summarize, I propose this work plan:
- Lower precision of stats displayed in the menu
- 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
Great discussion! 😍
I agree with all you said above! I'm going to add this to M4.3
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.
/cc @juliangruber based on my discussion with @jleni