Skip to content

Commit

Permalink
fix: remove unecessary calls to third party apis (#9759)
Browse files Browse the repository at this point in the history
## **Description**

The CollectibleContracts component was making more calls to the NFT API
than it should.

Users could either have the nft detection enabled or disabled.

When the nft detection is enabled, the component was:
1- Executing `detectNfts` function which triggers one call to get all
nfts for a user
2- Calling `updateNftMetadata` which triggers `N` number of calls to the
NFT-API assuming the user has `N` NFT.

The second call is unnecessary if the user already have the nftDetection
enabled.

In this fix, i added a check in the useEffect to see if the user has the
nftDetection enabled or not

```
    if (updatableCollectibles.length !== 0 && !useNftDetection) {
      updateAllCollectibleMetadata(updatableCollectibles);
    }
```
If it is already enabled, calling the detectNfts function should be
enough to refresh metadata.

When the user has the nftDetection Disabled, in this case the
`updateNftMetadata` will be executed.
I made an update in the `updateNftMetadata` fct to not execute the calls
every time the user pulls down t refresh. A 10 mins interval should be
enough.

Note: We will have an update on the nftDetectionController in the future
to update the polling strategy.

## **Related issues**

- Compare url on core:
https://github.com/MetaMask/core/compare/patch/mobile-assets-controllers-26...patch/mobile-assets-controllers-26-optimize-nft-refresh-calls?expand=1
- MetaMask/core#4325

## **Manual testing steps**

To check the unecessary calls:

1. Build on main
2. Go to app/components/UI/CollectibleContracts/index.js and add a log
inside useEffect
3. Go to node-modules, nftDetectionController.js function getOwnerNfts
and add a console.log
4. Go to node-modules, nftController.js function
getNftInformationFromApi and add a log
5. When you have nft detection disabled, go to nft tab, and pull down to
refresh and check the calls made in your terminal
6. Enable nft detection, go to nft tab, pull down to refresh and check
your logs

(See before video, notice on that wallet i have 5 nfts, it should be
making only 1 call per nft => 5 calls in total but its making more than
that. Every state update will result in the useEffect being executed
which we attempt to avoid in this fix)

To check the update on this PR:

1- Build on this branch
2. Go to app/components/UI/CollectibleContracts/index.js and add a log
inside useEffect
3. Go to node-modules, nftDetectionController.js function getOwnerNfts
and add a console.log
4. Go to node-modules, nftController.js function
getNftInformationFromApi and add a log
5. When you have nft detection disabled, go to nft tab, and pull down to
refresh and check the calls made in your terminal
6. Enable nft detection, go to nft tab, pull down to refresh and check
your logs



## **Screenshots/Recordings**

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

### **Before**



https://github.com/MetaMask/metamask-mobile/assets/10994169/77f7ba9e-add9-4ce8-ad78-bc41926276b9



### **After**




https://github.com/MetaMask/metamask-mobile/assets/10994169/4677a899-9fdd-46a3-98e3-9422d6fa9eb9


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.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
sahar-fehri authored Jun 5, 2024
1 parent eea55be commit cb39b6a
Show file tree
Hide file tree
Showing 4 changed files with 330 additions and 75 deletions.
3 changes: 3 additions & 0 deletions app/components/UI/CollectibleContracts/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const RefreshTestId = 'refreshControl';

export default RefreshTestId;
32 changes: 4 additions & 28 deletions app/components/UI/CollectibleContracts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
NFT_TAB_CONTAINER_ID,
} from '../../../../wdio/screen-objects/testIDs/Screens/WalletView.testIds';
import { useMetrics } from '../../../components/hooks/useMetrics';
import RefreshTestId from './constants';

const createStyles = (colors) =>
StyleSheet.create({
Expand Down Expand Up @@ -197,43 +198,17 @@ const CollectibleContracts = ({
const updatableCollectibles = collectibles.filter((single) =>
shouldUpdateCollectibleMetadata(single),
);
if (updatableCollectibles.length !== 0) {
if (updatableCollectibles.length !== 0 && !useNftDetection) {
updateAllCollectibleMetadata(updatableCollectibles);
}
}, [
collectibles,
updateAllCollectibleMetadata,
isIpfsGatewayEnabled,
displayNftMedia,
useNftDetection,
]);

/* const updateCollectibleMetadata = useCallback(
async (collectible) => {
const { NftController } = Engine.context;
const { address, tokenId } = collectible;
const isIgnored = isCollectibleIgnored(collectible);
if (!isIgnored) {
if (String(tokenId).includes('e+')) {
removeFavoriteCollectible(selectedAddress, chainId, collectible);
} else {
await NftController.addNft(address, String(tokenId));
}
}
},
[chainId, removeFavoriteCollectible, selectedAddress, isCollectibleIgnored],
);
useEffect(() => {
// TO DO: Move this fix to the controllers layer
collectibles.forEach((collectible) => {
if (shouldUpdateCollectibleMetadata(collectible)) {
updateCollectibleMetadata(collectible);
}
});
}, [collectibles, updateCollectibleMetadata]); */

const goToAddCollectible = useCallback(() => {
setIsAddNFTEnabled(false);
navigation.push('AddAsset', { assetType: 'collectible' });
Expand Down Expand Up @@ -356,6 +331,7 @@ const CollectibleContracts = ({
data={collectibleContracts}
renderItem={({ item, index }) => renderCollectibleContract(item, index)}
keyExtractor={(_, index) => index.toString()}
testID={RefreshTestId}
refreshControl={
<RefreshControl
colors={[colors.primary.default]}
Expand Down
252 changes: 251 additions & 1 deletion app/components/UI/CollectibleContracts/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import configureMockStore from 'redux-mock-store';
import { Provider } from 'react-redux';
import initialBackgroundState from '../../../util/test/initial-background-state.json';
import renderWithProvider from '../../../util/test/renderWithProvider';
import { act } from '@testing-library/react-hooks';

// eslint-disable-next-line import/no-namespace
import * as allSelectors from '../../../../app/reducers/collectibles/index.js';
import { cleanup, waitFor } from '@testing-library/react-native';
import Engine from '../../../core/Engine';

import TestHelpers from '../../../../e2e/helpers';

jest.mock('@react-navigation/native', () => {
const actualReactNavigation = jest.requireActual('@react-navigation/native');
return {
Expand Down Expand Up @@ -150,7 +153,7 @@ describe('CollectibleContracts', () => {
expect(nonOwnedNft).toBeNull();
});

it('UI refresh changes NFT image when metadata image changes', async () => {
it('UI refresh changes NFT image when metadata image changes - detection disabled', async () => {
const CURRENT_ACCOUNT = '0x1a';
const collectibleData = [
{
Expand Down Expand Up @@ -269,4 +272,251 @@ describe('CollectibleContracts', () => {
spyOnContracts.mockRestore();
spyOnUpdateNftMetadata.mockRestore();
});

it('UI refresh changes NFT image when metadata image changes - detection enabled', async () => {
const CURRENT_ACCOUNT = '0x1a';
const collectibleData = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
name: 'MyToken',
symbol: 'MTK',
},
];
const nftItemData = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
description:
'Lil Pudgys are a collection of 22,222 randomly generated NFTs minted on Ethereum.',
error: 'Opensea import error',
favorite: false,
image: 'https://api.pudgypenguins.io/lil/image/11222',
isCurrentlyOwned: true,
name: 'Lil Pudgy #113',
standard: 'ERC721',
tokenId: '113',
tokenURI: 'https://api.pudgypenguins.io/lil/113',
},
];

const nftItemDataUpdated = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
description:
'Lil Pudgys are a collection of 22,222 randomly generated NFTs minted on Ethereum.',
error: 'Opensea import error',
favorite: false,
image: 'https://api.pudgypenguins.io/lil/image/updated.png',
isCurrentlyOwned: true,
name: 'Lil Pudgy #113',
standard: 'ERC721',
tokenId: '113',
tokenURI: 'https://api.pudgypenguins.io/lil/113',
},
];
const mockState = {
collectibles: {
favorites: {},
},
engine: {
backgroundState: {
...initialBackgroundState,
NetworkController: {
network: '1',
providerConfig: {
ticker: 'ETH',
type: 'mainnet',
chainId: '1',
},
},
AccountTrackerController: {
accounts: { [CURRENT_ACCOUNT]: { balance: '0' } },
},
PreferencesController: {
useNftDetection: true,
displayNftMedia: true,
selectedAddress: CURRENT_ACCOUNT,
identities: {
[CURRENT_ACCOUNT]: {
address: CURRENT_ACCOUNT,
name: 'Account 1',
},
},
},
NftController: {
addNft: jest.fn(),
updateNftMetadata: jest.fn(),
allNfts: {
[CURRENT_ACCOUNT]: {
'1': [],
},
},
allNftContracts: {
[CURRENT_ACCOUNT]: {
'1': [],
},
},
},
NftDetectionController: {
detectNfts: jest.fn(),
},
},
},
};

const spyOnCollectibles = jest
.spyOn(allSelectors, 'collectiblesSelector')
.mockReturnValueOnce(nftItemData)
.mockReturnValueOnce(nftItemDataUpdated);
const spyOnContracts = jest
.spyOn(allSelectors, 'collectibleContractsSelector')
.mockReturnValue(collectibleData);
const spyOnUpdateNftMetadata = jest
.spyOn(Engine.context.NftController, 'updateNftMetadata')
.mockImplementation(async () => undefined);

const { getByTestId } = renderWithProvider(<CollectibleContracts />, {
state: mockState,
});
const nftImageBefore = getByTestId('nft-image');
expect(nftImageBefore.props.source.uri).toEqual(nftItemData[0].image);

const { queryByTestId } = renderWithProvider(<CollectibleContracts />, {
state: mockState,
});

await waitFor(() => {
expect(spyOnUpdateNftMetadata).toHaveBeenCalledTimes(0);
const nftImageAfter = queryByTestId('nft-image');
expect(nftImageAfter.props.source.uri).toEqual(
nftItemDataUpdated[0].image,
);
});

spyOnCollectibles.mockRestore();
spyOnContracts.mockRestore();
spyOnUpdateNftMetadata.mockRestore();
});

it('UI pull down experience should call detectNfts when detection is enabled', async () => {
const CURRENT_ACCOUNT = '0x1a';
const collectibleData = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
name: 'MyToken',
symbol: 'MTK',
},
];
const nftItemData = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
description:
'Lil Pudgys are a collection of 22,222 randomly generated NFTs minted on Ethereum.',
error: 'Opensea import error',
favorite: false,
image: 'https://api.pudgypenguins.io/lil/image/11222',
isCurrentlyOwned: true,
name: 'Lil Pudgy #113',
standard: 'ERC721',
tokenId: '113',
tokenURI: 'https://api.pudgypenguins.io/lil/113',
},
];

const nftItemDataUpdated = [
{
address: '0x72b1FDb6443338A158DeC2FbF411B71aeB157A42',
description:
'Lil Pudgys are a collection of 22,222 randomly generated NFTs minted on Ethereum.',
error: 'Opensea import error',
favorite: false,
image: 'https://api.pudgypenguins.io/lil/image/updated.png',
isCurrentlyOwned: true,
name: 'Lil Pudgy #113',
standard: 'ERC721',
tokenId: '113',
tokenURI: 'https://api.pudgypenguins.io/lil/113',
},
];
const mockState = {
collectibles: {
favorites: {},
},
engine: {
backgroundState: {
...initialBackgroundState,
NetworkController: {
network: '1',
providerConfig: {
ticker: 'ETH',
type: 'mainnet',
chainId: '1',
},
},
AccountTrackerController: {
accounts: { [CURRENT_ACCOUNT]: { balance: '0' } },
},
PreferencesController: {
useNftDetection: true,
displayNftMedia: true,
selectedAddress: CURRENT_ACCOUNT,
identities: {
[CURRENT_ACCOUNT]: {
address: CURRENT_ACCOUNT,
name: 'Account 1',
},
},
},
NftController: {
addNft: jest.fn(),
updateNftMetadata: jest.fn(),
allNfts: {
[CURRENT_ACCOUNT]: {
'1': [],
},
},
allNftContracts: {
[CURRENT_ACCOUNT]: {
'1': [],
},
},
},
NftDetectionController: {
detectNfts: jest.fn(),
},
},
},
};

jest
.spyOn(allSelectors, 'collectiblesSelector')
.mockReturnValueOnce(nftItemData)
.mockReturnValueOnce(nftItemDataUpdated);
jest
.spyOn(allSelectors, 'collectibleContractsSelector')
.mockReturnValue(collectibleData);
const spyOnUpdateNftMetadata = jest
.spyOn(Engine.context.NftController, 'updateNftMetadata')
.mockImplementation(async () => undefined);

const spyOnDetectNfts = jest
.spyOn(Engine.context.NftDetectionController, 'detectNfts')
.mockImplementation(async () => undefined);

const { getByTestId } = renderWithProvider(<CollectibleContracts />, {
state: mockState,
});
const scrollView = getByTestId('refreshControl');

expect(scrollView).toBeDefined();

const { refreshControl } = scrollView.props;
await act(async () => {
await refreshControl.props.onRefresh();
});

await TestHelpers.delay(1000);

expect(spyOnUpdateNftMetadata).toHaveBeenCalledTimes(0);
expect(spyOnDetectNfts).toHaveBeenCalledTimes(1);
});
});
Loading

0 comments on commit cb39b6a

Please sign in to comment.