feat: remove nft detection polling #4281
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
sahar-fehri
force-pushed
the
feat/remove-nft-detection-polling
branch
from
fc291e4
to
e4d1afd
Compare
7 tasks
sahar-fehri
force-pushed
the
feat/remove-nft-detection-polling
branch
from
e4d1afd
to
c58add7
Compare
cryptodev-2s
reviewed
Jun 7, 2024
| */ | ||
| export class NftDetectionController extends StaticIntervalPollingController< | ||
| export class NftDetectionController extends BaseController< |
There was a problem hiding this comment.
@sahar-fehri what do you think about making this a service instead as it's stateless ?
There was a problem hiding this comment.
Resolving this now because we discussed that we can iterate in the future to extract api interactions into a separate service.
There was a problem hiding this comment.
For future reference, I believe modeling stateless services as non-controllers is a separate concern from extracting external API integrations.
We probably want to create a ticket for the first task if it's out of scope for this PR.
bergeron
reviewed
Jun 10, 2024
bergeron
reviewed
Jun 10, 2024
Merged
7 tasks
bergeron
reviewed
Jun 10, 2024
bergeron
previously approved these changes
Jun 11, 2024
MajorLift
reviewed
Jun 11, 2024
MajorLift
reviewed
Jun 11, 2024
bergeron
approved these changes
Jun 12, 2024
MajorLift
reviewed
Jun 12, 2024
Comment on lines
+561
to
+562
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
| const updateKey: `${Hex}:${string}` = `${chainId}:${userAddress}`; |
There was a problem hiding this comment.
@sahar-fehri Edit: This is for my reference, don't worry about it!
This eslint-disable directive should be removable with the next @metamask/keyring-api release.
- Currently,
userAddressis resolving toany. - This is because the
AccountsController:getSelectedAccountaction returnsInternalAccount, - which is defined using
superstructinstead of@metamask/superstructin current the@metamask/keyring-apiversion. - Fix ESM/CJS compatibility superstruct#1
Similar issue: MetaMask/eth-snap-keyring#323 (comment)
mcmire
pushed a commit
that referenced
this pull request
Jun 12, 2024
## Explanation This PR removes polling logic from NftDetectionController. Calling detectNfts function will be tied to UI components instead of preference change events. NftDetectionController will now extend `BaseController` instead of `StaticIntervalPollingController`. The detectNfts logic has also changed to call the NFT-API and save the nfts in state as soon as they are available instead of waiting to fetch all the user NFTs before saving to state. ## References * Related to [67890](MetaMask/metamask-extension#24547) ## 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` - **BREAKING**: Changed NftDetectionController to extend `BaseController`. - **REMOVED**: Removed `interval` from contructor in `NftDetectionController`. ## 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
sahar-fehri
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Jun 19, 2024
## **Description** This PR aims to: 1- Enable nft autodetection by default 2- Show a modal only once if the user disables the nft autodetection 3- Make NFT detection tied to the components that use the NFTs instead of the 3mins polling strategy. This PR goes with this core PR: MetaMask/core#4281 [](https://codespaces.new/MetaMask/metamask-extension/pull/24547?quickstart=1) ## **Related issues** Related to: MetaMask/core#4281 ## **Manual testing steps** **Test the nft auto detection modal:** 1. Do a fresh import of the wallet 2. Switch to mainnet. 3. Go to settings => Security and privacy and make sure the toggle enable NFT auto detection is ON 4. Turn the nft auto detection toggle OFF 5. Go back to home page and you should see a new modal 6. Clicking on `not right now `button should close the modal and closing on `allow` button should enable the nft auto detection modal. This modal should be seen only once. **Test the removal of the polling in the backgound:** We have the toggle enabled by default now but this should not trigger calls to NFT-API every 3 mins anymore. Instead the calls should be triggered only when you click on the NFT tab. 1. Open the background console and click on Networks tab. 2. Filter by /tokens (so you are able to see only the calls that will fetch user nfts) 3. Notice that as long as you did not click on the NFT tab, you should not be able to see any calls made in the backround. (where the old logic should keep detecting your NFTs every 3 mins as long as you have MM open) 5. Click on the NFT tab and you should be able to see the requests in the background to fetch your nfts. 7. You can also click on Send, and click on asset picker and click on NFT tab, you should be able to see your NFTs there too. 8. Calls should be made only when you click on the NFT tab. **Test new notice banner behavior:** Users should see the NFT Notice banner as long as they are on mainnet + they have NFT detection OFF. Regardless of whether they have NFTs in the state or not. Clicking on the"Enable NFT autodetection" should remove the notice banner and enable the NFT detection without redirecting the user to settings. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> https://github.com/MetaMask/metamask-extension/assets/10994169/0815ac0c-a60c-400a-9ffe-92451dfc3ded ### **After** <!-- [screenshots/recordings] --> https://github.com/MetaMask/metamask-extension/assets/10994169/0b00f1c1-baf1-4205-bca2-991d11e39a2f Notice banner with toast: https://github.com/MetaMask/metamask-extension/assets/10994169/c1aaa168-a29f-4b38-b0c7-74aaa1945ba0 ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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.
sahar-fehri
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 26, 2024
) ## **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](MetaMask/core#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 calls `detectNfts()` 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** 1. Import MM application 2. Go to settings => Security and privacy and you should see that the NFT detection is enabled. 3. Go back to home page and click on NFT tab; you should be able to see your NFTs being added. 4. Switch to another account that has NFTs 5. Click on NFTs tab and you should also see your NFTs being added to sate. 6. Go to settings => Security and privacy and turn OFF the NFT detection toggle. 7. You should see a new modal. Clicking on "allow" button should re-enable the NFT detection toggle. ## **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/83e86852-3455-4b7c-bfc6-a658dc20c1b0 ### **After** Testing a fresh wallet import: https://github.com/MetaMask/metamask-mobile/assets/10994169/331668b1-3a27-493f-a648-3568e4ba67c2 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. https://github.com/MetaMask/metamask-mobile/assets/10994169/6bcb77f5-13ef-429e-a1f5-4d907145ee30 NFT detection banner with new Toast: https://github.com/MetaMask/metamask-mobile/assets/10994169/2f7465c0-4c42-409b-bfe4-ed70d9e1ca2c ## **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. --------- Co-authored-by: MetaMask Bot <37885440+metamaskbot@users.noreply.github.com> Co-authored-by: metamaskbot <metamaskbot@users.noreply.github.com> Co-authored-by: sethkfman <seth.kaufman@consensys.net> Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
Jonathansoufer
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 27, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/24547?quickstart=1){kind=link}
) 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 > 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 calls `detectNfts()` 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: MetaMask/core#4281 1. Import MM application 2. Go to settings => Security and privacy and you should see that the NFT detection is enabled. 3. Go back to home page and click on NFT tab; you should be able to see your NFTs being added. 4. Switch to another account that has NFTs 5. Click on NFTs tab and you should also see your NFTs being added to sate. 6. Go to settings => Security and privacy and turn OFF the NFT detection toggle. 7. You should see a new modal. Clicking on "allow" button should re-enable the NFT detection toggle. <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> https://github.com/MetaMask/metamask-mobile/assets/10994169/83e86852-3455-4b7c-bfc6-a658dc20c1b0 Testing a fresh wallet import: https://github.com/MetaMask/metamask-mobile/assets/10994169/331668b1-3a27-493f-a648-3568e4ba67c2 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. https://github.com/MetaMask/metamask-mobile/assets/10994169/6bcb77f5-13ef-429e-a1f5-4d907145ee30 NFT detection banner with new Toast: https://github.com/MetaMask/metamask-mobile/assets/10994169/2f7465c0-4c42-409b-bfe4-ed70d9e1ca2c - [ ] 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. - [ ] 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. --------- Co-authored-by: MetaMask Bot <37885440+metamaskbot@users.noreply.github.com> Co-authored-by: metamaskbot <metamaskbot@users.noreply.github.com> Co-authored-by: sethkfman <seth.kaufman@consensys.net> Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
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 removes polling logic from NftDetectionController.
Calling detectNfts function will be tied to UI components instead of preference change events.
NftDetectionController will now extend
BaseControllerinstead ofStaticIntervalPollingController.The detectNfts logic has also changed to call the NFT-API and save the nfts in state as soon as they are available instead of waiting to fetch all the user NFTs before saving to state.
References
Changelog
@metamask/assets-controllersBaseController.intervalfrom contructor inNftDetectionController.Checklist