Skip to content

Commit

Permalink
feat: integrate NFT api to display names for nfts within simulations …
Browse files Browse the repository at this point in the history
…for `ERC721`s (#25692)

<!--
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**

<!--
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?
-->

This PR integrates NFT api (`/collections`) to display name and image of
collection for nfts within simulations only for `ERC721` nfts.

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

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2507
Related to: MetaMask/core#4506

## **Manual testing steps**

1. Go to Opensea, try buying
https://opensea.io/assets/ethereum/0xd4307e0acd12cf46fd6cf93bc264f5d5d1598792/41757
(you don't need to buy) - this is `ERC721`
2. See collection name and image in simulations 

## **Screenshots/Recordings**

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

### **Before**

![Screenshot 2024-07-15 at 15 40
22](https://github.com/user-attachments/assets/b8c6debd-1eee-44af-b9d1-07c0e03cc378)

### **After**

![Screenshot 2024-07-15 at 15 39
42](https://github.com/user-attachments/assets/f393945a-3118-4747-a27b-e7a077b9b32f)

## **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
OGPoyraz authored Sep 23, 2024
1 parent 3ce1ce7 commit 5e011f8
Show file tree
Hide file tree
Showing 10 changed files with 380 additions and 22 deletions.
2 changes: 2 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -3432,6 +3432,8 @@ export default class MetamaskController extends EventEmitter {
nftController,
),

getNFTContractInfo: nftController.getNFTContractInfo.bind(nftController),

isNftOwner: nftController.isNftOwner.bind(nftController),

// AddressController
Expand Down
22 changes: 22 additions & 0 deletions ui/components/app/name/__snapshots__/name.test.tsx.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,27 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Name renders address with image 1`] = `
<div>
<div>
<div
class="name name__saved"
>
<img
alt=""
class="identicon"
src="test-image"
style="height: 16px; width: 16px; border-radius: 8px;"
/>
<p
class="mm-box mm-text name__name mm-text--body-md mm-box--color-text-default"
>
TestName
</p>
</div>
</div>
</div>
`;

exports[`Name renders address with no saved name 1`] = `
<div>
<div>
Expand Down
15 changes: 15 additions & 0 deletions ui/components/app/name/name.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,21 @@ describe('Name', () => {
expect(container).toMatchSnapshot();
});

it('renders address with image', () => {
useDisplayNameMock.mockReturnValue({
name: SAVED_NAME_MOCK,
hasPetname: true,
image: 'test-image',
});

const { container } = renderWithProvider(
<Name type={NameType.ETHEREUM_ADDRESS} value={ADDRESS_SAVED_NAME_MOCK} />,
store,
);

expect(container).toMatchSnapshot();
});

describe('metrics', () => {
// @ts-expect-error This is missing from the Mocha type definitions
it.each([
Expand Down
16 changes: 8 additions & 8 deletions ui/components/app/name/name.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ export type NameProps = {
/** Whether this is being rendered inside the NameDetails modal. */
internal?: boolean;

/** The type of value, e.g. NameType.ETHEREUM_ADDRESS */
type: NameType;

/** The raw value to display the name of. */
value: string;

/**
* Applies to recognized contracts with no petname saved:
* If true the contract symbol (e.g. WBTC) will be used instead of the contract name.
*/
preferContractSymbol?: boolean;

/** The type of value, e.g. NameType.ETHEREUM_ADDRESS */
type: NameType;

/** The raw value to display the name of. */
value: string;
};

function formatValue(value: string, type: NameType): string {
Expand Down Expand Up @@ -65,7 +65,7 @@ const Name = memo(
const [modalOpen, setModalOpen] = useState(false);
const trackEvent = useContext(MetaMetricsContext);

const { name, hasPetname } = useDisplayName(
const { name, hasPetname, image } = useDisplayName(
value,
type,
preferContractSymbol,
Expand Down Expand Up @@ -112,7 +112,7 @@ const Name = memo(
onClick={handleClick}
>
{hasDisplayName ? (
<Identicon address={value} diameter={16} />
<Identicon address={value} diameter={16} image={image} />
) : (
<Icon
name={IconName.Question}
Expand Down
73 changes: 68 additions & 5 deletions ui/hooks/useDisplayName.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { NameEntry, NameType } from '@metamask/name-controller';
import { NftContract } from '@metamask/assets-controllers';
import { renderHook } from '@testing-library/react-hooks';
import { getRemoteTokens } from '../selectors';
import { getNftContractsByAddressOnCurrentChain } from '../selectors/nft';
import { useDisplayName } from './useDisplayName';
import { useNames } from './useName';
import { useFirstPartyContractNames } from './useFirstPartyContractName';
import { useNftCollectionsMetadata } from './useNftCollectionsMetadata';

jest.mock('react-redux', () => ({
// TODO: Replace `any` with type
Expand All @@ -20,6 +22,10 @@ jest.mock('./useFirstPartyContractName', () => ({
useFirstPartyContractNames: jest.fn(),
}));

jest.mock('./useNftCollectionsMetadata', () => ({
useNftCollectionsMetadata: jest.fn(),
}));

jest.mock('../selectors', () => ({
getRemoteTokens: jest.fn(),
getCurrentChainId: jest.fn(),
Expand Down Expand Up @@ -62,6 +68,7 @@ describe('useDisplayName', () => {
const getNftContractsByAddressOnCurrentChainMock = jest.mocked(
getNftContractsByAddressOnCurrentChain,
);
const useNftCollectionsMetadataMock = jest.mocked(useNftCollectionsMetadata);

beforeEach(() => {
jest.resetAllMocks();
Expand All @@ -78,10 +85,12 @@ describe('useDisplayName', () => {
getNftContractsByAddressOnCurrentChainMock.mockReturnValue(
NO_WATCHED_NFT_NAME_FOUND_RETURN_VALUE,
);
useNftCollectionsMetadataMock.mockReturnValue({});
});

it('handles no name found', () => {
expect(useDisplayName(VALUE_MOCK, TYPE_MOCK)).toEqual({
const { result } = renderHook(() => useDisplayName(VALUE_MOCK, TYPE_MOCK));
expect(result.current).toEqual({
name: null,
hasPetname: false,
});
Expand All @@ -101,7 +110,9 @@ describe('useDisplayName', () => {
WATCHED_NFT_FOUND_RETURN_VALUE,
);

expect(useDisplayName(VALUE_MOCK, TYPE_MOCK)).toEqual({
const { result } = renderHook(() => useDisplayName(VALUE_MOCK, TYPE_MOCK));

expect(result.current).toEqual({
name: NAME_MOCK,
hasPetname: true,
contractDisplayName: CONTRACT_NAME_MOCK,
Expand All @@ -119,7 +130,9 @@ describe('useDisplayName', () => {
WATCHED_NFT_FOUND_RETURN_VALUE,
);

expect(useDisplayName(VALUE_MOCK, TYPE_MOCK)).toEqual({
const { result } = renderHook(() => useDisplayName(VALUE_MOCK, TYPE_MOCK));

expect(result.current).toEqual({
name: FIRST_PARTY_CONTRACT_NAME_MOCK,
hasPetname: false,
});
Expand All @@ -135,7 +148,9 @@ describe('useDisplayName', () => {
WATCHED_NFT_FOUND_RETURN_VALUE,
);

expect(useDisplayName(VALUE_MOCK, TYPE_MOCK)).toEqual({
const { result } = renderHook(() => useDisplayName(VALUE_MOCK, TYPE_MOCK));

expect(result.current).toEqual({
name: CONTRACT_NAME_MOCK,
hasPetname: false,
contractDisplayName: CONTRACT_NAME_MOCK,
Expand All @@ -147,9 +162,57 @@ describe('useDisplayName', () => {
WATCHED_NFT_FOUND_RETURN_VALUE,
);

expect(useDisplayName(VALUE_MOCK, TYPE_MOCK)).toEqual({
const { result } = renderHook(() => useDisplayName(VALUE_MOCK, TYPE_MOCK));

expect(result.current).toEqual({
name: WATCHED_NFT_NAME_MOCK,
hasPetname: false,
});
});

it('returns nft collection name from metadata if no other name is found', () => {
const IMAGE_MOCK = 'url';

useNftCollectionsMetadataMock.mockReturnValue({
[VALUE_MOCK.toLowerCase()]: {
name: CONTRACT_NAME_MOCK,
image: IMAGE_MOCK,
isSpam: false,
},
});

const { result } = renderHook(() =>
useDisplayName(VALUE_MOCK, TYPE_MOCK, false),
);

expect(result.current).toEqual({
name: CONTRACT_NAME_MOCK,
hasPetname: false,
contractDisplayName: undefined,
image: IMAGE_MOCK,
});
});

it('does not return nft collection name if collection is marked as spam', () => {
const IMAGE_MOCK = 'url';

useNftCollectionsMetadataMock.mockReturnValue({
[VALUE_MOCK.toLowerCase()]: {
name: CONTRACT_NAME_MOCK,
image: IMAGE_MOCK,
isSpam: true,
},
});

const { result } = renderHook(() =>
useDisplayName(VALUE_MOCK, TYPE_MOCK, false),
);

expect(result.current).toEqual(
expect.objectContaining({
name: null,
image: undefined,
}),
);
});
});
32 changes: 24 additions & 8 deletions ui/hooks/useDisplayName.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,36 @@
import { useMemo } from 'react';
import { NameType } from '@metamask/name-controller';
import { useSelector } from 'react-redux';
import { getRemoteTokens } from '../selectors';
import { getNftContractsByAddressOnCurrentChain } from '../selectors/nft';
import { useNames } from './useName';
import { useFirstPartyContractNames } from './useFirstPartyContractName';
import { useNftCollectionsMetadata } from './useNftCollectionsMetadata';

export type UseDisplayNameRequest = {
value: string;
type: NameType;
preferContractSymbol?: boolean;
type: NameType;
};

export type UseDisplayNameResponse = {
name: string | null;
hasPetname: boolean;
contractDisplayName?: string;
image?: string;
};

export function useDisplayNames(
requests: UseDisplayNameRequest[],
): UseDisplayNameResponse[] {
const nameRequests = requests.map(({ value, type }) => ({
value,
type,
}));
const nameRequests = useMemo(
() => requests.map(({ value, type }) => ({ value, type })),
[requests],
);

const nameEntries = useNames(nameRequests);
const firstPartyContractNames = useFirstPartyContractNames(nameRequests);
const nftCollections = useNftCollectionsMetadata(nameRequests);
const values = requests.map(({ value }) => value);

const contractInfo = useSelector((state) =>
Expand All @@ -42,6 +46,16 @@ export function useDisplayNames(
const firstPartyContractName = firstPartyContractNames[index];
const singleContractInfo = contractInfo[index];
const watchedNftName = watchedNftNames[value.toLowerCase()]?.name;
const nftCollectionProperties = nftCollections[value.toLowerCase()];

const isNotSpam = nftCollectionProperties?.isSpam === false;

const nftCollectionName = isNotSpam
? nftCollectionProperties?.name
: undefined;
const nftCollectionImage = isNotSpam
? nftCollectionProperties?.image
: undefined;

const contractDisplayName =
preferContractSymbol && singleContractInfo?.symbol
Expand All @@ -51,6 +65,7 @@ export function useDisplayNames(
const name =
nameEntry?.name ||
firstPartyContractName ||
nftCollectionName ||
contractDisplayName ||
watchedNftName ||
null;
Expand All @@ -61,15 +76,16 @@ export function useDisplayNames(
name,
hasPetname,
contractDisplayName,
image: nftCollectionImage,
};
});
}

/**
* Attempts to resolve the name for the given parameters.
*
* @param value
* @param type
* @param value - The address or contract address to resolve.
* @param type - The type of value, e.g. NameType.ETHEREUM_ADDRESS.
* @param preferContractSymbol - Applies to recognized contracts when no petname is saved:
* If true the contract symbol (e.g. WBTC) will be used instead of the contract name.
* @returns An object with two properties:
Expand All @@ -81,5 +97,5 @@ export function useDisplayName(
type: NameType,
preferContractSymbol: boolean = false,
): UseDisplayNameResponse {
return useDisplayNames([{ value, type, preferContractSymbol }])[0];
return useDisplayNames([{ preferContractSymbol, type, value }])[0];
}
Loading

0 comments on commit 5e011f8

Please sign in to comment.