-
Notifications
You must be signed in to change notification settings - Fork 985
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
feat_: optimize endpoint calls when fetching balances #21802
Conversation
Jenkins BuildsClick to see older builds (4)
|
f71268e
to
30da907
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(let [addresses (->> (get-in db [:wallet :accounts]) | ||
(vals) | ||
(keep :address) | ||
(vec))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we index the accounts by address in app-db, we can skip going through it again.
(let [addresses (->> (get-in db [:wallet :accounts]) | |
(vals) | |
(keep :address) | |
(vec))] | |
(let [addresses (-> (get-in db [:wallet :accounts]) | |
(keys))] |
:params [[address] true] | ||
:on-success [:wallet/store-wallet-token address] | ||
:on-error [:wallet/get-wallet-token-for-account-failed address]}]]]})) | ||
:params [addresses true] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion for later: if the user tries to refresh within one minute or something, can we fetch only the cached balance during that time? This would reduce the load on status-go, and RPC requests as well. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, maybe 1 minute is a lot of time depending on the situation (specially for L2s where transactions are faster), but let's define this with the design team and implement it on a separate issue.
62% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
|
100% of end-end tests have passed
Passed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
hi @briansztamfater thank you for PR. No issues from my side. PR is ready to be merged |
Signed-off-by: Brian Sztamfater <[email protected]>
30da907
to
203d416
Compare
may fix #21804
Summary
Not sure if related to this Android freeze, but when we load all accounts balances either on initial login or pull to refresh, we fetch prices and market values once per account. That may be lot of redundant data to process if the user has accounts with many tokens. This PR optimizes the corresponding re-frame events to fetch all balances for all accounts in the same endpoint call, and also fetches prices and market values once per refresh.
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready