Skip to content

Conversation

@piyalbasu
Copy link
Contributor

@piyalbasu piyalbasu commented Sep 16, 2025

Closes #2248

  1. We no longer make fetching a user's balance a prerequisite for loading the Account view. We fetch it once on app load, and then rely on the cache for subsequent Account view loads. For example, if you open the app, load the Account view, navigate to Settings, and then navigate back to Account, we will only have loaded account balances from the BE once. While not wholly impressive at the moment (this really only saves me ~750ms on subsequent loads of the Account view), this sets a pattern for making some of these once synchronous calls more asynchronous. In the following PR's, I'll be moving other requests to this pattern and, cumulatively, these will add up to real gainz.

  2. To help ensure users don't get stuck with stale information, when a user adds/removes a trustline or sends/swaps, we will opportunistically fetch account balances so when they return to the Account view, the user will see their latest action reflected accurately

  3. Also, when a user changes accounts or changes networks, I am forcing a re-fetch of balances. My hypothesis is that users are more willing to accept a slightly longer load time for changing an account and ensuring they have up-to-date data since this is likely they're not doing too frequently

@piyalbasu
Copy link
Contributor Author

piyalbasu commented Sep 16, 2025

Polling account balances every 30s for changes that happen outside of the app:

Screen.Recording.2025-09-16.at.4.59.48.PM.mov

@sdfcharles I'm following the precedent set by token prices and not animating when there's a delta. Let's talk more about this if we'd like to reintroduce the idea of animating changes.

Also, I think it'd be a good idea to add a "refresh" button/hotkey so user's can short circuit the 30s wait and choose to re-fetch balances early. Wdyt?

@piyalbasu
Copy link
Contributor Author

When taking actions that will affect the user's account balance, we'll re-fetch the balances for the user before returning them to the Account view

Screen.Recording.2025-09-16.at.5.04.48.PM.mov

} from "./helpers/stubs";

const MUXED_ACCOUNT_ADDRESS =
"MCCRNOIMODZT7HVQA3NM46YMLOBAXMSMHK3XXIN62CTWIPPP3J2E4AAAAAAAAAAAAEIAM";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally funded the muxed test acct, so creating a new one here

state.balanceData = {
...state.balanceData,
[action.payload.publicKey]: action.payload.balances,
[action.payload.networkDetails.network]: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

balances should be organized by network so we show the correct balance when a user switches network

@piyalbasu piyalbasu marked this pull request as draft September 16, 2025 21:36
reloadBalances={() =>
fetchData({
useAppDataCache: true,
shouldForceBalancesRefresh: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funding with Friendbot should force a refresh

@piyalbasu piyalbasu marked this pull request as ready for review September 17, 2025 18:05
const allowList = appData.settings.allowList;
const isMainnetNetwork = isMainnet(networkDetails);

const hasBalanceCache =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like there is a similar check to this inside of fetchBalances also, do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch - yup, removed here: f564c2f

@aristidesstaffieri
Copy link
Contributor

I'm not sure if this would be considered in scope for this PR but I think if you checked the icon cache here instead of the token list cache you can avoid fetching the tomls on subsequent navs as well.

isSelected={isSelected}
onClick={async (publicKey) => {
await dispatch(makeAccountActive(publicKey));
await fetchBalances(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this call here creates a lag or a frozen feeling because there is no visible loading state and nothing happens until after this promise resolves.

Is there a way to push this responsibility to the Account view or to represent the loading state?

Copy link
Contributor Author

@piyalbasu piyalbasu Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout. I tweaked this in the next commit so Account will take on the responsibility of fetching the balances and showing a loader

f564c2f

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice workaround!

@piyalbasu
Copy link
Contributor Author

I'm not sure if this would be considered in scope for this PR but I think if you checked the icon cache here instead of the token list cache you can avoid fetching the tomls on subsequent navs as well.

Yup - that's on my list of things to do. I haven't created tix for all of the issues yet, but I'm tackling basically everything in this doc: https://docs.google.com/document/d/1c-EF_kbXAInmmv5V5H1aYo4kT7ZhB5Fw7P7dlrfU4y8/edit?tab=t.0

@piyalbasu piyalbasu merged commit 517474b into release/5.35.1 Sep 22, 2025
4 of 5 checks passed
@piyalbasu piyalbasu deleted the feature/poll-and-cache-account-balances branch September 22, 2025 18:43
piyalbasu added a commit that referenced this pull request Oct 3, 2025
* upgrade to yarn 2 and use resolutions to block vulnerable package versions (#2239)

* upgrade to yarn 2 and use resolutions to block vulnerable package versions

* rm deprecated .yarnrc

* rm yarnpath

* try committing yarn binary to repo

* try corepack enable for gha

* update run tests cmd

* rm yarnpath

* rm npm i yarn

* update all pipelines

* rm superfluous history types

* ensure invoke host function tx shows contract parameters (#2243)

* ensure invoke host function tx shows contract parameters

* add test for fallback if contract spec retrieval fails

* Bugfix/rm auth param names (#2244)

* ensure invoke host function tx shows contract parameters

* add test for fallback if contract spec retrieval fails

* do not show contract parameters for authorizations

* add tests for create contract v1 and invoke contract

* add issuer for changeTrust op (#2246)

* add issuer for changeTrust op

* programmatically disable overflow:hidden when copying a value

* Revert "add issuer for changeTrust op (#2246)" (#2247)

This reverts commit 19c8a68.

* Bugfix/add issuer for changetrust (#2249)

* ensure invoke host function tx shows contract parameters

* add test for fallback if contract spec retrieval fails

* do not show contract parameters for authorizations

* add tests for create contract v1 and invoke contract

* add issuer for changeTrust op

* programmatically disable overflow:hidden when copying a value

* cache account balances and poll for updates (#2252)

* cache account balances and poll for updates

* fix CI tests

* rm `force:true` which was causing action to happen too fast

* do a fresh balance fetch on account/network change

* pr comments

* add more sentry tracking for Account and Wallets views (#2268)

* add more sentry tracking for Account and Wallets views

* adding more sentry reporting

* gracefully degrade on errors from Blockaid (#2269)

* gracefully degrade on errors from Blockaid

* should not be necessary to skip dapp scanning on custom network

* rm extra dep

* add a test for persisting configurations in the send flow (#2271)

* add a test for persisting configurations in the send flow

* rm logs and update muxed acct; lower xlm payment

* handle missing scan-tx result; add disabled state for Confirm Anyway (#2272)

* handle missing scan-tx result; add disabled state for Confirm Anyway

* assertions to show correct confirm button on Blockaid error

* add cache for balances to ensure we do a fresh lookup when needed (#2275)

* add cache for balances to ensure we do a fresh lookup when needed

* add try...catch to token-prices polling

* rm log

* only dispatch saveBalancesForAccount when fresh data has been fetched

* adjust test to wait for UI change

* replace yarn setup with just yarn

* after adding/removing asset: update status (#2285)

* Bugfix/update custom token balance (#2286)

* after adding/removing asset: update status

* update token balances after adding/removing token

* update comment

* test for showing e2e token in account balances view

* remove event properties from blockaid metric event properties (#2288)

* Bugfix/fix avail balance (#2291)

* better logic for preventing negative numbers in available balance

* simplify minimum balance calc by using BE's value

* update mock data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants