Skip to content

Commit

Permalink
fix: fix update nft metadata when toggles off (#4096)
Browse files Browse the repository at this point in the history
## Explanation

This PR fixes a bug on extension, where when a user imports an NFT when
IPFS and display media toggles are off, then enables them, the metadata
wont be refreshed.
We added a check in NftController to see if any of the toggles are on
then proceed to update nft metadata.
We will update the metadata only for NFTs that are missing name/image or
description.

### **Before**


https://github.com/MetaMask/metamask-extension/assets/10994169/c0d09b77-1cd2-495e-aec8-a4ba965ed38f

### **After**


https://github.com/MetaMask/metamask-extension/assets/10994169/06e56b4f-5c1b-4402-b6e2-154daff291d5


## References

<!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
-->

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/assets-controllers`

- **ADDED**: Added a check inside `onPreferencesStateChange` to check if
IPFS toggle or display media toggle is ON. If so i update the users nft
metadata.
- **REMOVED**: Removed Nfts argument from `updateNftMetadata` function.

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
  • Loading branch information
sahar-fehri authored Mar 26, 2024
1 parent 0e0ee52 commit b4af134
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 8 deletions.
122 changes: 121 additions & 1 deletion packages/assets-controllers/src/NftController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3408,6 +3408,7 @@ describe('NftController', () => {
nftMetadata: { name: '', description: '', image: '', standard: '' },
networkClientId: testNetworkClientId,
});

sinon
.stub(nftController, 'getNftInformation' as keyof typeof nftController)
.returns({
Expand Down Expand Up @@ -3464,6 +3465,7 @@ describe('NftController', () => {
},
networkClientId: testNetworkClientId,
});

sinon
.stub(nftController, 'getNftInformation' as keyof typeof nftController)
.rejects(new Error('Error'));
Expand Down Expand Up @@ -3500,7 +3502,7 @@ describe('NftController', () => {
});
});

it('should update metadata when some calls to fetch metadata succeed', async () => {
it('should update metadata when some calls to fetch metadata succeed', async () => {
const { nftController } = setupController();
const { selectedAddress } = nftController.config;
const spy = jest.spyOn(nftController, 'updateNft');
Expand All @@ -3515,6 +3517,7 @@ describe('NftController', () => {
},
networkClientId: testNetworkClientId,
});

await nftController.addNft('0xtest2', '2', {
nftMetadata: {
name: '',
Expand All @@ -3524,6 +3527,7 @@ describe('NftController', () => {
},
networkClientId: testNetworkClientId,
});

await nftController.addNft('0xtest3', '3', {
nftMetadata: {
name: '',
Expand All @@ -3533,6 +3537,7 @@ describe('NftController', () => {
},
networkClientId: testNetworkClientId,
});

sinon
.stub(nftController, 'getNftInformation' as keyof typeof nftController)
.onFirstCall()
Expand Down Expand Up @@ -3624,5 +3629,120 @@ describe('NftController', () => {
},
]);
});

it('should not update metadata when nfts has image/name/description already', async () => {
const { nftController, triggerPreferencesStateChange } =
setupController();
const spy = jest.spyOn(nftController, 'updateNftMetadata');
const testNetworkClientId = 'sepolia';

// Add nfts
await nftController.addNft('0xtest', '3', {
nftMetadata: {
name: 'test name',
description: 'test description',
image: 'test image',
standard: 'ERC721',
},
userAddress: OWNER_ADDRESS,
networkClientId: testNetworkClientId,
});

// trigger preference change
triggerPreferencesStateChange({
...getDefaultPreferencesState(),
isIpfsGatewayEnabled: false,
openSeaEnabled: true,
selectedAddress: OWNER_ADDRESS,
});

expect(spy).toHaveBeenCalledTimes(0);
});

it('should trigger calling updateNftMetadata when preferences change - openseaEnabled', async () => {
const { nftController, triggerPreferencesStateChange, changeNetwork } =
setupController();
changeNetwork(SEPOLIA);
const spy = jest.spyOn(nftController, 'updateNftMetadata');

const testNetworkClientId = 'sepolia';
// Add nfts
await nftController.addNft('0xtest', '1', {
nftMetadata: {
name: '',
description: '',
image: '',
standard: 'ERC721',
},
userAddress: OWNER_ADDRESS,
networkClientId: testNetworkClientId,
});

expect(
nftController.state.allNfts[OWNER_ADDRESS][SEPOLIA.chainId][0]
.isCurrentlyOwned,
).toBe(true);

sinon
.stub(nftController, 'getNftInformation' as keyof typeof nftController)
.returns({
name: 'name pudgy',
image: 'url pudgy',
description: 'description pudgy',
});

// trigger preference change
triggerPreferencesStateChange({
...getDefaultPreferencesState(),
isIpfsGatewayEnabled: false,
openSeaEnabled: true,
selectedAddress: OWNER_ADDRESS,
});

expect(spy).toHaveBeenCalledTimes(1);
});

it('should trigger calling updateNftMetadata when preferences change - ipfs enabled', async () => {
const { nftController, triggerPreferencesStateChange, changeNetwork } =
setupController();
changeNetwork(SEPOLIA);
const spy = jest.spyOn(nftController, 'updateNftMetadata');

const testNetworkClientId = 'sepolia';
// Add nfts
await nftController.addNft('0xtest', '1', {
nftMetadata: {
name: '',
description: '',
image: '',
standard: 'ERC721',
},
userAddress: OWNER_ADDRESS,
networkClientId: testNetworkClientId,
});

expect(
nftController.state.allNfts[OWNER_ADDRESS][SEPOLIA.chainId][0]
.isCurrentlyOwned,
).toBe(true);

sinon
.stub(nftController, 'getNftInformation' as keyof typeof nftController)
.returns({
name: 'name pudgy',
image: 'url pudgy',
description: 'description pudgy',
});

// trigger preference change
triggerPreferencesStateChange({
...getDefaultPreferencesState(),
isIpfsGatewayEnabled: true,
openSeaEnabled: false,
selectedAddress: OWNER_ADDRESS,
});

expect(spy).toHaveBeenCalledTimes(1);
});
});
});
33 changes: 26 additions & 7 deletions packages/assets-controllers/src/NftController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ export class NftController extends BaseControllerV1<NftConfig, NftState> {
...oldState,
...{ [userAddress]: newAddressState },
};

this.update({
[baseStateKey]: newState,
});
Expand Down Expand Up @@ -1041,7 +1040,7 @@ export class NftController extends BaseControllerV1<NftConfig, NftState> {
this.messagingSystem = messenger;

onPreferencesStateChange(
({
async ({
selectedAddress,
ipfsGateway,
openSeaEnabled,
Expand All @@ -1053,6 +1052,26 @@ export class NftController extends BaseControllerV1<NftConfig, NftState> {
openSeaEnabled,
isIpfsGatewayEnabled,
});

const needsUpdateNftMetadata =
(isIpfsGatewayEnabled && ipfsGateway !== '') || openSeaEnabled;

if (needsUpdateNftMetadata) {
const { chainId } = this.config;
const nfts: Nft[] =
this.state.allNfts[selectedAddress]?.[chainId] ?? [];
// filter only nfts
const nftsToUpdate = nfts.filter(
(singleNft) =>
!singleNft.name && !singleNft.description && !singleNft.image,
);
if (nftsToUpdate.length !== 0) {
await this.updateNftMetadata({
nfts: nftsToUpdate,
userAddress: selectedAddress,
});
}
}
},
);

Expand Down Expand Up @@ -1363,20 +1382,21 @@ export class NftController extends BaseControllerV1<NftConfig, NftState> {
* Refetches NFT metadata and updates the state
*
* @param options - Options for refetching NFT metadata
* @param options.nfts - Array of nfts
* @param options.networkClientId - The networkClientId that can be used to identify the network client to use for this request.
* @param options.nfts - nfts to update metadata for.
* @param options.userAddress - The current user address
* @param options.networkClientId - The networkClientId that can be used to identify the network client to use for this request.
*/
async updateNftMetadata({
nfts,
networkClientId,
userAddress = this.config.selectedAddress,
networkClientId,
}: {
nfts: Nft[];
networkClientId?: NetworkClientId;
userAddress?: string;
networkClientId?: NetworkClientId;
}) {
const chainId = this.getCorrectChainId({ networkClientId });

const nftsWithChecksumAdr = nfts.map((nft) => {
return {
...nft,
Expand Down Expand Up @@ -1696,7 +1716,6 @@ export class NftController extends BaseControllerV1<NftConfig, NftState> {
updatedNft,
...nfts.slice(nftInfo.index + 1),
];

this.updateNestedNftState(newNfts, ALL_NFTS_STATE_KEY, {
chainId,
userAddress: selectedAddress,
Expand Down

0 comments on commit b4af134

Please sign in to comment.