-
Notifications
You must be signed in to change notification settings - Fork 987
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
fix!: cache data related to token price and market data locally for GetWalletToken #21272
fix!: cache data related to token price and market data locally for GetWalletToken #21272
Conversation
Jenkins BuildsClick to see older builds (4)
|
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.
Nice work @seanstrom!
Linking directly with status-go PR status-im/status-go#5832
Pull to refresh many times quickly (roughly 5 times under 10 seconds)
In status-go we're caching for 60s, is there a reason the user can't pull to refresh less frequently and still benefit from caching? I'm just trying to understand the impact to users and I thought the effects would be positive even with just 2 pulls to refresh per 60s.
There's this Figma under construction by @xAlisher that will impact the behavior of pull to refresh and how we inform users about the freshness of balances and token prices https://www.figma.com/design/xLs1KYmF4e6WwRTZVJKeUK/Wallet?node-id=17025-536817&node-type=frame&t=WysUpXeljfC2aaUv-0. In future iterations about caching, we should try to align the technical implementation with this new UX once it's finalized and prioritized. cc @shivekkhurana
ty 🙌
Oops I forgot to link that PR, I'll add that link to the description 🙏
You're absolutely right, users will benefit for the cache for the 60s. Pulling to refresh 5 times is only necessary to show the glitch when the data is not cached.
Yeah definitely agree that we should try to explain how fresh the data is to the user, and the good news is that we timestamp the data for token-pricing and market-data inside status-go, so we could eventually forward that extra context to the status-mobile client. Although, I'm not sure if we track the timestamps for each network chain. |
96% of end-end tests have passed
Failed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (49)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
hi @seanstrom thank you for PR. No issues from my side. PR is ready to be merged |
0218f1f
to
3cf3787
Compare
I've updated the commit in this PR to update status-go to version |
96% of end-end tests have passed
Failed tests (2)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (49)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
100% of end-end tests have passed
Passed tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
@seanstrom hey. thank you for the update. The PR with updated status go works well. Can be merged. Thank you! |
3cf3787
to
da08fd4
Compare
related to: #20975
related status-go pr: status-im/status-go#5832
related status-desktop pr: status-im/status-desktop#16347
and a continuation of the "balance zero" work group
Summary
Testing notes
The main areas that should be affected is the wallet home screen, and technically some changes happened for wallet router, so it would be worth double checking that wallet transactions / estimates are still working.
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
Before Changes
Screen.Recording.2024-09-16.at.18.29.47.mov
After Changes
Screen.Recording.2024-09-16.at.18.19.57.mov
Risk
Described potential risks and worst case scenarios.
Tick one:
status: ready