-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: create nft auto detection modal and remove nft polling logic #9857
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
72d4c1b
to
e0abc2e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9857 +/- ##
==========================================
+ Coverage 47.24% 48.22% +0.97%
==========================================
Files 1370 1401 +31
Lines 33304 33865 +561
Branches 3586 3671 +85
==========================================
+ Hits 15736 16330 +594
+ Misses 16607 16526 -81
- Partials 961 1009 +48 ☔ View full report in Codecov by Sentry. |
Bitrise❌❌❌ Commit hash: c061bab Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
4 similar comments
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise🔄🔄🔄 Commit hash: 899d714 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
3 similar comments
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Bitrise❌❌❌ Commit hash: a6b3b00 Note
|
Quality Gate passedIssues Measures |
Bitrise❌❌❌ Commit hash: 899d714 Note
|
Bitrise🔄🔄🔄 Commit hash: 899d714 Note
|
Bitrise❌❌❌ Commit hash: 899d714 Note
|
1 similar comment
Bitrise❌❌❌ Commit hash: 899d714 Note
|
Bitrise✅✅✅ Commit hash: 899d714 Note
|
Bitrise🔄🔄🔄 Commit hash: 899d714 Note
|
Bitrise✅✅✅ Commit hash: 899d714 Note
|
Description
This PR removes any code that triggers polling on NftDetectionController.
Calling
detectNfts()
function is now only triggered when the user clicks on the NFT tab.It also Enables NFTDetection by default for new users.
Note
this PR has an asset-cpntroller patch from this core PR #4281
This patch keeps NFTDetection controller extending the polling controller while the PR makes it extend BaseController but it makes sure the polling is not triggered anymore in the client code.
It will be fully updated in newer versions of assets-controllers
I removed what triggers the
start()
of the polling, and now instead, mobile callsdetectNfts()
directly.Then i made updates on the fct
getOwnerNfts
so that it only fetches information for a specific cursor instead of looping through all user nfts.The loop is now being done inside the
detectNfts()
fct, and we basically fetch the first page of NFTs and save it to state so its available to clients to view.New functionalities highlight:
1- Enable NFT autodetection by default to new users.
2- Old users who have the NFT autodetection off will see the new NFT detection modal to encourage them to enable the modal.
3- When users click on the banner notice, they are no longer directed to settings and instead we enable the NFT detection after they click on “enable nft autodetection” and we show the Toast. We wanted to reduce friction and we removed that redirection to settings.
4- The code should not do any of the 3mins polling in the background anymore.
Related issues
Related: MetaMask/core#4281
Manual testing steps
Screenshots/Recordings
Before
Screen.Recording.2024-06-10.at.15.40.23.mov
After
Testing a fresh wallet import:
after-.mov
Testing an upgrade scenario:
First build on main, where you should see the NFT auto detection disabled by default if the user did not enable it
Then build on this PR:
You should see the new NFT detection modal and clicking on
allow
button should enable the NFT detection in the settings.Screen.Recording.2024-06-10.at.15.45.02.mov
NFT detection banner with new Toast:
Screen.Recording.2024-06-14.at.13.15.50.mov
Pre-merge author checklist
Pre-merge reviewer checklist