fix: avoid multiple calls to refresh nft metadata #4325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
sahar-fehri
force-pushed
the
fix/avoid-unnecessary-nft-metadata-update-calls
branch
from
e8b5283
to
8a95926
Compare
sahar-fehri
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 5, 2024
## **Description**
The CollectibleContracts component was making more calls to the NFT API
than it should.
Users could either have the nft detection enabled or disabled.
When the nft detection is enabled, the component was:
1- Executing `detectNfts` function which triggers one call to get all
nfts for a user
2- Calling `updateNftMetadata` which triggers `N` number of calls to the
NFT-API assuming the user has `N` NFT.
The second call is unnecessary if the user already have the nftDetection
enabled.
In this fix, i added a check in the useEffect to see if the user has the
nftDetection enabled or not
```
if (updatableCollectibles.length !== 0 && !useNftDetection) {
updateAllCollectibleMetadata(updatableCollectibles);
}
```
If it is already enabled, calling the detectNfts function should be
enough to refresh metadata.
When the user has the nftDetection Disabled, in this case the
`updateNftMetadata` will be executed.
I made an update in the `updateNftMetadata` fct to not execute the calls
every time the user pulls down t refresh. A 10 mins interval should be
enough.
Note: We will have an update on the nftDetectionController in the future
to update the polling strategy.
## **Related issues**
- Compare url on core:
https://github.com/MetaMask/core/compare/patch/mobile-assets-controllers-26...patch/mobile-assets-controllers-26-optimize-nft-refresh-calls?expand=1
- MetaMask/core#4325
## **Manual testing steps**
To check the unecessary calls:
1. Build on main
2. Go to app/components/UI/CollectibleContracts/index.js and add a log
inside useEffect
3. Go to node-modules, nftDetectionController.js function getOwnerNfts
and add a console.log
4. Go to node-modules, nftController.js function
getNftInformationFromApi and add a log
5. When you have nft detection disabled, go to nft tab, and pull down to
refresh and check the calls made in your terminal
6. Enable nft detection, go to nft tab, pull down to refresh and check
your logs
(See before video, notice on that wallet i have 5 nfts, it should be
making only 1 call per nft => 5 calls in total but its making more than
that. Every state update will result in the useEffect being executed
which we attempt to avoid in this fix)
To check the update on this PR:
1- Build on this branch
2. Go to app/components/UI/CollectibleContracts/index.js and add a log
inside useEffect
3. Go to node-modules, nftDetectionController.js function getOwnerNfts
and add a console.log
4. Go to node-modules, nftController.js function
getNftInformationFromApi and add a log
5. When you have nft detection disabled, go to nft tab, and pull down to
refresh and check the calls made in your terminal
6. Enable nft detection, go to nft tab, pull down to refresh and check
your logs
## **Screenshots/Recordings**
<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->
### **Before**
https://github.com/MetaMask/metamask-mobile/assets/10994169/77f7ba9e-add9-4ce8-ad78-bc41926276b9
### **After**
https://github.com/MetaMask/metamask-mobile/assets/10994169/4677a899-9fdd-46a3-98e3-9422d6fa9eb9
## **Pre-merge author checklist**
- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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.
bergeron
reviewed
Jun 5, 2024
bergeron
reviewed
Jun 5, 2024
sahar-fehri
force-pushed
the
fix/avoid-unnecessary-nft-metadata-update-calls
branch
from
da4b896
to
abc8312
Compare
bergeron
approved these changes
Jun 5, 2024
|
The PR description can be updated since the 10 minute check is no longer added |
7 tasks
sahar-fehri
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 6, 2024
## **Description** Updates a a patch for assets controllers based on a small update made on this PR MetaMask/core#4325 ## **Related issues** Related: MetaMask/core#4325 Related: #9759 ## **Manual testing steps** Same steps on this #9759 ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR updates the logic of
updateNftMetadatafunction and adds a mutex to synchronize state updates.Inside
getNftInformationfunction, before returning the image, we prioritize checking for api result then fallback to checking if there was an image in the blockchain result.The nft detection will be enabled by default in the future, this will avoid making unnecessary state updates when the image string returned from NFT-API is different than the string returned from
blockchainMetadata.References
Changelog
@metamask/assets-controllersupdateNftMetadatafunction.Checklist