fix: create nft auto detection modal and remove nft polling logic (#9857

) ## **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**  ### **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 <[email protected]> Co-authored-by: metamaskbot <[email protected]> Co-authored-by: sethkfman <[email protected]> Co-authored-by: Nico MASSART <[email protected]>
MetaMask · Jun 26, 2024 · ba76567 · ba76567
1 parent 107b16f
commit ba76567
Show file tree

Hide file tree

Showing 30 changed files with 1,635 additions and 54 deletions.
diff --git a/app/actions/security/index.ts b/app/actions/security/index.ts
@@ -7,6 +7,7 @@ export enum ActionType {
   USER_SELECTED_AUTOMATIC_SECURITY_CHECKS_OPTION = 'USER_SELECTED_AUTOMATIC_SECURITY_CHECKS_OPTION',
   SET_AUTOMATIC_SECURITY_CHECKS_MODAL_OPEN = 'SET_AUTOMATIC_SECURITY_CHECKS_MODAL_OPEN',
   SET_DATA_COLLECTION_FOR_MARKETING = 'SET_DATA_COLLECTION_FOR_MARKETING',
+  SET_NFT_AUTO_DETECTION_MODAL_OPEN = 'SET_NFT_AUTO_DETECTION_MODAL_OPEN',
 }
 
 export interface AllowLoginWithRememberMeUpdated
@@ -29,6 +30,11 @@ export interface SetAutomaticSecurityChecksModalOpen
   open: boolean;
 }
 
+export interface SetNftAutoDetectionModalOpen
+  extends ReduxAction<ActionType.SET_NFT_AUTO_DETECTION_MODAL_OPEN> {
+  open: boolean;
+}
+
 export interface SetDataCollectionForMarketing
   extends ReduxAction<ActionType.SET_DATA_COLLECTION_FOR_MARKETING> {
   enabled: boolean;
@@ -39,7 +45,8 @@ export type Action =
   | AutomaticSecurityChecks
   | UserSelectedAutomaticSecurityChecksOptions
   | SetAutomaticSecurityChecksModalOpen
-  | SetDataCollectionForMarketing;
+  | SetDataCollectionForMarketing
+  | SetNftAutoDetectionModalOpen;
 
 export const setAllowLoginWithRememberMe = (
   enabled: boolean,
@@ -68,6 +75,13 @@ export const setAutomaticSecurityChecksModalOpen = (
   open,
 });
 
+export const setNftAutoDetectionModalOpen = (
+  open: boolean,
+): SetNftAutoDetectionModalOpen => ({
+  type: ActionType.SET_NFT_AUTO_DETECTION_MODAL_OPEN,
+  open,
+});
+
 export const setDataCollectionForMarketing = (enabled: boolean) => ({
   type: ActionType.SET_DATA_COLLECTION_FOR_MARKETING,
   enabled,

diff --git a/app/actions/security/state.ts b/app/actions/security/state.ts
@@ -3,6 +3,7 @@ export interface SecuritySettingsState {
   automaticSecurityChecksEnabled: boolean;
   hasUserSelectedAutomaticSecurityCheckOption: boolean;
   isAutomaticSecurityChecksModalOpen: boolean;
+  isNFTAutoDetectionModalViewed: boolean;
   // 'null' represents the user not having set his preference over dataCollectionForMarketing yet
   dataCollectionForMarketing: boolean | null;
 }
diff --git a/app/component-library/components/Toast/Toast.tsx b/app/component-library/components/Toast/Toast.tsx
@@ -187,6 +187,18 @@ const Toast = forwardRef((_, ref: React.ForwardedRef<ToastRef>) => {
           />
         );
       }
+      case ToastVariants.Icon: {
+        const { iconName, iconColor, backgroundColor } = toastOptions;
+        return (
+          <Avatar
+            variant={AvatarVariant.Icon}
+            name={iconName}
+            iconColor={iconColor}
+            backgroundColor={backgroundColor}
+            style={styles.avatar}
+          />
+        );
+      }
     }
   };
 

diff --git a/app/component-library/components/Toast/Toast.types.ts b/app/component-library/components/Toast/Toast.types.ts
@@ -12,6 +12,7 @@ export enum ToastVariants {
   Plain = 'Plain',
   Account = 'Account',
   Network = 'Network',
+  Icon = 'Icon',
 }
 
 /**
@@ -65,13 +66,21 @@ interface NetworkToastOption extends BaseToastVariants {
   networkImageSource: ImageSourcePropType;
 }
 
+interface IconToastOption extends BaseToastVariants {
+  variant: ToastVariants.Icon;
+  iconName?: string;
+  iconColor?: string;
+  backgroundColor?: string;
+}
+
 /**
  * Different toast options combined in a union type.
  */
 export type ToastOptions =
   | PlainToastOption
   | AccountToastOption
-  | NetworkToastOption;
+  | NetworkToastOption
+  | IconToastOption;
 
 /**
  * Toast component reference.

diff --git a/app/components/Nav/App/index.js b/app/components/Nav/App/index.js
@@ -110,6 +110,7 @@ import OnboardingSuccess from '../../Views/OnboardingSuccess';
 import DefaultSettings from '../../Views/OnboardingSuccess/DefaultSettings';
 import BasicFunctionalityModal from '../../UI/BasicFunctionality/BasicFunctionalityModal/BasicFunctionalityModal';
 import SmartTransactionsOptInModal from '../../Views/SmartTransactionsOptInModal/SmartTranactionsOptInModal';
+import NFTAutoDetectionModal from '../../../../app/components/Views/NFTAutoDetectionModal/NFTAutoDetectionModal';
 
 const clearStackNavigatorOptions = {
   headerShown: false,
@@ -693,6 +694,10 @@ const App = ({ userLoggedIn }) => {
         name={Routes.SHEET.SHOW_NFT_DISPLAY_MEDIA}
         component={ShowDisplayNftMediaSheet}
       />
+      <Stack.Screen
+        name={Routes.MODAL.NFT_AUTO_DETECTION_MODAL}
+        component={NFTAutoDetectionModal}
+      />
     </Stack.Navigator>
   );
 

diff --git a/app/components/UI/CollectibleContracts/constants.ts b/app/components/UI/CollectibleContracts/constants.ts
@@ -1,3 +1,2 @@
-const RefreshTestId = 'refreshControl';
-
-export default RefreshTestId;
+export const RefreshTestId = 'refreshControl';
+export const SpinnerTestId = 'spinner';
diff --git a/app/components/UI/CollectibleContracts/index.js b/app/components/UI/CollectibleContracts/index.js
@@ -8,6 +8,7 @@ import {
   Platform,
   FlatList,
   RefreshControl,
+  ActivityIndicator,
 } from 'react-native';
 import { connect } from 'react-redux';
 import { fontStyles } from '../../../styles/common';
@@ -19,6 +20,7 @@ import {
   collectibleContractsSelector,
   collectiblesSelector,
   favoritesCollectiblesSelector,
+  isNftFetchingProgressSelector,
 } from '../../../reducers/collectibles';
 import { removeFavoriteCollectible } from '../../../actions/collectibles';
 import Text from '../../Base/Text';
@@ -44,7 +46,7 @@ import {
   NFT_TAB_CONTAINER_ID,
 } from '../../../../wdio/screen-objects/testIDs/Screens/WalletView.testIds';
 import { useMetrics } from '../../../components/hooks/useMetrics';
-import RefreshTestId from './constants';
+import { RefreshTestId, SpinnerTestId } from './constants';
 
 const createStyles = (colors) =>
   StyleSheet.create({
@@ -87,6 +89,9 @@ const createStyles = (colors) =>
       marginBottom: 8,
       fontSize: 14,
     },
+    spinner: {
+      marginBottom: 8,
+    },
   });
 
 /**
@@ -100,6 +105,7 @@ const CollectibleContracts = ({
   navigation,
   collectibleContracts,
   collectibles: allCollectibles,
+  isNftFetchingProgress,
   favoriteCollectibles,
   removeFavoriteCollectible,
   useNftDetection,
@@ -219,6 +225,14 @@ const CollectibleContracts = ({
   const renderFooter = useCallback(
     () => (
       <View style={styles.footer} key={'collectible-contracts-footer'}>
+        {isNftFetchingProgress ? (
+          <ActivityIndicator
+            size="large"
+            style={styles.spinner}
+            testID={SpinnerTestId}
+          />
+        ) : null}
+
         <Text style={styles.emptyText}>
           {strings('wallet.no_collectibles')}
         </Text>
@@ -233,7 +247,7 @@ const CollectibleContracts = ({
         </TouchableOpacity>
       </View>
     ),
-    [goToAddCollectible, isAddNFTEnabled, styles],
+    [goToAddCollectible, isAddNFTEnabled, styles, isNftFetchingProgress],
   );
 
   const renderCollectibleContract = useCallback(
@@ -282,7 +296,7 @@ const CollectibleContracts = ({
         NftDetectionController.detectNfts(),
         NftController.checkAndUpdateAllNftsOwnershipStatus(),
       ];
-      await Promise.all(actions);
+      await Promise.allSettled(actions);
       setRefreshing(false);
     });
   }, [setRefreshing]);
@@ -322,7 +336,7 @@ const CollectibleContracts = ({
           <>
             {isCollectionDetectionBannerVisible && (
               <View style={styles.emptyView}>
-                <CollectibleDetectionModal navigation={navigation} />
+                <CollectibleDetectionModal />
               </View>
             )}
             {renderFavoriteCollectibles()}
@@ -355,7 +369,6 @@ const CollectibleContracts = ({
       renderFooter,
       renderEmpty,
       isCollectionDetectionBannerVisible,
-      navigation,
       styles.emptyView,
     ],
   );
@@ -391,7 +404,11 @@ CollectibleContracts.propTypes = {
    * Array of collectibles objects
    */
   collectibles: PropTypes.array,
-
+  /**
+   * boolean indicating if fetching status is
+   * still in progress
+   */
+  isNftFetchingProgress: PropTypes.bool,
   /**
    * Navigation object required to push
    * the Asset detail view
@@ -426,6 +443,7 @@ const mapStateToProps = (state) => ({
   useNftDetection: selectUseNftDetection(state),
   collectibleContracts: collectibleContractsSelector(state),
   collectibles: collectiblesSelector(state),
+  isNftFetchingProgress: isNftFetchingProgressSelector(state),
   favoriteCollectibles: favoritesCollectiblesSelector(state),
   isIpfsGatewayEnabled: selectIsIpfsGatewayEnabled(state),
   displayNftMedia: selectDisplayNftMedia(state),

diff --git a/app/components/UI/CollectibleContracts/index.test.tsx b/app/components/UI/CollectibleContracts/index.test.tsx
@@ -495,4 +495,124 @@ describe('CollectibleContracts', () => {
     expect(spyOnUpdateNftMetadata).toHaveBeenCalledTimes(0);
     expect(spyOnDetectNfts).toHaveBeenCalledTimes(1);
   });
+
+  it('shows spinner if nfts are still being fetched', async () => {
+    const CURRENT_ACCOUNT = '0x1a';
+    const mockState = {
+      collectibles: {
+        favorites: {},
+        isNftFetchingProgress: true,
+      },
+      engine: {
+        backgroundState: {
+          ...initialBackgroundState,
+          NetworkController: {
+            network: '1',
+            providerConfig: {
+              ticker: 'ETH',
+              type: 'mainnet',
+              chainId: '1',
+            },
+          },
+          AccountTrackerController: {
+            accounts: { [CURRENT_ACCOUNT]: { balance: '0' } },
+          },
+          PreferencesController: {
+            useNftDetection: true,
+            displayNftMedia: true,
+            selectedAddress: CURRENT_ACCOUNT,
+            identities: {
+              [CURRENT_ACCOUNT]: {
+                address: CURRENT_ACCOUNT,
+                name: 'Account 1',
+              },
+            },
+          },
+          NftController: {
+            addNft: jest.fn(),
+            updateNftMetadata: jest.fn(),
+            allNfts: {
+              [CURRENT_ACCOUNT]: {
+                '1': [],
+              },
+            },
+            allNftContracts: {
+              [CURRENT_ACCOUNT]: {
+                '1': [],
+              },
+            },
+          },
+          NftDetectionController: {
+            detectNfts: jest.fn(),
+          },
+        },
+      },
+    };
+    const { queryByTestId } = renderWithProvider(<CollectibleContracts />, {
+      state: mockState,
+    });
+
+    const spinner = queryByTestId('spinner');
+    expect(spinner).not.toBeNull();
+  });
+
+  it('Does not show spinner if nfts are not still being fetched', async () => {
+    const CURRENT_ACCOUNT = '0x1a';
+    const mockState = {
+      collectibles: {
+        favorites: {},
+      },
+      engine: {
+        backgroundState: {
+          ...initialBackgroundState,
+          NetworkController: {
+            network: '1',
+            providerConfig: {
+              ticker: 'ETH',
+              type: 'mainnet',
+              chainId: '1',
+            },
+          },
+          AccountTrackerController: {
+            accounts: { [CURRENT_ACCOUNT]: { balance: '0' } },
+          },
+          PreferencesController: {
+            useNftDetection: true,
+            displayNftMedia: true,
+            selectedAddress: CURRENT_ACCOUNT,
+            identities: {
+              [CURRENT_ACCOUNT]: {
+                address: CURRENT_ACCOUNT,
+                name: 'Account 1',
+              },
+            },
+          },
+          NftController: {
+            addNft: jest.fn(),
+            updateNftMetadata: jest.fn(),
+            allNfts: {
+              [CURRENT_ACCOUNT]: {
+                '1': [],
+              },
+            },
+            allNftContracts: {
+              [CURRENT_ACCOUNT]: {
+                '1': [],
+              },
+            },
+          },
+          NftDetectionController: {
+            detectNfts: jest.fn(),
+          },
+        },
+      },
+    };
+
+    const { queryByTestId } = renderWithProvider(<CollectibleContracts />, {
+      state: mockState,
+    });
+
+    const spinner = queryByTestId('spinner');
+    expect(spinner).toBeNull();
+  });
 });