refactor: simplify refresh balance triggers #6232
Conversation
| createFiatConnectTransferCompleted.type, | ||
| depositTransactionSucceeded.type, | ||
| swapSuccess.type, | ||
| ], |
There was a problem hiding this comment.
this is the main change
There was a problem hiding this comment.
does it belong to home? what do you think about moving it to tokens and spawning it in the root saga instead?
There was a problem hiding this comment.
yeah i had the same thought but i am not sure where is best for this. i also don't think tokens is the best place, because the balance refresh also involves refreshing local currency + positions. home kind of makes sense if we think of it being a middle ground between everything?
There was a problem hiding this comment.
yes, tokens is not the best place, but home doesn't feel right either for the same reason, because this saga updates the other screens too. i feel like it should be a separate one spawned from the root because of its "general" nature. don't have any good naming idea though 😅. so i agree that home is a reasonable place for now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6232 +/- ##
==========================================
- Coverage 88.98% 88.93% -0.05%
==========================================
Files 737 737
Lines 31449 31444 -5
Branches 5828 5521 -307
==========================================
- Hits 27984 27966 -18
- Misses 3267 3433 +166
+ Partials 198 45 -153
... and 70 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| export function* localCurrencySaga() { | ||
| yield* spawn(watchFetchCurrentRate) | ||
| yield* spawn(watchSelectPreferredCurrency) | ||
| yield* takeLatest([Actions.SELECT_PREFERRED_CURRENCY], safely(fetchLocalCurrencyRateSaga)) |
There was a problem hiding this comment.
supernit: keeping the watch- prefixed saga seems easier to read to me
There was a problem hiding this comment.
hum, but that means from the root saga we spawn this localCurrencySaga and all it does is spawn another function that executes this line? 🙈 i'm not sure i understand why the extra layer of abstraction increases readability, is it because we use the watch* pattern sometimes? we've not been so consistent with that pattern though
There was a problem hiding this comment.
yeah, i noticed we use watch* pattern quite frequently and it seemed to me as a standard "building block". the abstraction could help to skim right away what this saga is doing: oh, it spawns a "watcher", that's it.
There was a problem hiding this comment.
okay i put the watcher function back. i don't see a lot of value in the particular pattern 🙈 but i do see value in the consistency
Description
This PR consolidates the way we are refreshing the token balances. I think we let this evolve organically for some time and it has become buggy and inconsistent.
Currently, before this change:
fetchTokenBalancesaction after every flow that initiates a transaction. i think it is probably unavoidable to need to remember to trigger a balance fetch, but currently we dispatch a success action for the flow and a separate action for the balances (the dreaded double dispatch)This change:
fetchTokenBalancesaction entirely and instead, trigger the balance refresh sagas on specific actions. i've tried to document them all.homereducer, most flows were never allowing this loading state to be set. only the send flow was, i think that's a bug. balance refreshes should be a background task and not affect whether we show or hide the balance....Test plan
I should see a network request to refresh token balance, exchange rate, positions, shortcuts when I have:
Related issues
Backwards compatibility
yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: