Skip to content

Commit c2cea4a

Browse files
chore(runway): cherry-pick fix: cp-7.60.0 use correct chainId collectibles for nft send flow (#23071)
- fix: cp-7.60.0 use correct chainId collectibles for nft send flow (#22966) <!-- 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 underlying sendflow uses `selectChainId` which should be marked as deprecated. - This selected does not return the correct chain ID when you are using "Popular Networks" view as you can have multiple chainIDs selected now!! This has unearthed a very large scope at this selector underpins many areas in mobile and extension 💀 Ideally all flows and features should move away from this selector and handle multiple selected chainIDs. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: fix: correct nft images available during send flow ## **Related issues** Fixes: #20982 ## **Manual testing steps** 1. Select Popular Networks Tab 2. Go through NFT send flow - Select NFT from (e.g.) Linea (not ethereum) 3. Expected: On review page, should see NFT and can select NFT image without crashing app ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> https://www.loom.com/share/683675a4c01b47b0a99f6ff5e9dfa30a ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Switches NFT lookup to a chain-scoped selector to fix multi-network send flow, updates HeroNft placeholder behavior, adds selector tests, and deprecates legacy selectors. > > - **State/Selectors**: > - Add `selectAllCollectiblesByChain` in `reducers/collectibles/collectibles.ts` with unit tests, enabling per-chain NFT selection. > - Mark `collectiblesSelector` and `selectChainId` as deprecated with notes about multi-network selection. > - **Hooks**: > - Update `useNft` to: > - Derive `hexChainId` and select NFTs via `selectAllCollectiblesByChain`. > - Return `chainId` as hex and resolve contract/name accordingly. > - **UI**: > - `HeroNft` placeholder: make non-interactive when no NFT; only show `#tokenId` if present; remove "Show" text. > - **Tests**: > - Adjust `hero-nft.test.tsx` to reflect placeholder behavior and collection image fallback. > - Add `collectibles.test.ts` covering `selectAllCollectiblesByChain` edge cases. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 4c30784. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> [e5692de](e5692de) Co-authored-by: Prithpal Sooriya <prithpal.sooriya@consensys.net>
1 parent 259bba5 commit c2cea4a

File tree

7 files changed

+175
-29
lines changed

7 files changed

+175
-29
lines changed

app/components/Views/confirmations/components/hero-nft/hero-nft.test.tsx

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -41,33 +41,29 @@ describe('HeroNft', () => {
4141
});
4242

4343
it('renders placeholder when image is not provided', () => {
44-
const { getByText, getByTestId, queryAllByText } = renderWithProvider(
45-
<HeroNft />,
46-
{
47-
state: merge({}, MOCK_STATE_NFT, {
48-
engine: {
49-
backgroundState: {
50-
NftController: {
51-
allNfts: {
52-
[MOCK_ADDRESS_1.toLowerCase()]: {
53-
'0x1': [
54-
{
55-
...mockNft,
56-
image: undefined,
57-
},
58-
],
59-
},
44+
const { getByTestId, queryAllByText } = renderWithProvider(<HeroNft />, {
45+
state: merge({}, MOCK_STATE_NFT, {
46+
engine: {
47+
backgroundState: {
48+
NftController: {
49+
allNfts: {
50+
[MOCK_ADDRESS_1.toLowerCase()]: {
51+
'0x1': [
52+
{
53+
...mockNft,
54+
image: undefined,
55+
},
56+
],
6057
},
6158
},
6259
},
6360
},
64-
}),
65-
},
66-
);
61+
},
62+
}),
63+
});
6764

68-
expect(getByText('Show')).toBeDefined();
6965
expect(queryAllByText('#12345')).toHaveLength(2);
70-
expect(getByTestId('hero-nft-placeholder')).toBeDefined();
66+
expect(getByTestId('hero-nft-placeholder')).toBeOnTheScreen();
7167

7268
fireEvent.press(getByTestId('hero-nft-placeholder'));
7369
expect(mockNavigate).toHaveBeenCalledWith('NftDetailsFullImage', {
@@ -78,6 +74,28 @@ describe('HeroNft', () => {
7874
});
7975
});
8076

77+
it('placeholder is not interactable when no nft found', () => {
78+
const state = merge({}, MOCK_STATE_NFT, {
79+
engine: {
80+
backgroundState: {
81+
NftController: {},
82+
},
83+
},
84+
});
85+
state.engine.backgroundState.NftController.allNfts[
86+
MOCK_ADDRESS_1.toLowerCase()
87+
]['0x1'] = [];
88+
const { getByTestId } = renderWithProvider(<HeroNft />, {
89+
state,
90+
});
91+
92+
expect(getByTestId('hero-nft-placeholder')).toBeOnTheScreen();
93+
94+
fireEvent.press(getByTestId('hero-nft-placeholder'));
95+
96+
expect(mockNavigate).not.toHaveBeenCalled();
97+
});
98+
8199
it('renders NFT with network badge', () => {
82100
const { getByTestId, getByText } = renderWithProvider(<HeroNft />, {
83101
state: MOCK_STATE_NFT,
@@ -89,6 +107,7 @@ describe('HeroNft', () => {
89107
expect(getByText('#12345')).toBeDefined();
90108

91109
fireEvent.press(getByTestId('nft-image'));
110+
92111
expect(mockNavigate).toHaveBeenCalledWith('NftDetailsFullImage', {
93112
collectible: mockNft,
94113
});
@@ -123,6 +142,7 @@ describe('HeroNft', () => {
123142
expect(getByText('#12345')).toBeDefined();
124143

125144
fireEvent.press(getByTestId('nft-image'));
145+
126146
expect(mockNavigate).toHaveBeenCalledWith('NftDetailsFullImage', {
127147
collectible: {
128148
...mockNft,

app/components/Views/confirmations/components/hero-nft/hero-nft.tsx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,7 @@ import Badge, {
88
import BadgeWrapper, {
99
BadgePosition,
1010
} from '../../../../../component-library/components/Badges/BadgeWrapper';
11-
import Text, {
12-
TextColor,
13-
} from '../../../../../component-library/components/Texts/Text';
11+
import Text from '../../../../../component-library/components/Texts/Text';
1412
import { useStyles } from '../../../../../component-library/hooks/useStyles';
1513
import CollectibleMedia from '../../../../UI/CollectibleMedia';
1614
import { useNft } from '../../hooks/nft/useNft';
@@ -38,6 +36,10 @@ const NftImageAndNetworkBadge = ({
3836
const showPlaceholder = !nft || !chainId?.length || (!image && !imageUrl);
3937

4038
const onPress = useCallback(() => {
39+
if (!nft) {
40+
return;
41+
}
42+
4143
navigation.navigate('NftDetailsFullImage', {
4244
collectible: nft,
4345
});
@@ -51,8 +53,7 @@ const NftImageAndNetworkBadge = ({
5153
testID="hero-nft-placeholder"
5254
>
5355
<View style={styles.noImagePlaceholder}>
54-
<Text>{`#${tokenId}`}</Text>
55-
<Text color={TextColor.Primary}>Show</Text>
56+
{tokenId && <Text>{`#${tokenId}`}</Text>}
5657
</View>
5758
</TouchableOpacity>
5859
);

app/components/Views/confirmations/hooks/nft/useNft.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import { useSelector } from 'react-redux';
22
import { Nft } from '@metamask/assets-controllers';
33
import { toHex } from '@metamask/controller-utils';
4-
import { collectiblesSelector } from '../../../../../reducers/collectibles';
4+
import type { Hex } from '@metamask/utils';
55
import { selectAllNftContracts } from '../../../../../selectors/nftController';
66
import { safeToChecksumAddress } from '../../../../../util/address';
77
import { parseStandardTokenTransactionData } from '../../utils/transaction';
88
import { useTransactionMetadataRequest } from '../transactions/useTransactionMetadataRequest';
9+
import { selectAllCollectiblesByChain } from '../../../../../reducers/collectibles/collectibles';
10+
import { RootState } from '../../../../../reducers';
911

1012
export interface UseNftResponse {
1113
chainId: string;
@@ -39,13 +41,16 @@ export const useNft = (): UseNftResponse => {
3941
transactionData?.args?._value ?? transactionData?.args[2]
4042
)?.toString();
4143

42-
const nfts: Nft[] = useSelector(collectiblesSelector);
44+
const hexChainId = (chainId && toHex(chainId)) as Hex;
45+
const nfts: Nft[] = useSelector((state: RootState) =>
46+
selectAllCollectiblesByChain(state, hexChainId),
47+
);
4348
const nft = tokenId ? nfts.find((c) => c.tokenId === tokenId) : undefined;
4449

4550
const nftContract = useNftContract(chainId, tokenAddress);
4651

4752
return {
48-
chainId: toHex(chainId).toString(),
53+
chainId: hexChainId.toString(),
4954
name: nftContract?.name,
5055
nft,
5156
tokenId,
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
/* eslint-disable import/no-namespace */
2+
import { selectAllCollectiblesByChain } from './collectibles';
3+
import * as AccountsControllerSelectorsModule from '../../selectors/accountsController';
4+
import * as NFTControllerSelectorsModule from '../../selectors/nftController';
5+
import type { RootState } from '../index';
6+
import type { Hex } from '@metamask/utils';
7+
import { Nft } from '@metamask/assets-controllers';
8+
9+
jest.mock('../../selectors/accountsController');
10+
jest.mock('../../selectors/nftController');
11+
12+
describe('selectAllCollectiblesByChain', () => {
13+
beforeEach(() => {
14+
jest.clearAllMocks();
15+
selectAllCollectiblesByChain.clearCache();
16+
});
17+
18+
const arrange = () => {
19+
const mockSelectSelectedInternalAccountAddress = jest.spyOn(
20+
AccountsControllerSelectorsModule,
21+
'selectSelectedInternalAccountAddress',
22+
);
23+
const mockSelectAllNfts = jest.spyOn(
24+
NFTControllerSelectorsModule,
25+
'selectAllNfts',
26+
);
27+
return {
28+
mockSelectSelectedInternalAccountAddress,
29+
mockSelectAllNfts,
30+
};
31+
};
32+
33+
const mockState: RootState = {} as RootState;
34+
const mockChainId: Hex = '0x1';
35+
const mockAddress = '0x123456789abcdef';
36+
const mockNftContracts = {
37+
[mockAddress]: {
38+
[mockChainId]: [
39+
{ address: '0xcontract1', tokenId: '1', name: 'NFT 1' } as Nft,
40+
{ address: '0xcontract2', tokenId: '2', name: 'NFT 2' } as Nft,
41+
],
42+
'0x89': [{ address: '0xcontract3', tokenId: '3', name: 'NFT 3' } as Nft],
43+
},
44+
};
45+
46+
it('returns collectibles for valid address and chain', () => {
47+
const { mockSelectSelectedInternalAccountAddress, mockSelectAllNfts } =
48+
arrange();
49+
mockSelectSelectedInternalAccountAddress.mockReturnValue(mockAddress);
50+
mockSelectAllNfts.mockReturnValue(mockNftContracts);
51+
52+
const result = selectAllCollectiblesByChain(mockState, mockChainId);
53+
54+
expect(result).toEqual(mockNftContracts[mockAddress][mockChainId]);
55+
expect(result).toHaveLength(2);
56+
});
57+
58+
it.each([
59+
{
60+
case: 'no address',
61+
selectedAddress: undefined,
62+
allNfts: mockNftContracts,
63+
chainId: mockChainId,
64+
},
65+
{
66+
case: 'no nfts',
67+
selectedAddress: mockAddress,
68+
allNfts: {},
69+
chainId: mockChainId,
70+
},
71+
{
72+
case: 'missing address',
73+
selectedAddress: '0xnonexistent',
74+
allNfts: mockNftContracts,
75+
chainId: mockChainId,
76+
},
77+
{
78+
case: 'missing chainId',
79+
selectedAddress: mockAddress,
80+
allNfts: mockNftContracts,
81+
chainId: '0xnonexistent' as Hex,
82+
},
83+
])('returns empty array - $case', ({ selectedAddress, allNfts, chainId }) => {
84+
const { mockSelectSelectedInternalAccountAddress, mockSelectAllNfts } =
85+
arrange();
86+
87+
mockSelectSelectedInternalAccountAddress.mockReturnValue(selectedAddress);
88+
mockSelectAllNfts.mockReturnValue(allNfts);
89+
90+
const result = selectAllCollectiblesByChain(mockState, chainId);
91+
92+
expect(result).toEqual([]);
93+
});
94+
});
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import { createSelector } from 'reselect';
2+
import type { Hex } from '@metamask/utils';
3+
import type { RootState } from '../index';
4+
import { selectSelectedInternalAccountAddress } from '../../selectors/accountsController';
5+
import { selectAllNfts } from '../../selectors/nftController';
6+
7+
export const selectAllCollectiblesByChain = createSelector(
8+
[
9+
selectAllNfts,
10+
selectSelectedInternalAccountAddress,
11+
(_: RootState, chainId: Hex) => chainId,
12+
],
13+
(allNftContracts, address, chainId) => {
14+
if (!address) {
15+
return [];
16+
}
17+
return allNftContracts?.[address]?.[chainId] || [];
18+
},
19+
);

app/reducers/collectibles/index.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ export const collectibleContractsSelector = createSelector(
2323
allNftContracts[address]?.[chainId] || [],
2424
);
2525

26+
/**
27+
* @deprecated - this does not return all collectibles if multiple networks are selected
28+
*/
2629
export const collectiblesSelector = createDeepEqualSelector(
2730
selectSelectedInternalAccountAddress,
2831
selectChainId,

app/selectors/networkController.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,10 @@ export const selectEvmChainId = createSelector(
158158
(providerConfig) => providerConfig.chainId,
159159
);
160160

161+
/**
162+
* @deprecated this does not reliabially return selected chain as users can have "popular networks" selected (multiple networks)
163+
* Logic and components need to handle multiple networks selected
164+
*/
161165
export const selectChainId = createDeepEqualSelector(
162166
selectSelectedNonEvmNetworkChainId,
163167
selectEvmChainId,

0 commit comments

Comments
 (0)