Replace GetWalletToken endpoint with FetchOrGetCachedWalletBalances
#21625
Conversation
Jenkins BuildsClick to see older builds (38)
|
vkjr
force-pushed
the
get-wallet-token-balances
branch
2 times, most recently
from
f445964
to
c44b9bd
Compare
vkjr
changed the title
GetWalletToken endpoint with FetchOrGetCachedWalletBalancesGetWalletToken endpoint with FetchOrGetCachedWalletBalances
vkjr
requested review from
briansztamfater,
smohamedjavid,
ulisesmac,
shivekkhurana,
ilmotta,
alwx and
clauxx
| (str formatted-symbol (cut-fiat-balance-to-two-decimals balance)))) | ||
| (str formatted-symbol fiat-balance))) | ||
|
|
||
| (defn prettify-balance |
There was a problem hiding this comment.
just a small refactoring to use more idiomatic ->
| (str (cut-crypto-decimals-to-fit-usd-cents crypto-balance conversion-rate) | ||
| " " | ||
| (string/upper-case token-symbol))) | ||
| (-> crypto-balance |
There was a problem hiding this comment.
Also small refactoring
| @@ -80,165 +80,6 @@ | |||
| (rf/dispatch [:wallet/update-receiver-networks | |||
| chain-ids]))}]))}])) | |||
|
|
|||
| #_(defn- edit-amount | |||
There was a problem hiding this comment.
We will never need this since send screen is going to be simplified
There was a problem hiding this comment.
Why can't we simply remove it then?
There was a problem hiding this comment.
it is removed in this PR
| @@ -116,7 +116,7 @@ | |||
| {} | |||
| raw-data)] | |||
| {:db (-> db | |||
| (assoc-in [:wallet :tokens :market-values-per-token] market-values) | |||
| (update-in [:wallet :tokens :market-values-per-token] merge market-values) | |||
There was a problem hiding this comment.
Before we requested market values just one time so assoc-in was used, but now we do this more often with smaller subset of tokens, so need an update-in
| @@ -146,7 +146,7 @@ | |||
| :wallet.tokens/store-prices | |||
| (fn [{:keys [db]} [prices]] | |||
| {:db (-> db | |||
| (assoc-in [:wallet :tokens :prices-per-token] prices) | |||
| (update-in [:wallet :tokens :prices-per-token] merge prices) | |||
vkjr
force-pushed
the
get-wallet-token-balances
branch
from
ff364b3
to
cf8da96
Compare
| prices-per-token (rf/sub [:wallet/prices-per-token]) | ||
| balance (utils/calculate-total-token-balance token [chain-id]) | ||
| crypto-value (utils/get-standard-crypto-format token balance) | ||
| crypto-value (utils/get-standard-crypto-format token balance prices-per-token) | ||
| fiat-value (utils/calculate-token-fiat-value | ||
| {:currency currency | ||
| :balance balance | ||
| :token token}) | ||
| {:currency currency | ||
| :balance balance | ||
| :token token | ||
| :prices-per-token prices-per-token}) |
There was a problem hiding this comment.
Could we have subs for utils/get-standard-crypto-format and utils/calculate-token-fiat-value, since in a few places it seems to be directly using values from other subs?
There was a problem hiding this comment.
These are utility functions and they receive different parameters in different contexts (except for prices-per-token), so I believe they aren't very suitable for being a subscription.
But I understand what is the problem that becomes more obvious. We have a piece of information (prices-per-token) that is not needed by UI itself, but have to be passed to utility functions. So the UI here is just a middleman that operates unknown data for no reason. And we have more cases like this. For example passing :profile/currency from ui to utility functions to conversion functions.
I believe that subscription is not a best way to solve this. That data belongs to lower layer (we can think of it as of business-logic), so it should be accessed on lower layer. For example we can create a money/conversion api for ui that contains same functions but they receive less arguments. And calls then redirected to utils functions but that additional arguments like currency, prices-per-token, etc.. taken from database and UI don't know about them.
This is probably something we can discuss on DevX meeting.
There was a problem hiding this comment.
agreed, best would be to figure it out during the devx meeting
|
hi @vkjr could you please rebase and resolve existing conflicts? thanx |
62% of end-end tests have passedFailed tests (2)Click to expandClass TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
vkjr
force-pushed
the
get-wallet-token-balances
branch
2 times, most recently
from
a17528a
to
c708313
Compare
|
@VolodLytvynenko, conflicts resolved |
| token-decimals (rf/sub [:wallet/send-display-token-decimals]) | ||
|
|
|
Hi @vkjr thank you for PR. Take a look at found issue PR_ISSUE 1: The balances are not refreshed after the fiat changeSteps:
Actual result:The balances are not updated. (see on screen. 26 USDT estimated as 26 RUB) Expected result:Fiat is converted to the selected one OS:IOS, Android Devices:
|
|
@vkjr - we need to update the following event as well. Fetch only the prices and market values on currency change. status-mobile/src/status_im/contexts/profile/settings/events.cljs Lines 146 to 149 in a1e3bb3 |
|
@VolodLytvynenko, thanks for finding, will fix it. |
|
@smohamedjavid, thanks! I believe this should already work because |
@vkjr - I meant we call the With these PR changes, we can fetch only token prices and market values for the selected currency and USD instead of the whole token data because if the user casually changes the currency multiple times, it will request the whole token data for all those times (pretty heavy for mobile). |
|
@smohamedjavid, ah, now I see your point, thanks! |
vkjr
force-pushed
the
get-wallet-token-balances
branch
from
be811d9
to
0721455
Compare
|
@VolodLytvynenko, fixed the issue, could you please check? Thanks! |
75% of end-end tests have passedFailed tests (1)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
|
| (reduce concat) | ||
| (map :symbol) | ||
| set | ||
| vec)] |
There was a problem hiding this comment.
Since there's no guarantee of order in keys from tokens, the resulting vector order of symbols is also unknown. In this case, can't we skip the final vec conversion and persist a set? Seems more correct to me since we don't want duplicates, so why use the vector type?
This implementation is considerably faster if what I said is correct and we can store a set:
(reduce-kv (fn [acc _ v]
(into acc (map :symbol v)))
#{}
tokens)| :tokens (:tokens %) | ||
| :chain-ids chain-ids}) | ||
| (map #(utils/calculate-balance-from-tokens {:currency currency | ||
| :tokens (:tokens %) |
There was a problem hiding this comment.
(Personal take) Multiline functions with % tend to give poor readability. The characters savings don't compensate in most multiline functions.
| (map #(utils/calculate-balance-from-tokens {:currency currency | ||
| :tokens (:tokens %) | ||
| :chain-ids chain-ids}) | ||
| (map #(utils/calculate-balance-from-tokens {:currency currency |
There was a problem hiding this comment.
[Not a suggestion for change in this PR] This is a good example where we are pulling too much in subs and the code is inefficient.
To compute this sub we only need the addresses and tokens of accounts. The result of :wallet/accounts sub contains lots of unrelated data, including timestamp and clock, things that can change more often and which would invalidate the cache. The better thing to do is to make the inputs be 2 subs, the accounts addresses and accounts tokens, not the whole :wallet/accounts.
This :wallet/accounts sub reminds me of the :communities/community sub, which if we subscribe to it will be guaranteed inefficient because it can change for too many reasons (thus changes more frequently and invalidates caches and causes re-processing by Reagent more easily/frequently).
status-mobile/src/status_im/subs/communities.cljs
Lines 21 to 28 in c6c3e08
| (map :symbol) | ||
| (into acc))) | ||
| #{}) | ||
| vec)) |
There was a problem hiding this comment.
Is there a reason the unordered sequence of unique symbols must be a vector and not a set?
There was a problem hiding this comment.
Thank you for spotting!
Not some well-thought reason. I replicated the type of parameter that was passed to :effects.wallet.tokens/fetch-market-values before. But that probably is not necessary. Will check and fix it.
|
Hi @vkjr Hi @vkjr, unfortunately, the issue is still reproducible. However, it only occurs after re-login or when the app returns from the background. PR_ISSUE 1: Balances shown in USD after fiat currency change and relogin or backgroundSteps:
Actual result:Balances are displayed in USD instead of the selected fiat currency. usd.mp4Expected result:Fiat is converted to the selected one OS:IOS, Android Devices:
Logs: |
fixes #20473
Summary
GetWalletTokenendpoint instatus-gowas replaced byFetchOrGetCachedWalletBalancesVisible impact - when user drags down wallet screen and triggers refresh, it happens much quicker - fraction of second instead of few seconds.
Review notes
Deprecated
GetWalletTokenfunction was packing a big chunk of information together - not only token balances but also their prices and market values. That resulted in data duplication - inside tokens we had the same information about prices and market values as we already have in re-frame db under[:wallet :tokens :prices-per-token]and[:wallet :tokens :market-values-per-token]. Moreover, in time that information became out of sync, because data under[:wallet :tokens ...]never updated after wallet initialisation while data inside token structure updated regularly.This PR replaces the endpoint and removes the data duplication.
Token structures fetched with
FetchOrGetCachedWalletBalancesdo not have information about prices and market values. But this information can be found under[:wallet :tokens :prices-per-token]and[:wallet :tokens :market-values-per-token], so big amount of small changes is a result of necessity to pass another structures with prices and market values apart from token itself.When
FetchOrGetCachedWalletBalancesfinishes successfully we now additionally request prices and market values for those tokens that exist in wallet to have an up-to-date data after user triggered refresh.Testing notes
From user perspective this PR should't change anything except the speed of wallet refresh when user drags screen down. It is faster now.
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready