Skip to content
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 account balance discrepancies #5959

Merged
merged 41 commits into from
Aug 14, 2024
Merged

Fix account balance discrepancies #5959

merged 41 commits into from
Aug 14, 2024

Conversation

walmat
Copy link
Contributor

@walmat walmat commented Jul 26, 2024

Fixes APP-1689
Fixes APP-1564

What changed (plus any additional context for devs)

We had a couple issues going on here.

  1. In the change wallet sheet we were using the addys account summary API to display balances, this was somehow really off.
  2. On the wallet screen, we were filtering assets a little too heavily so the balance was different between BX and App.
  3. In the send sheet I am now consuming the new method as well to keep it consistent

In order to solve both, we now consume a shared react-query hook in both places, which is the same one the BX uses.

Screen recordings / screenshots

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-05.at.16.49.58.mp4

What to test

Copy link

linear bot commented Jul 26, 2024

Copy link

linear bot commented Aug 1, 2024

@jinchung jinchung requested review from benisgold and jinchung and removed request for benisgold August 2, 2024 22:25
src/languages/en_US.json Outdated Show resolved Hide resolved
}),
},
]);
queryClient.invalidateQueries(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

separated all of the instances of invalidateQueries out because Arrays weren't working for whatever reason.

connectedToHardhat,
}),
});
queryClient.invalidateQueries(nftsQueryKey({ address: accountAddress }));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here. Arrays were just not working..

src/hooks/useWallets.ts Outdated Show resolved Hide resolved
@benisgold benisgold requested a review from jinchung August 9, 2024 23:40
src/hooks/useWallets.ts Outdated Show resolved Hide resolved
@jinchung
Copy link
Member

in the ControlPanel, AddressRow, and ContactRow, we have either the balance or show "No balance" - wondering about the loading state here as for now it'll just show "No balance" which can be jarring; at some point Matthew had something like a "Loading balance" copy placeholder, wonder if we want to use something like that

})),
});

const isLoading = isSummaryLoading || positionQueries.some(query => query.isLoading);
Copy link
Member

Choose a reason for hiding this comment

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

just want to check:
for the useAddysSummary hook, isLoading will only be set to true at the initial load but will be false afterwards since we have keepPreviousData set to true which is great.

can we double check that we have something similar for the position queries, and that we aren't going back to isLoading set to true every time we're fetching after the initial fetch? we should check what we have configured for the positions data

otherwise would expect to result in something like profile balance showing up and then the row disappearing for a bit during a positions refresh, and coming back, etc. also while we're at it, generally check that the data is being refreshed in a reasonable manner now that we rely on this for balances instead of the underlying token data

Copy link
Member

@benisgold benisgold Aug 13, 2024

Choose a reason for hiding this comment

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

keepPreviousData is actually not used correctly there. react query actually "keeps previous data" all the time - even if data is stale, react query still provides it until the new data (fetched in the background) is ready to replace it. keepPreviousData keeps the previously fetched data when the query key changes, which is not behavior we want. i'm removing that.

but you're right that the useWalletBalances version of isLoading is somewhat problematic, for 2 reasons:

  1. the addys query has a cache time of 24 hours while the positions queries do not have cache times
  2. even if the cache times were the same may not be aligned, especially since these queries could be used in other contexts

this means that the problem you described would currently be valid every time a user cold starts the app since there is no positions query cache.

to mitigate it i've given all the queries in useWalletBalances a staleTime of 2 minutes and a cacheTime of 24 hours. this means worst case scenario isLoading will be true a few times a day, but in reality it will probably be fewer.

Copy link
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

🌮

@benisgold benisgold merged commit e5ab360 into develop Aug 14, 2024
6 checks passed
@benisgold benisgold deleted the @matthew/APP-1689 branch August 14, 2024 18:32
brunobar79 added a commit that referenced this pull request Aug 20, 2024
* Degen mode analytics, copy adjustment (#5979)

* Brody/swap v2 e2e (#5915)

* init

* test file

* clean up

* matthew review fixes

* lint

* oops!!!

* type casting

* rename

* .

* lint

* oops hardhat stuff removed

* reverts

* simplify hardhat check

* Update src/__swaps__/screens/Swap/components/FadeMask.tsx

Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>

* Apply suggestions from code review

IS_TESTING > IS_TEST

Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>

* fix lint

* okay actually fix lint

---------

Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>

* Revert CI script changes (#5961)

* init

* oop

* update logging

* test

* undo

* Revert "rerun only failed tests (#5878)"

This reverts commit 497d27a.

* Bump fast-xml-parser from 4.4.0 to 4.4.1 (#5965)

Bumps [fast-xml-parser](https://github.com/NaturalIntelligence/fast-xml-parser) from 4.4.0 to 4.4.1.
- [Release notes](https://github.com/NaturalIntelligence/fast-xml-parser/releases)
- [Changelog](https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/CHANGELOG.md)
- [Commits](NaturalIntelligence/fast-xml-parser@v4.4.0...v4.4.1)

---
updated-dependencies:
- dependency-name: fast-xml-parser
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: i18n updates (#5980)

* Revert "Brody/swap v2 e2e (#5915)" (#5987)

This reverts commit a38f633.

* Convert App.js => App.tsx (#5792)

* .

* undo tsconfig change and create routes.tsx with platform selector

* chore: merge

* yep

* fix

* rm authorize

* change initial route context initial value

* wrap in usecallbacks

* Fix token to buy list empty spaces (#5983)

* fixes empty spaces

* also add searching to recents

* rm log

* remove duplicate recents and add migration

* fix sort

* fix bridge button sending users to swaps v1 (#5971)

* fix (#5989)

* fix favorites tokens with no icon (#5982)

* fix favorites default list

* wip

* 👍

* Update src/resources/favorites.ts

* fix format

---------

Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>

* codeowners file removed (#5991)

* Trending DApps (#5974)

* initial work for trending dapps

* progress on trending dapps

* impl. trending hooks

* combine selectors

* fix crash and fix empty state not showing up (#5975)

* bump version to v1.9.35 (#5992)

* Swaps: Popular Tokens (#5990)

* Default the Swap input currency to ETH  (#5994)

* . (#5993)

* Design system improvements (#5984)

* Revert ". (#5993)" (#5999)

* swaps bug fix: missing asset balance for some tokens (#5998)

* remove filter

* forgot this

* Drag and drop components, browser favorites reordering (#5978)

* Add drag-and-drop components, useLayoutWorklet

Slightly modified implementation based on https://github.com/mgcrea/react-native-dnd

* Make browser favorites reorderable, migrate favoriteDapps store to createRainbowStore

* Fix fallback logos, add useLayoutWorklet documentation

* Reorder imports

* Clean up favorites stores, types

Removes unnecessary additions to the legacy store

* Improve reliability of browser favorites URL handling

Resolves issues causing certain favorites not to be removable (due to params in the URL or a subtly different URL path), by:

- Adding more lenient fallback lookup mechanisms if no exact match is found
- Trimming params from the standardized URLs

Also moves the browser favorites store to within the /state/browser/ folder.

* Create EasingGradient, useEasingGradient

* Add activeScale, default activeOpacity to 1

* Clean up activeScale

* Replace JS-based timer with useAnimatedTimeout, misc. cleanup

* Add dragDirection option to Draggable

* Add DraggableFlatList

Fully working with the exception of auto scrolling while dragging

* Improve browser homepage cards

* Fix light mode card contrast, Android card gradient

* Expose stop function in useAnimatedTimeout

* useAnimatedTime: stop → clearTimeout

* Improve card BlurView rendering

* Make reorderable favorites mounting/unmounting smoother

* Clean up animated reaction dependencies

* Add missing favorites store method

* Fix account balance discrepancies (#5959)

* fix balance discrepency between bx and app and app and change wallet sheet

* Update src/helpers/buildWalletSections.tsx

* fix missing import from commit

* also fix send sheet balance discrepancy

* fix swap user assets not refetching on pending txn resolution and PTR

* .

* update en_US key

* rmeove unused hook and bake it into the user assets store

* good lord

* separate invalidateQueries because they don't update when grouped for some reason

* more instances

* cleanup

* cleanup useWatchPendingTxs

* code suggestions

* break up useWallets

* rm latestBackup from useWallets

* useMemo -> useEffect

* logging

* cleanup

* remove unused withPositions params

* type

* fix PROFILE_STICKY_HEADER loading value

* fix

* optional chaining

* fixes to buildWalletSections + related code

* fix accountInfo isLoadingAccountBalance

* clean up useWalletBalances

* stuff is still broken but committing walletsWithBalancesAndNames fix

* refactor useWalletsWithBalancesAndNames and fix consumers

* fix wallet screen balance loading state

* console.log

* switch amount types from string | number -> string

* align stale/cache times, remove keepPreviousData

* remove spammy isDamaged logging

* add loading balance copy + remove incorrectly used no balance copy

* fix i18n mistake

---------

Co-authored-by: Ben Goldberg <bengoldberg@rainbow.me>

* update swaps sdk (#5996)

* Swaps: Change fee denomination from USD -> actual payment token (#6000)

* hm

* done

* idk how this got deleted

* cleanup

* fix bad rebase

---------

Co-authored-by: gregs <ukfaeku@gmail.com>

* Fun stuff (#5995)

* add filteration

* clenaup

* fix runtime error (#6001)

* rotate user-agent (#6003)

* fixed dapp browser height on android (#6004)

* fixes android light mode button navigation colors (#6005)

* fix TokenToBuyList weirdness when favoriting (#6002)

* wip

* fix

* ops

* bring back slice

* Revert "fixes android light mode button navigation colors (#6005)"

This reverts commit fc87001.

* Revert "Convert App.js => App.tsx (#5792)"

This reverts commit 0d09b10.

* Revert "Drag and drop components, browser favorites reordering (#5978)"

This reverts commit 5feae69.

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christian Baroni <7061887+christianbaroni@users.noreply.github.com>
Co-authored-by: brdy <41711440+BrodyHughes@users.noreply.github.com>
Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Daniel Sinclair <4412473+DanielSinclair@users.noreply.github.com>
Co-authored-by: gregs <ukfaeku@gmail.com>
Co-authored-by: Bruno Barbieri <1247834+brunobar79@users.noreply.github.com>
Co-authored-by: Ben Goldberg <bengoldberg@rainbow.me>
Co-authored-by: Bruno Barbieri <brunobar79@gmail.com>
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