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: Nft chainId and current global chainId arent always the same #30517

Merged
merged 6 commits into from
Feb 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import NftDetails from './nft-details';
import { NftDetailsComponent } from './nft-details';

const nft = {
name: 'Catnip Spicywright',
Expand All @@ -18,6 +18,8 @@ const nft = {
},
};

const nftChainId = '0x1';

export default {
title: 'Components/App/NftsDetail',

Expand All @@ -28,17 +30,18 @@ export default {
},
args: {
nft,
nftChainId,
},
};

export const DefaultStory = (args) => {
return <NftDetails {...args} />;
return <NftDetailsComponent {...args} />;
};

DefaultStory.storyName = 'Default';

export const NoImage = (args) => {
return <NftDetails {...args} />;
return <NftDetailsComponent {...args} />;
};

NoImage.args = {
Expand Down
25 changes: 23 additions & 2 deletions ui/components/app/assets/nfts/nft-details/nft-details.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { fireEvent, waitFor } from '@testing-library/react';
import React from 'react';
import { useParams } from 'react-router-dom';
import configureMockStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import copyToClipboard from 'copy-to-clipboard';
Expand Down Expand Up @@ -39,6 +40,7 @@ jest.mock('react-router-dom', () => ({
useHistory: () => ({
push: mockHistoryPush,
}),
useParams: jest.fn(),
}));

jest.mock('../../../../../ducks/send/index.js', () => ({
Expand Down Expand Up @@ -72,6 +74,7 @@ describe('NFT Details', () => {
});

it('should match minimal props and state snapshot', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.GOERLI });
getAssetImageURL.mockResolvedValue(
'https://bafybeiclzx7zfjvuiuwobn5ip3ogc236bjqfjzoblumf4pau4ep6dqramu.ipfs.dweb.link',
);
Expand All @@ -88,6 +91,7 @@ describe('NFT Details', () => {
});

it(`should route to '/' route when the back button is clicked`, () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const { queryByTestId } = renderWithProvider(
<NftDetails {...props} />,
mockStore,
Expand All @@ -101,6 +105,7 @@ describe('NFT Details', () => {
});

it(`should call removeAndIgnoreNFT with proper nft details and route to '/' when removing nft`, async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const { queryByTestId } = renderWithProvider(
<NftDetails {...props} />,
mockStore,
Expand All @@ -122,6 +127,7 @@ describe('NFT Details', () => {
});

it(`should call setRemoveNftMessage with error when removeAndIgnoreNft fails and route to '/'`, async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const { queryByTestId } = renderWithProvider(
<NftDetails {...props} />,
mockStore,
Expand All @@ -145,6 +151,7 @@ describe('NFT Details', () => {
});

it('should copy nft address', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const { queryByTestId } = renderWithProvider(
<NftDetails {...props} />,
mockStore,
Expand All @@ -157,6 +164,7 @@ describe('NFT Details', () => {
});

it('should navigate to draft transaction send route with ERC721 data', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const nftProps = {
nft: nfts[5],
};
Expand All @@ -180,6 +188,7 @@ describe('NFT Details', () => {
});

it('should not render send button if isCurrentlyOwned is false', () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const sixthNftProps = {
nft: nfts[6],
};
Expand All @@ -195,6 +204,7 @@ describe('NFT Details', () => {
});

it('should render send button if it is an ERC1155', () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
const nftProps = {
nft: nfts[1],
};
Expand All @@ -211,6 +221,7 @@ describe('NFT Details', () => {

describe(`Alternative Networks' OpenSea Links`, () => {
it('should open opeasea link with goeli testnet chainId', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.GOERLI });
global.platform = { openTab: jest.fn() };

const { queryByTestId } = renderWithProvider(
Expand All @@ -234,6 +245,7 @@ describe('NFT Details', () => {
});

it('should open tab to mainnet opensea url with nft info', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.MAINNET });
global.platform = { openTab: jest.fn() };

const mainnetState = {
Expand Down Expand Up @@ -266,11 +278,15 @@ describe('NFT Details', () => {
});

it('should open tab to polygon opensea url with nft info', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.POLYGON });
const polygonState = {
...mockState,
metamask: {
...mockState.metamask,
...mockNetworkState({ chainId: CHAIN_IDS.POLYGON }),
...mockNetworkState({
chainId: CHAIN_IDS.POLYGON,
nickname: 'polygon',
}),
},
};
const polygonMockStore = configureMockStore([thunk])(polygonState);
Expand All @@ -296,11 +312,15 @@ describe('NFT Details', () => {
});

it('should open tab to sepolia opensea url with nft info', async () => {
useParams.mockReturnValue({ chainId: CHAIN_IDS.SEPOLIA });
const sepoliaState = {
...mockState,
metamask: {
...mockState.metamask,
...mockNetworkState({ chainId: CHAIN_IDS.SEPOLIA }),
...mockNetworkState({
chainId: CHAIN_IDS.SEPOLIA,
nickname: 'sepolia',
}),
},
};
const sepoliaMockStore = configureMockStore([thunk])(sepoliaState);
Expand All @@ -326,6 +346,7 @@ describe('NFT Details', () => {
});

it('should not render opensea redirect button', async () => {
useParams.mockReturnValue({ chainId: '0x99' });
const randomNetworkState = {
...mockState,
metamask: {
Expand Down
66 changes: 60 additions & 6 deletions ui/components/app/assets/nfts/nft-details/nft-details.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React, { useEffect, useContext } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { useHistory } from 'react-router-dom';
import { useHistory, useParams } from 'react-router-dom';
import { isEqual } from 'lodash';
import { getTokenTrackerLink, getAccountLink } from '@metamask/etherscan-link';
import { Nft } from '@metamask/assets-controllers';
import { Hex } from '@metamask/utils';
import {
TextColor,
IconColor,
Expand All @@ -19,8 +20,15 @@ import {
import { useI18nContext } from '../../../../../hooks/useI18nContext';
import { shortenAddress } from '../../../../../helpers/utils/util';
import { getNftImageAlt } from '../../../../../helpers/utils/nfts';
import { getCurrentChainId } from '../../../../../../shared/modules/selectors/networks';
import { getCurrentNetwork, getIpfsGateway } from '../../../../../selectors';
import {
getCurrentChainId,
getNetworkConfigurationsByChainId,
} from '../../../../../../shared/modules/selectors/networks';
import {
getCurrentNetwork,
getIpfsGateway,
getNetworkConfigurationIdByChainId,
} from '../../../../../selectors';
import {
ASSET_ROUTE,
DEFAULT_ROUTE,
Expand All @@ -31,6 +39,8 @@ import {
removeAndIgnoreNft,
setRemoveNftMessage,
setNewNftAddedMessage,
setActiveNetworkWithError,
setSwitchedNetworkDetails,
} from '../../../../../store/actions';
import { CHAIN_IDS } from '../../../../../../shared/constants/network';
import NftOptions from '../nft-options/nft-options';
Expand Down Expand Up @@ -71,13 +81,20 @@ import { Numeric } from '../../../../../../shared/modules/Numeric';
// eslint-disable-next-line import/no-restricted-paths
import { addUrlProtocolPrefix } from '../../../../../../app/scripts/lib/util';
import useGetAssetImageUrl from '../../../../../hooks/useGetAssetImageUrl';
import { getImageForChainId } from '../../../../../selectors/multichain';
import NftDetailInformationRow from './nft-detail-information-row';
import NftDetailInformationFrame from './nft-detail-information-frame';
import NftDetailDescription from './nft-detail-description';

const MAX_TOKEN_ID_LENGTH = 15;

export default function NftDetails({ nft }: { nft: Nft }) {
export function NftDetailsComponent({
nft,
nftChainId,
}: {
nft: Nft;
nftChainId: string;
}) {
const {
image,
imageOriginal,
Expand All @@ -104,6 +121,14 @@ export default function NftDetails({ nft }: { nft: Nft }) {
const currency = useSelector(getCurrentCurrency);
const selectedNativeConversionRate = useSelector(getConversionRate);

const nftNetworkConfigs = useSelector(getNetworkConfigurationsByChainId);
const nftChainNetwork = nftNetworkConfigs[nftChainId as Hex];
const nftChainImage = getImageForChainId(nftChainId as string);
const networks = useSelector(getNetworkConfigurationIdByChainId) as Record<
string,
string
>;

const [addressCopied, handleAddressCopy] = useCopyToClipboard();

const nftImageAlt = getNftImageAlt(nft);
Expand Down Expand Up @@ -240,7 +265,28 @@ export default function NftDetails({ nft }: { nft: Nft }) {
const sendDisabled =
standard !== TokenStandard.ERC721 && standard !== TokenStandard.ERC1155;

const setCorrectChain = async () => {
// If we aren't presently on the chain of the nft, change to it
if (nftChainId !== currentChain.chainId) {
try {
const networkConfigurationId = networks[nftChainId as Hex];
await dispatch(setActiveNetworkWithError(networkConfigurationId));
await dispatch(
setSwitchedNetworkDetails({
networkClientId: networkConfigurationId,
}),
);
} catch (err) {
console.error(`Failed to switch chains for NFT.
Target chainId: ${nftChainId}, Current chainId: ${currentChain.chainId}.
${err}`);
throw err;
}
}
};

const onSend = async () => {
await setCorrectChain();
await dispatch(
startNewDraftTransaction({
type: AssetType.NFT,
Expand Down Expand Up @@ -350,8 +396,8 @@ export default function NftDetails({ nft }: { nft: Nft }) {
<NftItem
src={nftItemSrc as string | undefined}
alt={nftImageAlt}
networkName={currentChain.nickname ?? ''}
networkSrc={currentChain.rpcPrefs?.imageUrl}
networkName={nftChainNetwork.name ?? ''}
networkSrc={nftChainImage}
isIpfsURL={isIpfsURL}
onClick={handleImageClick}
detailView
Expand Down Expand Up @@ -851,3 +897,11 @@ export default function NftDetails({ nft }: { nft: Nft }) {
</Page>
);
}

function NftDetails({ nft }: { nft: Nft }) {
const { chainId } = useParams();

return <NftDetailsComponent nft={nft} nftChainId={chainId ?? ''} />;
}
Comment on lines +901 to +905
Copy link
Contributor Author

@gambinish gambinish Feb 24, 2025

Choose a reason for hiding this comment

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

Had to make this additional wrapper to satisfy the storybook ci test. It didn't like me using useParams in storybook and only wanted props (rather than an internal hook), so this wrapper allows me to bypass that issue while maintaining functionality.


export default NftDetails;
Loading