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: add thresshold for update NFT metadata #5134

Merged
merged 12 commits into from
Jan 16, 2025

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Jan 13, 2025

Explanation

When onPreferencesControllerStateChange is triggered inside nftController, it also triggers a check if nfts saved in state needs to be updated.
We keep retrying to update NFTs (fetching their metadata) when they have no image, name or description.

Note: When switching newtworks; this line is the trigger for the onPreferencesControllerStateChange.

While the updateNftUpdateForAccount should be triggered once for NFTs that actually have a (name, description, image)
We will keep retrying for NFTs that don't. (Could be nfts created for testing purposes or some raffle tickets do not have those fields).

We had a user reporting that he is seeing an excessive amount of RPC calls made on one of his accounts on Arbitrum Sepolia. This account holds more than a 1000 NFT. Every time he switches the network to Arbitrum Sepolia, he sees more than a 1000 eth_call made.

That was the result of updateNftUpdateForAccount being triggered everytime the network is switched + The nfts not having (name,image, description)

This PR adds a 500 threshold to avoid sending an excessive amount of RPC calls to fetch nft metadata and adds a check if the triggered onPreferencesControllerStateChange updates any of the concerned params ( ipfsGateway, openseaEnabled, isIpfsGatewayEnabled)

Better improvement:

A better improvement for this would be to add support for BalanceScanner contract, which allows to fetch tokenURI for multiple addresses and tokenIds with fct tokenURIsofTokenIdsForTokens.

We will control how often this is refreshed. For users that have nfts > NFT_UPDATE_THRESHOLD, we save the timestamp of the first update in a mapping chainId => lastTimestamp of updateNftMetadata, and only update after a specific period of time NFT_UPDATE_REFRESH_PERIOD = 48hours.

References

Changelog

@metamask/controller-utils

  • ADDED: Added NFT_UPDATE_THRESHOLD constant

@metamask/assets-controllers

  • CHANGED: Added a check if the nftsToUpdate is less than NFT_UPDATE_THRESHOLD inside updateNftUpdateForAccount

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@sahar-fehri sahar-fehri marked this pull request as ready for review January 13, 2025 11:20
@sahar-fehri sahar-fehri requested review from a team as code owners January 13, 2025 11:20
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

The code itself looks good as it achieves what's stated in the PR description, though I feel like firing 500 eth_call requests every time users switch selected addresses or preferences are updated sounds still exaggerated.

For instance, is there a reason for requiring updates on NFTs on every preference change?

@sahar-fehri
Copy link
Contributor Author

  • updateNftUpdateForAccount

Thanks @mikesposito ! That's right, i updated the code so it checks before doing the update if at least one of the concerned params has been updated as part of the preference update (ipfsGateway, openSeaEnabled or isIpfsGatewayEnabled)

if (
this.#ipfsGateway !== ipfsGateway ||
this.#openSeaEnabled !== openSeaEnabled ||
this.#isIpfsGatewayEnabled !== isIpfsGatewayEnabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this so that even if this gets triggered for any reason; we do not proceed to update the nftmetadata unless one of the concerned params has changed

gambinish
gambinish previously approved these changes Jan 13, 2025
bergeron
bergeron previously approved these changes Jan 15, 2025
mikesposito
mikesposito previously approved these changes Jan 16, 2025
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

Looks good!

packages/assets-controllers/src/NftController.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@sahar-fehri sahar-fehri merged commit 21c4ec6 into main Jan 16, 2025
117 checks passed
@sahar-fehri sahar-fehri deleted the fix/add-threshold-for-update-nft-metadata-fct branch January 16, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants