From 475e9bfe12e25ef9183a1170a5d6510e2cad608a Mon Sep 17 00:00:00 2001 From: tommasini <46944231+tommasini@users.noreply.github.com> Date: Wed, 23 Oct 2024 22:37:38 +0100 Subject: [PATCH 1/7] chore: add tags to custom spans (#11977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Cherry-picked this [PR](https://github.com/MetaMask/metamask-mobile/pull/11623) Android build: https://app.bitrise.io/build/3cf918b6-a0b8-4a46-a3e8-78ebd74b9006 ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile 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: Aslau Mario-Daniel --- app/selectors/nftController.ts | 13 +- app/selectors/tokensController.ts | 21 +++ app/store/index.ts | 2 + app/util/sentry/tags/index.test.ts | 229 +++++++++++++++++++++++++++++ app/util/sentry/tags/index.ts | 30 ++++ app/util/trace.test.ts | 54 +++++-- app/util/trace.ts | 141 ++++++++++++++++-- e2e/fixtures/fixture-builder.js | 12 ++ 8 files changed, 470 insertions(+), 32 deletions(-) create mode 100644 app/util/sentry/tags/index.test.ts create mode 100644 app/util/sentry/tags/index.ts diff --git a/app/selectors/nftController.ts b/app/selectors/nftController.ts index a2a3a669074..b6a217f727a 100644 --- a/app/selectors/nftController.ts +++ b/app/selectors/nftController.ts @@ -1,5 +1,5 @@ import { createSelector } from 'reselect'; -import { NftControllerState } from '@metamask/assets-controllers'; +import { Nft, NftControllerState } from '@metamask/assets-controllers'; import { RootState } from '../reducers'; const selectNftControllerState = (state: RootState) => @@ -15,3 +15,14 @@ export const selectAllNfts = createSelector( selectNftControllerState, (nftControllerState: NftControllerState) => nftControllerState.allNfts, ); + +export const selectAllNftsFlat = createSelector( + selectAllNfts, + (nftsByChainByAccount) => { + const nftsByChainArray = Object.values(nftsByChainByAccount); + return nftsByChainArray.reduce((acc, nftsByChain) => { + const nftsArrays = Object.values(nftsByChain); + return acc.concat(...nftsArrays); + }, [] as Nft[]); + }, +); diff --git a/app/selectors/tokensController.ts b/app/selectors/tokensController.ts index f63f4c857a6..ccebe64ebfb 100644 --- a/app/selectors/tokensController.ts +++ b/app/selectors/tokensController.ts @@ -37,3 +37,24 @@ export const selectDetectedTokens = createSelector( (tokensControllerState: TokensControllerState) => tokensControllerState?.detectedTokens, ); + +const selectAllTokens = createSelector( + selectTokensControllerState, + (tokensControllerState: TokensControllerState) => + tokensControllerState?.allTokens, +); + +export const selectAllTokensFlat = createSelector( + selectAllTokens, + (tokensByAccountByChain) => { + if (Object.values(tokensByAccountByChain).length === 0) { + return []; + } + const tokensByAccountArray = Object.values(tokensByAccountByChain); + + return tokensByAccountArray.reduce((acc, tokensByAccount) => { + const tokensArray = Object.values(tokensByAccount); + return acc.concat(...tokensArray); + }, [] as Token[]); + }, +); diff --git a/app/store/index.ts b/app/store/index.ts index b1b29f63dd7..a96eb65823e 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -15,6 +15,7 @@ import thunk from 'redux-thunk'; import persistConfig from './persistConfig'; import { AppStateEventProcessor } from '../core/AppStateEventListener'; +import { getTraceTags } from '../util/sentry/tags'; // TODO: Improve type safety by using real Action types instead of `any` // TODO: Replace "any" with type @@ -104,6 +105,7 @@ const createStoreAndPersistor = async () => { { name: TraceName.EngineInitialization, op: TraceOperation.EngineInitialization, + tags: getTraceTags(store.getState()), }, () => { EngineService.initalizeEngine(store); diff --git a/app/util/sentry/tags/index.test.ts b/app/util/sentry/tags/index.test.ts new file mode 100644 index 00000000000..5511ab7cf92 --- /dev/null +++ b/app/util/sentry/tags/index.test.ts @@ -0,0 +1,229 @@ +import { RootState } from '../../../reducers'; +import { getTraceTags } from './'; +import initialRootState, { + backgroundState, +} from '../../../util/test/initial-root-state'; +import { userInitialState } from '../../../reducers/user'; +import { createMockAccountsControllerState } from '../../../util/test/accountsControllerTestUtils'; + +describe('Tags Utils', () => { + beforeEach(() => { + jest.resetAllMocks(); + }); + + describe('getTraceTags', () => { + it('includes if unlocked', () => { + const state = { + ...initialRootState, + user: { ...userInitialState, userLoggedIn: true }, + }; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.unlocked']).toStrictEqual(true); + }); + + it('includes if not unlocked', () => { + const state = { + ...initialRootState, + user: { ...userInitialState, userLoggedIn: false }, + }; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.unlocked']).toStrictEqual(false); + }); + + it('includes pending approval type', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + ApprovalController: { + ...backgroundState.ApprovalController, + pendingApprovals: { + 1: { + type: 'eth_sendTransaction', + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.pending_approval']).toStrictEqual( + 'eth_sendTransaction', + ); + }); + + it('includes first pending approval type if multiple', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + + ApprovalController: { + ...backgroundState.ApprovalController, + pendingApprovals: { + 1: { + type: 'eth_sendTransaction', + }, + 2: { + type: 'personal_sign', + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.pending_approval']).toStrictEqual( + 'eth_sendTransaction', + ); + }); + + it('includes account count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + AccountsController: createMockAccountsControllerState([ + '0x1234', + '0x4321', + ]), + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.account_count']).toStrictEqual(2); + }); + + it('includes nft count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + NftController: { + ...backgroundState.NftController, + allNfts: { + '0x1234': { + '0x1': [ + { + tokenId: '1', + }, + { + tokenId: '2', + }, + ], + '0x2': [ + { + tokenId: '3', + }, + { + tokenId: '4', + }, + ], + }, + '0x4321': { + '0x3': [ + { + tokenId: '5', + }, + ], + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.nft_count']).toStrictEqual(5); + }); + + it('includes notification count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + NotificationServicesController: { + metamaskNotificationsList: [{}, {}, {}], + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.notification_count']).toStrictEqual(3); + }); + + it('includes token count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + TokensController: { + allTokens: { + '0x1': { + '0x1234': [{}, {}], + '0x4321': [{}], + }, + '0x2': { + '0x5678': [{}], + }, + }, + }, + }, + }, + } as unknown as RootState; + + const tags = getTraceTags(state); + + expect(tags?.['wallet.token_count']).toStrictEqual(4); + }); + + it('includes transaction count', () => { + const state = { + ...initialRootState, + engine: { + backgroundState: { + ...backgroundState, + TransactionController: { + transactions: [ + { + id: 1, + chainId: '0x1', + }, + { + id: 2, + chainId: '0x1', + }, + { + id: 3, + chainId: '0x2', + }, + ], + }, + }, + }, + } as unknown as RootState; + const tags = getTraceTags(state); + + expect(tags?.['wallet.transaction_count']).toStrictEqual(3); + }); + }); +}); diff --git a/app/util/sentry/tags/index.ts b/app/util/sentry/tags/index.ts new file mode 100644 index 00000000000..796bb9212fe --- /dev/null +++ b/app/util/sentry/tags/index.ts @@ -0,0 +1,30 @@ +import { RootState } from '../../../reducers'; +import { selectAllNftsFlat } from '../../../selectors/nftController'; +import { selectInternalAccounts } from '../../../selectors/accountsController'; +import { selectAllTokensFlat } from '../../../selectors/tokensController'; +import { getNotificationsList } from '../../../selectors/notifications'; +import { selectTransactions } from '../../../selectors/transactionController'; +import { selectPendingApprovals } from '../../../selectors/approvalController'; + +export function getTraceTags(state: RootState) { + if (!Object.keys(state?.engine?.backgroundState).length) return; + const unlocked = state.user.userLoggedIn; + const accountCount = selectInternalAccounts(state).length; + const nftCount = selectAllNftsFlat(state).length; + const notificationCount = getNotificationsList(state).length; + const tokenCount = selectAllTokensFlat(state).length; + const transactionCount = selectTransactions(state).length; + const pendingApprovals = Object.values(selectPendingApprovals(state)); + + const firstApprovalType = pendingApprovals?.[0]?.type; + + return { + 'wallet.account_count': accountCount, + 'wallet.nft_count': nftCount, + 'wallet.notification_count': notificationCount, + 'wallet.pending_approval': firstApprovalType, + 'wallet.token_count': tokenCount, + 'wallet.transaction_count': transactionCount, + 'wallet.unlocked': unlocked, + }; +} diff --git a/app/util/trace.test.ts b/app/util/trace.test.ts index cb90722c9cb..b9e541ebdc5 100644 --- a/app/util/trace.test.ts +++ b/app/util/trace.test.ts @@ -1,17 +1,19 @@ -import { startSpan, startSpanManual, withScope } from '@sentry/react-native'; +import { + Scope, + setMeasurement, + startSpan, + startSpanManual, + withScope, +} from '@sentry/react-native'; import { Span } from '@sentry/types'; -import { - endTrace, - trace, - TraceName, - TRACES_CLEANUP_INTERVAL, -} from './trace'; +import { endTrace, trace, TraceName, TRACES_CLEANUP_INTERVAL } from './trace'; jest.mock('@sentry/react-native', () => ({ withScope: jest.fn(), startSpan: jest.fn(), startSpanManual: jest.fn(), + setMeasurement: jest.fn(), })); const NAME_MOCK = TraceName.Middleware; @@ -36,15 +38,23 @@ describe('Trace', () => { const startSpanMock = jest.mocked(startSpan); const startSpanManualMock = jest.mocked(startSpanManual); const withScopeMock = jest.mocked(withScope); - const setTagsMock = jest.fn(); + const setMeasurementMock = jest.mocked(setMeasurement); + const setTagMock = jest.fn(); beforeEach(() => { jest.resetAllMocks(); startSpanMock.mockImplementation((_, fn) => fn({} as Span)); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - withScopeMock.mockImplementation((fn: any) => fn({ setTags: setTagsMock })); + startSpanManualMock.mockImplementation((_, fn) => + fn({} as Span, () => { + // Intentionally empty + }), + ); + + withScopeMock.mockImplementation((fn: (arg: Scope) => unknown) => + fn({ setTag: setTagMock } as unknown as Scope), + ); }); describe('trace', () => { @@ -87,8 +97,12 @@ describe('Trace', () => { expect.any(Function), ); - expect(setTagsMock).toHaveBeenCalledTimes(1); - expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK); + expect(setTagMock).toHaveBeenCalledTimes(2); + expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1'); + expect(setTagMock).toHaveBeenCalledWith('tag2', true); + + expect(setMeasurementMock).toHaveBeenCalledTimes(1); + expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none'); }); it('invokes Sentry if no callback provided', () => { @@ -113,8 +127,12 @@ describe('Trace', () => { expect.any(Function), ); - expect(setTagsMock).toHaveBeenCalledTimes(1); - expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK); + expect(setTagMock).toHaveBeenCalledTimes(2); + expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1'); + expect(setTagMock).toHaveBeenCalledWith('tag2', true); + + expect(setMeasurementMock).toHaveBeenCalledTimes(1); + expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none'); }); it('invokes Sentry if no callback provided with custom start time', () => { @@ -141,8 +159,12 @@ describe('Trace', () => { expect.any(Function), ); - expect(setTagsMock).toHaveBeenCalledTimes(1); - expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK); + expect(setTagMock).toHaveBeenCalledTimes(2); + expect(setTagMock).toHaveBeenCalledWith('tag1', 'value1'); + expect(setTagMock).toHaveBeenCalledWith('tag2', true); + + expect(setMeasurementMock).toHaveBeenCalledTimes(1); + expect(setMeasurementMock).toHaveBeenCalledWith('tag3', 123, 'none'); }); }); diff --git a/app/util/trace.ts b/app/util/trace.ts index 339943be165..1d79eb7ced5 100644 --- a/app/util/trace.ts +++ b/app/util/trace.ts @@ -2,15 +2,19 @@ import { startSpan as sentryStartSpan, startSpanManual, withScope, + setMeasurement, + Scope, } from '@sentry/react-native'; import performance from 'react-native-performance'; -import type { Primitive, Span, StartSpanOptions } from '@sentry/types'; +import type { Span, StartSpanOptions, MeasurementUnit } from '@sentry/types'; import { createModuleLogger, createProjectLogger } from '@metamask/utils'; // Cannot create this 'sentry' logger in Sentry util file because of circular dependency const projectLogger = createProjectLogger('sentry'); const log = createModuleLogger(projectLogger, 'trace'); - +/** + * The supported trace names. + */ export enum TraceName { DeveloperTest = 'Developer Test', Middleware = 'Middleware', @@ -56,24 +60,72 @@ export interface PendingTrace { startTime: number; timeoutId: NodeJS.Timeout; } - +/** + * A context object to associate traces with each other and generate nested traces. + */ export type TraceContext = unknown; - +/** + * A callback function that can be traced. + */ export type TraceCallback = (context?: TraceContext) => T; - +/** + * A request to create a new trace. + */ export interface TraceRequest { + /** + * Custom data to associate with the trace. + */ data?: Record; + + /** + * A unique identifier when not tracing a callback. + * Defaults to 'default' if not provided. + */ id?: string; + + /** + * The name of the trace. + */ name: TraceName; + + /** + * The parent context of the trace. + * If provided, the trace will be nested under the parent trace. + */ parentContext?: TraceContext; + + /** + * Override the start time of the trace. + */ startTime?: number; + + /** + * Custom tags to associate with the trace. + */ tags?: Record; + /** + * Custom operation name to associate with the trace. + */ op?: string; } - +/** + * A request to end a pending trace. + */ export interface EndTraceRequest { + /** + * The unique identifier of the trace. + * Defaults to 'default' if not provided. + */ id?: string; + + /** + * The name of the trace. + */ name: TraceName; + + /** + * Override the end time of the trace. + */ timestamp?: number; } @@ -81,6 +133,16 @@ export function trace(request: TraceRequest, fn: TraceCallback): T; export function trace(request: TraceRequest): TraceContext; +/** + * Create a Sentry transaction to analyse the duration of a code flow. + * If a callback is provided, the transaction will be automatically ended when the callback completes. + * If the callback returns a promise, the transaction will be ended when the promise resolves or rejects. + * If no callback is provided, the transaction must be manually ended using `endTrace`. + * + * @param request - The data associated with the trace, such as the name and tags. + * @param fn - The optional callback to record the duration of. + * @returns The context of the trace, or the result of the callback if provided. + */ export function trace( request: TraceRequest, fn?: TraceCallback, @@ -92,6 +154,12 @@ export function trace( return traceCallback(request, fn); } +/** + * End a pending trace that was started without a callback. + * Does nothing if the pending trace cannot be found. + * + * @param request - The data necessary to identify and end the pending trace. + */ export function endTrace(request: EndTraceRequest) { const { name, timestamp } = request; const id = getTraceId(request); @@ -124,6 +192,10 @@ function traceCallback(request: TraceRequest, fn: TraceCallback): T { const start = Date.now(); let error: unknown; + if (span) { + initSpan(span, request); + } + return tryCatchMaybePromise( () => fn(span), (currentError) => { @@ -154,6 +226,10 @@ function startTrace(request: TraceRequest): TraceContext { span?.end(timestamp); }; + if (span) { + initSpan(span, request); + } + const timeoutId = setTimeout(() => { log('Trace cleanup due to timeout', name, id); end(); @@ -178,14 +254,7 @@ function startSpan( request: TraceRequest, callback: (spanOptions: StartSpanOptions) => T, ) { - const { - data: attributes, - name, - parentContext, - startTime, - tags, - op, - } = request; + const { data: attributes, name, parentContext, startTime, op } = request; const parentSpan = (parentContext ?? null) as Span | null; const spanOptions: StartSpanOptions = { @@ -199,7 +268,7 @@ function startSpan( }; return withScope((scope) => { - scope.setTags(tags as Record); + initScope(scope, request); return callback(spanOptions); }) as T; @@ -216,6 +285,40 @@ function getTraceKey(request: TraceRequest) { return [name, id].join(':'); } +/** + * Initialise the isolated Sentry scope created for each trace. + * Includes setting all non-numeric tags. + * + * @param scope - The Sentry scope to initialise. + * @param request - The trace request. + */ +function initScope(scope: Scope, request: TraceRequest) { + const tags = request.tags ?? {}; + + for (const [key, value] of Object.entries(tags)) { + if (typeof value !== 'number') { + scope.setTag(key, value); + } + } +} + +/** + * Initialise the Sentry span created for each trace. + * Includes setting all numeric tags as measurements so they can be queried numerically in Sentry. + * + * @param _span - The Sentry span to initialise. + * @param request - The trace request. + */ +function initSpan(_span: Span, request: TraceRequest) { + const tags = request.tags ?? {}; + + for (const [key, value] of Object.entries(tags)) { + if (typeof value === 'number') { + sentrySetMeasurement(key, value, 'none'); + } + } +} + function getPerformanceTimestamp(): number { return performance.timeOrigin + performance.now(); } @@ -248,3 +351,11 @@ function tryCatchMaybePromise( return undefined; } + +function sentrySetMeasurement( + key: string, + value: number, + unit: MeasurementUnit, +) { + setMeasurement(key, value, unit); +} diff --git a/e2e/fixtures/fixture-builder.js b/e2e/fixtures/fixture-builder.js index c4e987b3f01..b850fab5970 100644 --- a/e2e/fixtures/fixture-builder.js +++ b/e2e/fixtures/fixture-builder.js @@ -421,6 +421,18 @@ class FixtureBuilder { pendingApprovalCount: 0, approvalFlows: [], }, + NotificationServicesController: { + subscriptionAccountsSeen: [], + isMetamaskNotificationsFeatureSeen: false, + isNotificationServicesEnabled: false, + isFeatureAnnouncementsEnabled: false, + metamaskNotificationsList: [], + metamaskNotificationsReadList: [], + isUpdatingMetamaskNotifications: false, + isFetchingMetamaskNotifications: false, + isUpdatingMetamaskNotificationsAccount: [], + isCheckingAccountsPresence: false, + }, }, }, privacy: { From 67b73a5352ec4dd1a19e13526c96c3228be2b20d Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Wed, 23 Oct 2024 15:17:15 -0700 Subject: [PATCH 2/7] feat: Remove Account Snap Warning (Flask) (#11451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** 1. What is the reason for the change? - This change adds an extra warning to users when trying to remove an account snap (https://metamask.github.io/snap-simple-keyring/latest/ for example) AND the user has snap accounts in their wallet - Snap accounts are not managed/owned by metamask and therefore cannot be recovered in metamask. User's must manage and backup these accounts in the snaps UI. 2. What is the improvement/solution? - Adds an extra layer of friction when trying to remove an account snap and the user has snap accounts in their wallet - This is a `FLASK ONLY` feature ## **Related issues** Fixes: https://github.com/MetaMask/accounts-planning/issues/155 ## **Manual testing steps** 1. check this branch 2. open `.js.env` 3. ensure that `export METAMASK_BUILD_TYPE=flask` is set to flask (not main) 4. build the app on this branch 5. create/import a wallet 6. inside the mobile browser, open this URL: https://metamask.github.io/snap-simple-keyring/latest/ 7. connect this snap 8. click create account, you can do this as many times as you want. 9. navigate back to the main wallet view 10. NOTICE there should be new accounts in your wallet with the name "Snap Account x" 11. Go to settings 12. navigate to snaps 13. locate the Snap simple keyring settings 14. scroll to the bottom of the page and click the red "Remove" button 15. Notice: new warning should pop up indicating that the user should backup these accounts before removing the snap 16. click continue 17. you should be prompted to type the name of the snap in the text input. 18. YOU SHOULD NOT BE ABLE TO REMOVE THE SNAP WITHOUT TYPING The CORRECT SNAP NAME 19. Once you type the correct snap name, you should be able to remove the snap 20. After clicking remove the snap should be removed. #### Testing a non account snap 1. check this branch 2. open `.js.env` 3. ensure that `export METAMASK_BUILD_TYPE=flask` is set to flask (not main) 4. build the app on this branch 5. create/import a wallet 6. navigate to settings/snaps 7. click on the pre installed Message signing snap 8. click on the remove button at the bottom 9. `THERE SHOULD NOT BE A WARNING` #### Testing removing a snap account without snap accounts in the wallet 1. check this branch 2. open `.js.env` 3. ensure that `export METAMASK_BUILD_TYPE=flask` is set to flask (not main) 4. build the app on this branch 5. create/import a wallet 6. inside the mobile browser, open this URL: https://metamask.github.io/snap-simple-keyring/latest/ 7. connect this snap. Do not create any accounts this time. 8. Go to settings 9. navigate to snaps 10. locate the Snap simple keyring settings 11. scroll to the bottom of the page and click the red "Remove" button 12. There should be no warning pop up and the snap should be removed ## **Screenshots/Recordings** ### Extension Version Screenshot 2024-10-01 at 5 38 58 PM Screenshot 2024-10-01 at 5 39 14 PM Screenshot 2024-10-01 at 5 39 20 PM ### **After** with snap accounts: https://github.com/user-attachments/assets/24aa2993-98fa-4d72-bc1f-b7f9c1aeb6ad Screenshot 2024-10-10 at 3 26 46 PM Without snap accounts: https://github.com/user-attachments/assets/7cf42c03-dc6e-40fb-b490-fb0eb4d8ba83 ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile 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. --- .../Views/AccountActions/AccountActions.tsx | 62 ++- .../KeyringSnapRemovalWarning.constants.ts | 9 + .../KeyringSnapRemovalWarning.styles.ts | 43 ++ .../KeyringSnapRemovalWarning.tsx | 226 ++++++++++ .../Snaps/KeyringSnapRemovalWarning/index.ts | 3 + .../test/KeyringSnapRemovalWarning.test.tsx | 337 ++++++++++++++ .../Views/Snaps/SnapSettings/SnapSettings.tsx | 168 +++++-- .../SnapSettings/test/SnapSettings.test.tsx | 419 ++++++++++++++---- .../KeyringAccountListItem.constants.ts | 4 + .../KeyringAccountListItem.styles.ts | 36 ++ .../KeyringAccountListItem.tsx | 74 ++++ .../KeyringAccountListItem/index.ts | 3 + .../test/KeyringAccountListItem.test.tsx | 42 ++ app/core/Engine.ts | 59 +-- .../SnapKeyring/utils/getAccountsBySnapId.ts | 17 + app/util/address/index.test.ts | 12 + app/util/address/index.ts | 11 + app/util/test/accountsControllerTestUtils.ts | 31 ++ .../Modals/AccountActionsModal.selectors.js | 5 +- locales/languages/en.json | 20 +- 20 files changed, 1421 insertions(+), 160 deletions(-) create mode 100644 app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.constants.ts create mode 100644 app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.styles.ts create mode 100644 app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.tsx create mode 100644 app/components/Views/Snaps/KeyringSnapRemovalWarning/index.ts create mode 100644 app/components/Views/Snaps/KeyringSnapRemovalWarning/test/KeyringSnapRemovalWarning.test.tsx create mode 100644 app/components/Views/Snaps/components/KeyringAccountListItem/KeyringAccountListItem.constants.ts create mode 100644 app/components/Views/Snaps/components/KeyringAccountListItem/KeyringAccountListItem.styles.ts create mode 100644 app/components/Views/Snaps/components/KeyringAccountListItem/KeyringAccountListItem.tsx create mode 100644 app/components/Views/Snaps/components/KeyringAccountListItem/index.ts create mode 100644 app/components/Views/Snaps/components/KeyringAccountListItem/test/KeyringAccountListItem.test.tsx create mode 100644 app/core/SnapKeyring/utils/getAccountsBySnapId.ts diff --git a/app/components/Views/AccountActions/AccountActions.tsx b/app/components/Views/AccountActions/AccountActions.tsx index 22feb6a46b9..99547d6cfda 100644 --- a/app/components/Views/AccountActions/AccountActions.tsx +++ b/app/components/Views/AccountActions/AccountActions.tsx @@ -34,7 +34,12 @@ import { protectWalletModalVisible } from '../../../actions/user'; import Routes from '../../../constants/navigation/Routes'; import { AccountActionsModalSelectorsIDs } from '../../../../e2e/selectors/Modals/AccountActionsModal.selectors'; import { useMetrics } from '../../../components/hooks/useMetrics'; -import { isHardwareAccount } from '../../../util/address'; +import { + isHardwareAccount, + ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) + isSnapAccount, + ///: END:ONLY_INCLUDE_IF +} from '../../../util/address'; import { removeAccountsFromPermissions } from '../../../core/Permissions'; import ExtendedKeyringTypes, { HardwareDeviceTypes, @@ -189,6 +194,49 @@ const AccountActions = () => { } }, [controllers.KeyringController]); + ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) + + /** + * Remove the snap account from the keyring + */ + const removeSnapAccount = useCallback(async () => { + if (selectedAddress) { + await controllers.KeyringController.removeAccount(selectedAddress as Hex); + await removeAccountsFromPermissions([selectedAddress]); + trackEvent(MetaMetricsEvents.ACCOUNT_REMOVED, { + accountType: keyring?.type, + selectedAddress, + }); + } + }, [ + controllers.KeyringController, + keyring?.type, + selectedAddress, + trackEvent, + ]); + + const showRemoveSnapAccountAlert = useCallback(() => { + Alert.alert( + strings('accounts.remove_snap_account'), + strings('accounts.remove_snap_account_alert_description'), + [ + { + text: strings('accounts.remove_account_alert_cancel_btn'), + style: 'cancel', + }, + { + text: strings('accounts.remove_account_alert_remove_btn'), + onPress: async () => { + sheetRef.current?.onCloseBottomSheet(async () => { + await removeSnapAccount(); + }); + }, + }, + ], + ); + }, [removeSnapAccount]); + ///: END:ONLY_INCLUDE_IF + /** * Forget the device if there are no more accounts in the keyring * @param keyringType - The keyring type @@ -306,6 +354,18 @@ const AccountActions = () => { testID={AccountActionsModalSelectorsIDs.REMOVE_HARDWARE_ACCOUNT} /> )} + { + ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) + selectedAddress && isSnapAccount(selectedAddress) && ( + + ) + ///: END:ONLY_INCLUDE_IF + } { + const { theme } = params; + const { colors } = theme; + + return StyleSheet.create({ + bottomSheet: { + flex: 1, + }, + container: { + paddingHorizontal: 16, + }, + description: { + paddingVertical: 8, + }, + buttonContainer: { + paddingTop: 16, + }, + input: { + borderWidth: 1, + borderColor: colors.border.default, + borderRadius: 4, + padding: 10, + marginVertical: 10, + }, + errorText: { + color: colors.error.default, + }, + placeholderText: { + color: colors.text.muted, + }, + scrollView: { + flexGrow: 1, + maxHeight: 300, + }, + }); +}; + +export default styleSheet; +///: END:ONLY_INCLUDE_IF diff --git a/app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.tsx b/app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.tsx new file mode 100644 index 00000000000..b2ec560b5dd --- /dev/null +++ b/app/components/Views/Snaps/KeyringSnapRemovalWarning/KeyringSnapRemovalWarning.tsx @@ -0,0 +1,226 @@ +///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) +import React, { + useEffect, + useRef, + useState, + useCallback, + useMemo, +} from 'react'; +import { View, TextInput, ScrollView } from 'react-native'; +import { NativeViewGestureHandler } from 'react-native-gesture-handler'; +import { Snap } from '@metamask/snaps-utils'; +import BottomSheet, { + BottomSheetRef, +} from '../../../../component-library/components/BottomSheets/BottomSheet'; +import Text, { + TextVariant, +} from '../../../../component-library/components/Texts/Text'; +import { InternalAccount } from '@metamask/keyring-api'; +import BannerAlert from '../../../../component-library/components/Banners/Banner/variants/BannerAlert'; +import { BannerAlertSeverity } from '../../../../component-library/components/Banners/Banner'; +import BottomSheetHeader from '../../../../component-library/components/BottomSheets/BottomSheetHeader'; +import { useStyles } from '../../../hooks/useStyles'; +import stylesheet from './KeyringSnapRemovalWarning.styles'; +import { strings } from '../../../../../locales/i18n'; +import { KeyringAccountListItem } from '../components/KeyringAccountListItem'; +import { getAccountLink } from '@metamask/etherscan-link'; +import { useSelector } from 'react-redux'; +import { selectProviderConfig } from '../../../../selectors/networkController'; +import BottomSheetFooter, { + ButtonsAlignment, +} from '../../../../component-library/components/BottomSheets/BottomSheetFooter'; +import { + ButtonProps, + ButtonSize, + ButtonVariants, +} from '../../../../component-library/components/Buttons/Button/Button.types'; +import { + KEYRING_SNAP_REMOVAL_WARNING, + KEYRING_SNAP_REMOVAL_WARNING_CANCEL, + KEYRING_SNAP_REMOVAL_WARNING_CONTINUE, + KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT, +} from './KeyringSnapRemovalWarning.constants'; +import Logger from '../../../../util/Logger'; + +interface KeyringSnapRemovalWarningProps { + snap: Snap; + keyringAccounts: InternalAccount[]; + onCancel: () => void; + onClose: () => void; + onSubmit: () => void; +} + +export default function KeyringSnapRemovalWarning({ + snap, + keyringAccounts, + onCancel, + onClose, + onSubmit, +}: KeyringSnapRemovalWarningProps) { + const [showConfirmation, setShowConfirmation] = useState(false); + const [confirmedRemoval, setConfirmedRemoval] = useState(false); + const [confirmationInput, setConfirmationInput] = useState(''); + const [error, setError] = useState(false); + const { chainId } = useSelector(selectProviderConfig); + const { styles } = useStyles(stylesheet, {}); + const bottomSheetRef = useRef(null); + + useEffect(() => { + setShowConfirmation(keyringAccounts.length === 0); + }, [keyringAccounts]); + + const validateConfirmationInput = useCallback( + (input: string): boolean => input === snap.manifest.proposedName, + [snap.manifest.proposedName], + ); + + const handleConfirmationInputChange = useCallback( + (text: string) => { + setConfirmationInput(text); + setConfirmedRemoval(validateConfirmationInput(text)); + }, + [validateConfirmationInput], + ); + + const handleContinuePress = useCallback(() => { + if (!showConfirmation) { + setShowConfirmation(true); + } else if (confirmedRemoval) { + try { + onSubmit(); + } catch (e) { + Logger.error( + e as Error, + 'KeyringSnapRemovalWarning: error while removing snap', + ); + setError(true); + } + } + }, [showConfirmation, confirmedRemoval, onSubmit]); + + const cancelButtonProps: ButtonProps = useMemo( + () => ({ + variant: ButtonVariants.Secondary, + label: strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.cancel_button', + ), + size: ButtonSize.Lg, + onPress: onCancel, + testID: KEYRING_SNAP_REMOVAL_WARNING_CANCEL, + }), + [onCancel], + ); + + const continueButtonProps: ButtonProps = useMemo( + () => ({ + variant: ButtonVariants.Primary, + label: showConfirmation + ? strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.remove_snap_button', + ) + : strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.continue_button', + ), + size: ButtonSize.Lg, + onPress: handleContinuePress, + isDisabled: showConfirmation && !confirmedRemoval, + isDanger: showConfirmation, + testID: KEYRING_SNAP_REMOVAL_WARNING_CONTINUE, + }), + [showConfirmation, confirmedRemoval, handleContinuePress], + ); + + const buttonPropsArray = useMemo( + () => [cancelButtonProps, continueButtonProps], + [cancelButtonProps, continueButtonProps], + ); + + const accountListItems = useMemo( + () => + keyringAccounts.map((account, index) => ( + + )), + [keyringAccounts, chainId], + ); + + return ( + + + + + {strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.title', + )} + + + + {showConfirmation ? ( + <> + + {`${strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.remove_account_snap_alert_description_1', + )} `} + + {snap.manifest.proposedName} + + {` ${strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.remove_account_snap_alert_description_2', + )}`} + + + {error && ( + + {strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.remove_snap_error', + { + snapName: snap.manifest.proposedName, + }, + )} + + )} + + ) : ( + <> + + {strings( + 'app_settings.snaps.snap_settings.remove_account_snap_warning.description', + )} + + + + {accountListItems} + + + + )} + + + + ); +} +///: END:ONLY_INCLUDE_IF diff --git a/app/components/Views/Snaps/KeyringSnapRemovalWarning/index.ts b/app/components/Views/Snaps/KeyringSnapRemovalWarning/index.ts new file mode 100644 index 00000000000..94d58b3d412 --- /dev/null +++ b/app/components/Views/Snaps/KeyringSnapRemovalWarning/index.ts @@ -0,0 +1,3 @@ +///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) +export { default as KeyringSnapRemovalWarning } from './KeyringSnapRemovalWarning'; +///: END:ONLY_INCLUDE_IF diff --git a/app/components/Views/Snaps/KeyringSnapRemovalWarning/test/KeyringSnapRemovalWarning.test.tsx b/app/components/Views/Snaps/KeyringSnapRemovalWarning/test/KeyringSnapRemovalWarning.test.tsx new file mode 100644 index 00000000000..6359b5a7b3d --- /dev/null +++ b/app/components/Views/Snaps/KeyringSnapRemovalWarning/test/KeyringSnapRemovalWarning.test.tsx @@ -0,0 +1,337 @@ +import React from 'react'; +import { fireEvent, waitFor } from '@testing-library/react-native'; +import KeyringSnapRemovalWarning from '../KeyringSnapRemovalWarning'; +import { + KEYRING_SNAP_REMOVAL_WARNING, + KEYRING_SNAP_REMOVAL_WARNING_CANCEL, + KEYRING_SNAP_REMOVAL_WARNING_CONTINUE, + KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT, +} from '../KeyringSnapRemovalWarning.constants'; +import { useSelector } from 'react-redux'; +import renderWithProvider from '../../../../../util/test/renderWithProvider'; +import { Snap, SnapStatus } from '@metamask/snaps-utils'; +import { SnapId } from '@metamask/snaps-sdk'; +import { SemVerVersion } from '@metamask/utils'; +import { createMockSnapInternalAccount } from '../../../../../util/test/accountsControllerTestUtils'; +import { KEYRING_ACCOUNT_LIST_ITEM } from '../../components/KeyringAccountListItem/KeyringAccountListItem.constants'; + +jest.mock('react-redux', () => ({ + ...jest.requireActual('react-redux'), + useSelector: jest.fn(), +})); + +jest.mock('react-native-safe-area-context', () => { + // using disting digits for mock rects to make sure they are not mixed up + const inset = { top: 1, right: 2, bottom: 3, left: 4 }; + const frame = { width: 5, height: 6, x: 7, y: 8 }; + return { + SafeAreaProvider: jest.fn().mockImplementation(({ children }) => children), + SafeAreaConsumer: jest + .fn() + .mockImplementation(({ children }) => children(inset)), + useSafeAreaInsets: jest.fn().mockImplementation(() => inset), + useSafeAreaFrame: jest.fn().mockImplementation(() => frame), + }; +}); + +jest.mock('@react-navigation/native', () => { + const actualNav = jest.requireActual('@react-navigation/native'); + return { + ...actualNav, + useNavigation: () => ({ + navigate: jest.fn(), + }), + }; +}); + +describe('KeyringSnapRemovalWarning', () => { + const mockSnapName = 'MetaMask Simple Snap Keyring'; + const mockSnap: Snap = { + blocked: false, + enabled: true, + id: 'npm:@metamask/snap-simple-keyring-snap' as SnapId, + initialPermissions: { + 'endowment:keyring': { + allowedOrigins: ['https://metamask.github.io', 'metamask.github.io'], + }, + 'endowment:rpc': { + dapps: true, + }, + snap_manageAccounts: {}, + snap_manageState: {}, + }, + manifest: { + version: '1.1.6' as SemVerVersion, + description: 'An example of a key management snap for a simple keyring.', + proposedName: mockSnapName, + repository: { + type: 'git', + url: 'git+https://github.com/MetaMask/snap-simple-keyring.git', + }, + source: { + shasum: 'P2BbaJn6jb7+ecBF6mJJnheQ4j8dtEZ8O4FLqLv8e8M=', + location: { + npm: { + filePath: 'dist/bundle.js', + iconPath: 'images/icon.svg', + packageName: '@metamask/snap-simple-keyring-snap', + registry: 'https://registry.npmjs.org/', + }, + }, + }, + initialPermissions: { + 'endowment:keyring': { + allowedOrigins: ['https://metamask.github.io', 'metamask.github.io'], + }, + 'endowment:rpc': { + dapps: true, + }, + snap_manageAccounts: {}, + snap_manageState: {}, + }, + manifestVersion: '0.1', + }, + status: 'stopped' as SnapStatus, + sourceCode: '', + version: '1.1.6' as SemVerVersion, + versionHistory: [ + { + version: '1.1.6', + date: 1727403640652, + origin: 'https://metamask.github.io', + }, + ], + auxiliaryFiles: [], + localizationFiles: [], + }; + + const MOCK_ADDRESS_1 = '0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272'; + const MOCK_ADDRESS_2 = '0xA7E9922b0e7DB390c3B108127739eFebe4d6293E'; + + const mockKeyringAccount1 = createMockSnapInternalAccount( + MOCK_ADDRESS_1, + 'Snap Account 1', + ); + const mockKeyringAccount2 = createMockSnapInternalAccount( + MOCK_ADDRESS_2, + 'Snap Account 2', + ); + + const mockKeyringAccounts = [mockKeyringAccount1, mockKeyringAccount2]; + const onCancelMock = jest.fn(); + const onCloseMock = jest.fn(); + const onSubmitMock = jest.fn(); + + beforeEach(() => { + jest.clearAllMocks(); + (useSelector as jest.Mock).mockReturnValue({ chainId: '1' }); + }); + + it('renders correctly with initial props', () => { + const { getByTestId, queryByText } = renderWithProvider( + , + ); + + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + expect(continueButton).toBeTruthy(); + expect(continueButton.props.children[1].props.children).toBe('Continue'); + + const cancelButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CANCEL); + expect(cancelButton).toBeTruthy(); + expect(cancelButton.props.children[1].props.children).toBe('Cancel'); + + const warningBannerTitle = queryByText( + 'Be sure you can access any accounts created by this Snap on your own before removing it', + ); + expect(warningBannerTitle).toBeTruthy(); + }); + + it('renders the correct number of keyring account list items', () => { + const { getAllByTestId } = renderWithProvider( + , + ); + + const accountListItems = getAllByTestId(KEYRING_ACCOUNT_LIST_ITEM); + expect(accountListItems).toHaveLength(mockKeyringAccounts.length); + }); + it('shows confirmation input when keyringAccounts is empty', () => { + const { getByTestId } = renderWithProvider( + , + ); + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + expect(continueButton).toBeTruthy(); + expect(continueButton.props.disabled).toBe(true); + expect(continueButton.props.children[1].props.children).toBe('Remove Snap'); + + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + expect(textInput).toBeTruthy(); + }); + + it('enables continue button when correct snap name is entered', async () => { + const { getByTestId } = renderWithProvider( + , + ); + + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + expect(continueButton.props.disabled).toBe(true); + + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + expect(textInput).toBeTruthy(); + fireEvent.changeText(textInput, mockSnapName); + + await waitFor(() => { + expect(continueButton.props.disabled).toBe(false); + }); + }); + + it('does not enable continue button when incorrect snap name is entered', async () => { + const { getByTestId } = renderWithProvider( + , + ); + + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + expect(continueButton.props.disabled).toBe(true); + + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + expect(textInput).toBeTruthy(); + fireEvent.changeText(textInput, 'Wrong snap name'); + + await waitFor(() => { + expect(continueButton.props.disabled).toBe(true); + }); + }); + + it('calls onSubmit when confirmed and continue is pressed', async () => { + const { getByTestId } = renderWithProvider( + , + ); + + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + expect(textInput).toBeTruthy(); + fireEvent.changeText(textInput, mockSnapName); + + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + fireEvent.press(continueButton); + expect(onSubmitMock).toHaveBeenCalled(); + }); + + it('displays error when onSubmit throws', async () => { + onSubmitMock.mockImplementation(() => { + throw new Error('Error'); + }); + + const { getByTestId, getByText } = renderWithProvider( + , + ); + + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + fireEvent.changeText(textInput, mockSnapName); + + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + fireEvent.press(continueButton); + + await waitFor(() => { + expect( + getByText( + `Failed to remove ${mockSnapName}`, + ), + ).toBeTruthy(); + }); + }); + + it('calls onCancel when cancel button is pressed', () => { + const { getByTestId } = renderWithProvider( + , + ); + + const cancelButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CANCEL); + fireEvent.press(cancelButton); + + expect(onCancelMock).toHaveBeenCalled(); + }); + + it('calls onClose when BottomSheet is closed', () => { + const { getByTestId } = renderWithProvider( + , + ); + + const bottomSheet = getByTestId(KEYRING_SNAP_REMOVAL_WARNING); + fireEvent(bottomSheet, 'onClose'); + + expect(onCloseMock).toHaveBeenCalled(); + }); + it('allows removal of snaps with empty names and keeps the continue button enabled', () => { + const { getByTestId } = renderWithProvider( + , + ); + const textInput = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_TEXT_INPUT); + fireEvent.changeText(textInput, ''); + const continueButton = getByTestId(KEYRING_SNAP_REMOVAL_WARNING_CONTINUE); + expect(continueButton.props.disabled).toBe(false); + expect(textInput.props.value).toBe(''); + expect(continueButton.props.children[1].props.children).toBe('Remove Snap'); + }); +}); diff --git a/app/components/Views/Snaps/SnapSettings/SnapSettings.tsx b/app/components/Views/Snaps/SnapSettings/SnapSettings.tsx index 19f739400eb..d56c1d0b200 100644 --- a/app/components/Views/Snaps/SnapSettings/SnapSettings.tsx +++ b/app/components/Views/Snaps/SnapSettings/SnapSettings.tsx @@ -1,5 +1,5 @@ -///: BEGIN:ONLY_INCLUDE_IF(external-snaps) -import React, { useCallback, useEffect } from 'react'; +///: BEGIN:ONLY_INCLUDE_IF(external-snaps,keyring-snaps) +import React, { useCallback, useEffect, useState } from 'react'; import { View, ScrollView, SafeAreaView } from 'react-native'; import Engine from '../../../../core/Engine'; @@ -28,7 +28,11 @@ import { useStyles } from '../../../hooks/useStyles'; import { useSelector } from 'react-redux'; import SNAP_SETTINGS_REMOVE_BUTTON from './SnapSettings.constants'; import { selectPermissionControllerState } from '../../../../selectors/snaps/permissionController'; - +import KeyringSnapRemovalWarning from '../KeyringSnapRemovalWarning/KeyringSnapRemovalWarning'; +import { getAccountsBySnapId } from '../../../../core/SnapKeyring/utils/getAccountsBySnapId'; +import { selectInternalAccounts } from '../../../../selectors/accountsController'; +import { InternalAccount } from '@metamask/keyring-api'; +import Logger from '../../../../util/Logger'; interface SnapSettingsProps { snap: Snap; } @@ -42,9 +46,16 @@ const SnapSettings = () => { const navigation = useNavigation(); const { snap } = useParams(); - const permissionsState = useSelector(selectPermissionControllerState); + const [ + isShowingSnapKeyringRemoveWarning, + setIsShowingSnapKeyringRemoveWarning, + ] = useState(false); + + const [keyringAccounts, setKeyringAccounts] = useState([]); + const internalAccounts = useSelector(selectInternalAccounts); + // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any function getPermissionSubjects(state: any) { @@ -70,53 +81,118 @@ const SnapSettings = () => { ); }, [colors, navigation, snap.manifest.proposedName]); + const isKeyringSnap = Boolean(permissionsFromController?.snap_manageAccounts); + + useEffect(() => { + if (isKeyringSnap) { + (async () => { + const addresses = await getAccountsBySnapId(snap.id); + const snapIdentities = Object.values(internalAccounts).filter( + (internalAccount) => + addresses.includes(internalAccount.address.toLowerCase()), + ); + setKeyringAccounts(snapIdentities); + })(); + } + }, [snap?.id, internalAccounts, isKeyringSnap]); + + const handleKeyringSnapRemovalWarningClose = useCallback(() => { + setIsShowingSnapKeyringRemoveWarning(false); + }, []); + + const removeSnap = useCallback(async () => { - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { SnapController } = Engine.context as any; + const { SnapController } = Engine.context; await SnapController.removeSnap(snap.id); + + if (isKeyringSnap && keyringAccounts.length > 0) { + try { + for (const keyringAccount of keyringAccounts) { + await Engine.removeAccount(keyringAccount.address); + } + } catch(error) { + Logger.error(error as Error, 'SnapSettings: failed to remove snap accounts when calling Engine.removeAccount'); + } + } navigation.goBack(); - }, [navigation, snap.id]); + }, [isKeyringSnap, keyringAccounts, navigation, snap.id]); + + const handleRemoveSnap = useCallback(() => { + if (isKeyringSnap && keyringAccounts.length > 0) { + setIsShowingSnapKeyringRemoveWarning(true); + } else { + removeSnap(); + } + }, [isKeyringSnap, keyringAccounts.length, removeSnap]); + + + const handleRemoveSnapKeyring = useCallback(() => { + try { + setIsShowingSnapKeyringRemoveWarning(true); + removeSnap(); + setIsShowingSnapKeyringRemoveWarning(false); + } catch { + setIsShowingSnapKeyringRemoveWarning(false); + } finally { + setIsShowingSnapKeyringRemoveWarning(false); + } + }, [removeSnap]); + + const shouldRenderRemoveSnapAccountWarning = + isShowingSnapKeyringRemoveWarning && + isKeyringSnap && + keyringAccounts.length > 0; return ( - - - - - - - - - - - - {strings( - 'app_settings.snaps.snap_settings.remove_snap_section_title', - )} - - - {strings( - 'app_settings.snaps.snap_settings.remove_snap_section_description', - )} - -