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 1 min timeout for fetching new token info #16226

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Jan 7, 2025

Description

  • Now the newly received token data is being fetched instantly, instead of waiting for the 1 min
    timeout

  • Token added through "Add token" also fetches last week rates to display the change

Related Issue

Resolve #15493

Screenshots:

Receive new token Before:

Screen.Recording.2025-01-08.at.13.35.44.mov

Receive new token After:

Screen.Recording.2025-01-08.at.13.15.15.mov

Add token Before:

Screen.Recording.2025-01-08.at.13.28.37.mov

Add token After:

Screen.Recording.2025-01-08.at.13.18.27.mov

Copy link

github-actions bot commented Jan 7, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch from f782ee7 to 31356eb Compare January 8, 2025 12:24
@enjojoy enjojoy marked this pull request as ready for review January 8, 2025 12:44
@enjojoy enjojoy requested a review from tomasklim January 8, 2025 12:44
@enjojoy enjojoy changed the title fix(suite): check for new tokens in the account Fix 1 min timeout for fetching token info Jan 8, 2025
@enjojoy enjojoy changed the title Fix 1 min timeout for fetching token info Fix 1 min timeout for fetching new token info Jan 8, 2025
@enjojoy enjojoy requested a review from izmy January 8, 2025 12:45
@tomasklim tomasklim requested a review from izmy January 9, 2025 10:59
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

Tested again and it works, weird.

However, you do not need to fix add token, it will start working automatically by your fix

@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch 2 times, most recently from be0ed00 to 282ebc2 Compare January 9, 2025 14:33
};

const areDifferentTokens = () => {
return JSON.stringify(account.tokens) !== JSON.stringify(freshInfo.tokens);
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON operation are very expensive and this function could run very often and I think tokens could be really huge if account has lot of tokens. I am pretty sure it will have negative perf impact on mobile for bigger accounts.

What about using just account.tokens.length !== freshInfo.tokens.length ?

Copy link
Member

Choose a reason for hiding this comment

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

it's possible for networks using blockbook, not for non-blockbook

Copy link
Contributor

Choose a reason for hiding this comment

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

How could token disappear from account? Anyway as I said I would prefer something more efficient like this (credit to chatGPT):

function areTheyDifferent(
  oldArr: TokenInfo[],
  freshArr: TokenInfo[]
): boolean {
  // Quick check: if lengths differ, they definitely have different items
  if (oldArr.length !== freshArr.length) {
    return true;
  }

  const oldIds = new Set(oldArr.map(item => item.contract));
  const freshIds = new Set(freshArr.map(item => item.contract));

  // If the sizes differ, there's a new/missing contractId
  if (oldIds.size !== freshIds.size) {
    return true;
  }

  // Check if every ID in old exists in fresh (and vice versa, since sizes are the same)
  for (const id of oldIds) {
    if (!freshIds.has(id)) {
      return true;
    }
  }

  // If we get here, the sets of contractIds are the same
  return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

You swap one token for another.

1 token dissapear and the other appear. Length is same

Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolving this as my comment about performance of this solution still stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I remember about it, will get to it soon   

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we need to do any changes in this file at all

@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch from 6d3a6d0 to 398e095 Compare January 13, 2025 09:56
@tomasklim tomasklim self-requested a review January 13, 2025 10:11
@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch from 398e095 to a861bcf Compare January 13, 2025 11:41
JSON.stringify(freshInfo?.misc?.stakingPools) !==
JSON.stringify(account?.misc?.stakingPools)
JSON.stringify(account?.misc?.stakingPools) ||
areDifferentTokens()
Copy link
Member

@tomasklim tomasklim Jan 13, 2025

Choose a reason for hiding this comment

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

total amount of txs changes when you receive tokens so no changes here are needed. The tokens appeared before, the only problem was update of fiat rates

@@ -744,11 +753,14 @@ export const isAccountOutdated = (account: Account, freshInfo: AccountInfo) => {
// changed rewards amount (rewards are distributed every epoch (5 days))
freshInfo.misc!.staking?.rewards !== account.misc.staking.rewards ||
// changed stake pool
freshInfo.misc!.staking?.poolId !== account.misc.staking.poolId
freshInfo.misc!.staking?.poolId !== account.misc.staking.poolId ||
areDifferentTokens()
Copy link
Member

@tomasklim tomasklim Jan 13, 2025

Choose a reason for hiding this comment

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

Cardano works same as blockbook so probable same as for Ethereum -> total amount of txs changes when you receive tokens so no changes here are needed

);
case 'solana':
// compare last transaction signature since the total number of txs may not be fetched fully
return freshInfo.history.txids?.[0] !== account.history.txids?.[0];
return (
freshInfo.history.txids?.[0] !== account.history.txids?.[0] || areDifferentTokens()
Copy link
Member

@tomasklim tomasklim Jan 13, 2025

Choose a reason for hiding this comment

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

Solana works different but when something in the account changes, there has to be new tx (incoming or outcoming) so no changes here are needed

@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch from a861bcf to 3ddc351 Compare January 14, 2025 08:58
@enjojoy enjojoy force-pushed the fix/long-new-token-fetch branch from 3ddc351 to d48eda4 Compare January 14, 2025 09:33
@tomasklim tomasklim merged commit 360b22b into develop Jan 14, 2025
35 checks passed
@tomasklim tomasklim deleted the fix/long-new-token-fetch branch January 14, 2025 12:43
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.

It takes up to 1 minute to fetch rates for newly received token
4 participants