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: fix detecting NFTs after closing browser #4178

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Apr 17, 2024

Explanation

This PR goes together with this extension PR MetaMask/metamask-extension#24162

We want to reduce third party calls for nft detection when a user has closed the MM popup/window.
To do that we are leveraging stopNetworkRequests and triggerNetworkrequests functions inside extension to either stop() or start() the nft detection.
While doing this fixed the following scenario:

1- User imports MM wallet
2- User enables nft detection toggle
3- User closes MM tab or popup
4- We are no longuer making calls to third party because we called stop() function on nftDetectionController.

It does not fix this scenario:
1- User imports MM wallet
2- User enabled nft detection toggle
3- User closes chrome all together
4- User reopens chrome without opening MM, and we notice that it is making calls in the background to third party even though MM is not open.

This behavior was happening because of a couple of reasons; the property disabled being set to true in defaultConfig

    this.defaultConfig = {
      interval: DEFAULT_INTERVAL,
      chainId: initialChainId,
      selectedAddress: '',
      disabled: true,
    };

made the check on this line pass


and triggered the start() function (because the useNftDetection is true).

I fixed this by having extension pass the disabled in the contructor rather than having the true value hardcoded in the controller.

I have also refactored the onPreferencesStateChange to have it make this check

      if(selectedAddress !== previouslySelectedAddress){
          this.configure({ selectedAddress });
      }

Separately.
When the user first opens the selectedAddress is '' because its also set in default config which, because of the OR statement triggered the code to also go into the start()

References

Changelog

@metamask/assets-controllers

  • ADDED: Added disabled inside contructor of nftDetectionController
  • CHANGED: Pass new disabled value instead of true in the default config
  • CHANGED: Made the check for the selected address separate from the check of useNftDetection value

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 sahar-fehri requested a review from a team as a code owner April 17, 2024 13:30
@sahar-fehri sahar-fehri merged commit 4a6cd1a into main Apr 23, 2024
139 checks passed
@sahar-fehri sahar-fehri deleted the fix/optimize-detect-nft-calls-when-browser-closed branch April 23, 2024 19:32
sahar-fehri added a commit to MetaMask/metamask-extension that referenced this pull request Apr 23, 2024
## **Description**

Once nftDetection is enabled by the user, the nftDetection controller
will start polling data in the background every 3 min even if the user
closes the metamask popup or the tab.

We want to stop unnecessary polling when MM is closed in an attempt to
reduce traffic.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24026?quickstart=1)

## **Related issues**

Related to: MetaMask/core#4178

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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-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-extension that referenced this pull request Apr 24, 2024
Once nftDetection is enabled by the user, the nftDetection controller
will start polling data in the background every 3 min even if the user
closes the metamask popup or the tab.

We want to stop unnecessary polling when MM is closed in an attempt to
reduce traffic.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24026?quickstart=1)

Related to: MetaMask/core#4178

1. Go to this page...
2.
3.

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] 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.

- [ ] 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants