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

fetchErc20Decimals should not use getTokenStandardAndDetails #24374

Closed
dbrans opened this issue May 3, 2024 · 3 comments · Fixed by #27088
Closed

fetchErc20Decimals should not use getTokenStandardAndDetails #24374

dbrans opened this issue May 3, 2024 · 3 comments · Fixed by #27088
Assignees
Labels
INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-confirmations Push issues to confirmations team type-enhancement type-refactor

Comments

@dbrans
Copy link
Contributor

dbrans commented May 3, 2024

We are making 3-4 additional RPC requests for each ERC20 token during simulation renders. Given that 90% of transactions involve only a single token, and many of these are not ERC20 tokens, this extra load is generally manageable. However, for the 10% of transactions involving multiple ERC20 tokens, this can quickly add up.

For example, transactions with 10 or more tokens could result in 20-30 additional requests, which might impact performance and rate limits.

@bergeron wrote:

I do suspect getTokenStandardAndDetails is used in some places it shouldn't. It should only be used when we have an address and no idea what kind of token it is. Because it has to cycle through all the standards. For example this code just wants ERC20 decimals:

async function fetchErc20Decimals(address: Hex): Promise<number> {
  try {
    const { decimals } = await getTokenStandardAndDetails(address);
    return decimals ? parseInt(decimals, 10) : ERC20_DEFAULT_DECIMALS;
  } catch {
    return ERC20_DEFAULT_DECIMALS;
  }
}

Which should be 1 RPC request since we already 'know' its an ERC20, but that could do up to 4 RPC requests (checking for NFTs and returning other ERC20 details other than decimals)

@dbrans dbrans added the team-confirmations Push issues to confirmations team label May 3, 2024
@metamaskbot metamaskbot added the INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. label May 3, 2024
@jpuri jpuri assigned jpuri and unassigned jpuri Jun 12, 2024
@bschorchit
Copy link

Data insight: transaction with 10+ tokens are represent less than 0.1%

@digiwand
Copy link
Contributor

moved from epic Simulations to epic 2024 Q3 - Confirmation Performance as the issue involves additional instances outside of simulations

@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 19, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Sep 29, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-12.5.0 on issue. Adding release label release-12.5.0 on issue, as issue is linked to PR #27088 which has this release label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-confirmations Push issues to confirmations team type-enhancement type-refactor
Projects
None yet
5 participants