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: accounts/accounts metadata mismatch #5695

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Nov 27, 2023

Description

Fixes a bug introduced in #5693 which fixed a regression introduced in #5693 but brought a smolish mismatch 🤯

tl;dr:

  • the accounts metadata now has too many accounts, including for accounts without Tx history
  • the accounts slice however, has just the right amount of accounts. This results in this guy never returning anything else than loading:

export const selectPortfolioLoadingStatus = createSelector(
selectPortfolioLoadingStatusGranular,
(portfolioLoadingStatusGranular): PortfolioLoadingStatus => {
const vals = values(portfolioLoadingStatusGranular)
if (vals.some(val => val === 'loading')) return 'loading'
if (vals.some(val => val === 'error')) return 'error'
return 'success'

This is explained by l.130 here:

export const selectPortfolioLoadingStatusGranular = createDeepEqualOutputSelector(
selectPortfolioAccountMetadata,
selectPortfolioAccounts,
(accountMetadata, accountsById): PortfolioLoadingStatusGranular => {
const requestedAccountIds = keys(accountMetadata)
return requestedAccountIds.reduce<PortfolioLoadingStatusGranular>((acc, accountId) => {
const account = accountsById[accountId]
const accountStatus = account ? (account.assetIds.length ? 'success' : 'error') : 'loading'

Given we react on a different accountsById and accountMetadata set, the account will always be undefined - as it should - for accounts without history, but the metadata will be wrongly defined, resulting in the portfolio state being set as loading.

This introduces a bunch of bugs, mostly related to the now lack of reactivity in <AppContext />: since we're always in a portfolioLoading state, we never run the effects related to e.g fetching DeFi meta/user/data.

While this bug may be invisible when clearing the cache (there will be one render where the portfolio state is indeed "success"), having existing accounts and refreshing the app will make it obvious these effects are never ran, and DeFi data isn't fetched.

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #5692

Risk

  • Accounts may be borked, test accordingly

Testing

  • Demo wallet is still happy
  • DeFi data is still displayed
  • Accounts 0+ are still properly fetched when using native/KK
  • The number of accounts for other EVM chains isn't inherited from the Ethereum account

Engineering

  • ☝🏽
  • Run a breakpoint in portfolioLoadingStatus in <AppContext /> and ensure we continue the execution after it

Operations

  • ☝🏽

Screenshots (if applicable)

Account with DeFi position

  • Prod without a cache clear
image
  • Prod after a cache clear
Screenshot 2023-11-27 at 15 19 59
  • This diff
Screenshot 2023-11-27 at 15 20 10

Accounts

  • Prod
Screenshot 2023-11-27 at 15 21 13
  • This diff
Screenshot 2023-11-27 at 15 21 26

Portfolio loading status

Screenshot 2023-11-27 at 15 22 00

Other EVM accounts do not inherit the number of Ethereum accounts

image image

Copy link
Contributor Author

gomesalexandre commented Nov 27, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@gomesalexandre gomesalexandre marked this pull request as ready for review November 27, 2023 14:41
@gomesalexandre gomesalexandre requested a review from a team as a code owner November 27, 2023 14:41
@gomesalexandre gomesalexandre enabled auto-merge (squash) November 27, 2023 17:01
@gomesalexandre gomesalexandre merged commit cefba7f into develop Nov 27, 2023
6 checks passed
@gomesalexandre gomesalexandre deleted the fix_accounts_mismatch branch November 27, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants