-
Notifications
You must be signed in to change notification settings - Fork 13
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
Comments
💯 I was thinking about making this dependent on the UI being shown too.
For reference, we're consuming two different services:
💯
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).
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.
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 <onVisible>
<updateWallet />
</onVisible> |
Oh, that's a bummer. It means we have two options:
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.
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. 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?
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.
Sounds great. Maybe we can move the
Nice! I am not that much familiar with React, but I am happy to follow your lead here. |
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.
+1
That's a really good idea 👏
I don't perceive this as a lot of additional complexity. The tasks are:
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.
To summarize, I propose this work plan:
Wdyt? |
SGTM!
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: |
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
The text was updated successfully, but these errors were encountered: