Skip to content

Commit

Permalink
perf: use global token list in hook (#25501)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

The `useIsOriginalTokenSymbol` is meant to check a given token's symbol
– one that can be overridden by the user – against the true token
symbol. We currently fetch this token symbol using the
`AssetsContractController`'s `getTokenSymbol` function which calls its
sister `getTokenStandardAndDetails` function, which uses the network's
provider (e.g., Infura) to query that token's on-chain metadata.

Because this hook is rendered (i.e. the blockchain/node is queried) once
for each token on each render, users can get rate-limited relatively
quickly; this is much more likely for users that import a large amount
of tokens or spend a long time in the extension.

The solution is to prefer the token list saved in the global state over
the API.

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25501?quickstart=1)

## **Related issues**

Fixes: #24859 

## **Manual testing steps**

1. Go to the home page
2. Open the "networks" tab in dev tools; add a filter for that network's
provider (e.g., "infura" for Mainnet, assuming default network settings)
3. Switch back and forth between the NFTs/Activity tab and the Tokens
tab
4. Ensure we aren't violating the RPC provider API with requests every
time the token tab loads

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->


https://github.com/MetaMask/metamask-extension/assets/44588480/5b2c797d-9c77-437a-b50f-dc8126177b32


### **After**

<!-- [screenshots/recordings] -->


https://github.com/MetaMask/metamask-extension/assets/44588480/9dc3cffd-87a6-43c4-bc79-a4116095626d


## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
BZahory authored Jun 25, 2024
1 parent 7add8ac commit 2cd5813
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 7 deletions.
31 changes: 29 additions & 2 deletions ui/hooks/useIsOriginalTokenSymbol.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,44 @@
// TODO: reconsider this approach altogether
// checking against on-chain data to see if a user has changed a token symbol is not ideal
// we should just keep track of the original symbol in state, or better yet, rely on the address instead of the symbol
// see: https://github.com/MetaMask/metamask-extension/pull/21610 (original PR)

import { useEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import { getTokenSymbol } from '../store/actions';
import { getTokenList } from '../selectors';

/**
* This hook determines whether a token uses the original symbol based on data not influenced by the user.
*
* @param {string} tokenAddress - the address of the token
* @param {string} tokenSymbol - the local symbol of the token
* @returns a boolean indicating whether the token uses the original symbol
*/
export function useIsOriginalTokenSymbol(tokenAddress, tokenSymbol) {
const [isOriginalNativeSymbol, setIsOriginalNativeSymbol] = useState(null);

const tokens = useSelector(getTokenList);

useEffect(() => {
async function getTokenSymbolForToken(address) {
const symbol = await getTokenSymbol(address);
// attempt to fetch from cache first
let trueSymbol = tokens[address?.toLowerCase()]?.symbol;

// if tokens aren't available, fetch from the blockchain
if (!trueSymbol) {
trueSymbol = await getTokenSymbol(address);
}

// if the symbol is the same as the tokenSymbol, it's the original
setIsOriginalNativeSymbol(
symbol?.toLowerCase() === tokenSymbol?.toLowerCase(),
trueSymbol?.toLowerCase() === tokenSymbol?.toLowerCase(),
);
}

getTokenSymbolForToken(tokenAddress);
// no need to wait for tokens to load, since we'd fetch without them if they aren't available
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [tokenAddress, tokenSymbol]);

return isOriginalNativeSymbol;
Expand Down
45 changes: 40 additions & 5 deletions ui/hooks/useIsOriginalTokenSymbol.test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,26 @@
import { renderHook, act } from '@testing-library/react-hooks';
import { act } from '@testing-library/react-hooks';
import * as actions from '../store/actions';
import mockState from '../../test/data/mock-state.json';

import { renderHookWithProvider } from '../../test/lib/render-helpers';
import { useIsOriginalTokenSymbol } from './useIsOriginalTokenSymbol';

// Mocking the getTokenSymbol function
jest.mock('../store/actions', () => ({
getTokenSymbol: jest.fn(),
}));

const state = {
metamask: {
...mockState.metamask,
tokenList: {
'0x1234': {
symbol: 'ABCD',
},
},
},
};

describe('useIsOriginalTokenSymbol', () => {
it('useIsOriginalTokenSymbol returns correct value when token symbol matches', async () => {
const tokenAddress = '0x123';
Expand All @@ -17,8 +31,9 @@ describe('useIsOriginalTokenSymbol', () => {
let result;

await act(async () => {
result = renderHook(() =>
useIsOriginalTokenSymbol(tokenAddress, tokenSymbol),
result = renderHookWithProvider(
() => useIsOriginalTokenSymbol(tokenAddress, tokenSymbol),
state,
);
});

Expand All @@ -35,12 +50,32 @@ describe('useIsOriginalTokenSymbol', () => {
let result;

await act(async () => {
result = renderHook(() =>
useIsOriginalTokenSymbol(tokenAddress, tokenSymbol),
result = renderHookWithProvider(
() => useIsOriginalTokenSymbol(tokenAddress, tokenSymbol),
state,
);
});

// Expect the hook to return false when the symbol matches the original symbol
expect(result.result.current).toBe(false);
});

it('useIsOriginalTokenSymbol uses cached value when available', async () => {
const tokenAddress = '0x1234';
const tokenSymbol = 'ABCD';

actions.getTokenSymbol.mockResolvedValue('Should not matter'); // Mock the getTokenSymbol function

let result;

await act(async () => {
result = renderHookWithProvider(
() => useIsOriginalTokenSymbol(tokenAddress, tokenSymbol),
state,
);
});

// Expect the hook to return true when the symbol matches the original symbol
expect(result.result.current).toBe(true);
});
});

0 comments on commit 2cd5813

Please sign in to comment.