fix: NFT detection running too many times

[bergeron]

Explanation

The NftDetectionController was making unnecessary HTTP requests.

(1) It was fetching NFTs when any setting changes:

Screen.Recording.2024-02-13.at.6.11.28.PM.mov

(2) When each polling interval arrives, it can send ~7 identical requests instead of 1. See timeline:

(3) one redundant request when switching accounts

All come from onPreferencesStateChange triggering a re-start.

(2) happens because it fires ~7 times on startup from keyring related updates. This causes each to run:

this.stopPolling();
await this.detectNfts();
this.intervalId = setInterval(...)

Because detectNfts is async, intervalId is still undefined. So they each schedule their own interval without stopping a previous interval.

Fix: Only start/stop when relevant settings change.

References

Changelog

@metamask/assets-controllers

• FIXED: NFT detection running too many times

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

[mcmire]

Are there any tests we can write to ensure that this doesn't occur again?

[bergeron]

Are there any tests we can write to ensure that this doesn't occur again?

probably. I'll try to write a unit test flipping some preferences and checking how many times the HTTP route is called

[mcmire]

@bergeron Okay, cool. These tests are not in the best shape, and I'm noticing that there aren't any tests at all for when useNftDetection is set. This seems pretty important, so if you have to spend more than an hour on this, don't worry about it — we can add these tests later. From looking at the changes here, it makes sense that this would reduce the number of calls, so I'll approve it.

[bergeron]

Added a unit test that failed on the old code, and passes on the new code

[mcmire]

Glad you were able to figure it out. Nice and simple!