From 6fc133f0b88f6d38ef9e30292bfb504c8273661b Mon Sep 17 00:00:00 2001 From: Fumiko Date: Wed, 18 Dec 2024 13:18:52 -0700 Subject: [PATCH] fix: (Delete) update tests, logic update in handler, UI update --- .../handlers/avalanche_deleteAccounts.test.ts | 212 ++++++++++++++++-- .../handlers/avalanche_deleteAccounts.ts | 103 +++++---- .../common/approval/ApprovalSection.tsx | 4 +- src/pages/Wallet/DeleteAccounts.tsx | 82 ++++--- 4 files changed, 314 insertions(+), 87 deletions(-) diff --git a/src/background/services/accounts/handlers/avalanche_deleteAccounts.test.ts b/src/background/services/accounts/handlers/avalanche_deleteAccounts.test.ts index b48ecd72..dea5f3ac 100644 --- a/src/background/services/accounts/handlers/avalanche_deleteAccounts.test.ts +++ b/src/background/services/accounts/handlers/avalanche_deleteAccounts.test.ts @@ -5,9 +5,15 @@ import { DAppProviderRequest } from '@src/background/connections/dAppConnection/ import { openApprovalWindow } from '@src/background/runtime/openApprovalWindow'; import { DEFERRED_RESPONSE } from '@src/background/connections/middlewares/models'; -import { Account } from '../models'; +import { + type Accounts, + AccountType, + type ImportedAccount, + type PrimaryAccount, +} from '../models'; import { canSkipApproval } from '@src/utils/canSkipApproval'; import { AvalancheDeleteAccountsHandler } from './avalanche_deleteAccounts'; +import type { WalletDetails } from '../../wallet/models'; jest.mock('@src/utils/canSkipApproval'); jest.mock('@src/background/runtime/openApprovalWindow'); @@ -15,18 +21,47 @@ jest.mock('@src/background/runtime/openApprovalWindow'); describe('src/background/services/accounts/handlers/avalanche_deleteAccounts', () => { const deleteAccounts = jest.fn(); const getAccountByID = jest.fn(); + const getAccounts = jest.fn(); const accountServiceMock = { getAccountByID, deleteAccounts, + getAccounts, } as any; + const getPrimaryWalletsDetails = jest.fn(); + + const secretsServiceMock = { + getPrimaryWalletsDetails, + } as any; + const wallet = { + id: 'walletId', + name: 'test wallet', + } as WalletDetails; + + const primaryAccount = { + id: 'primaryAccountId', + name: 'account name', + walletId: wallet.id, + type: AccountType.PRIMARY, + } as PrimaryAccount; + const primaryAccount2 = { + id: 'primaryAccountId2', + name: 'account name2', + walletId: wallet.id, + type: AccountType.PRIMARY, + } as PrimaryAccount; + const importedAccount = { + id: 'importedAccountId', + name: 'account name', + } as ImportedAccount; + beforeEach(() => { jest.resetAllMocks(); }); it('returns error when domain info is not known', async () => { - const handler = new AvalancheDeleteAccountsHandler({} as any); + const handler = new AvalancheDeleteAccountsHandler({} as any, {} as any); const request = { id: '123', method: DAppProviderRequest.ACCOUNTS_DELETE, @@ -40,26 +75,48 @@ describe('src/background/services/accounts/handlers/avalanche_deleteAccounts', ( }); it('prompts approval for non-core requests', async () => { - const handler = new AvalancheDeleteAccountsHandler(accountServiceMock); + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + jest.mocked(canSkipApproval).mockResolvedValueOnce(false); + + getAccountByID.mockImplementation((id) => { + if (id === primaryAccount2.id) { + return primaryAccount2; + } + return importedAccount; + }); + getAccounts.mockReturnValue({ + primary: { [wallet.id]: [primaryAccount, primaryAccount2] }, + imported: { [importedAccount.id]: importedAccount }, + } as Accounts); + + getPrimaryWalletsDetails.mockResolvedValue([wallet]); + const request = { id: '123', method: DAppProviderRequest.ACCOUNTS_DELETE, - params: [['accountId']], + params: [[primaryAccount2.id, importedAccount.id]], site: { domain: 'google.com', tabId: 1, }, } as any; - jest.mocked(canSkipApproval).mockResolvedValueOnce(false); - const account = { id: 'accountId', name: 'account name' } as Account; - getAccountByID.mockReturnValueOnce(account); const result = await handler.handleAuthenticated(buildRpcCall(request)); expect(openApprovalWindow).toHaveBeenCalledWith( expect.objectContaining({ displayData: { - accounts: { accountId: { id: 'accountId', name: 'account name' } }, + accounts: { + primary: { + [wallet.id]: [primaryAccount2], + }, + imported: [importedAccount], + wallet: { [wallet.id]: wallet.name }, + }, }, }), 'deleteAccounts' @@ -69,50 +126,163 @@ describe('src/background/services/accounts/handlers/avalanche_deleteAccounts', ( }); it('does not prompt approval for core suite', async () => { - const handler = new AvalancheDeleteAccountsHandler(accountServiceMock); + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + jest.mocked(canSkipApproval).mockResolvedValueOnce(true); + + getAccountByID.mockReturnValueOnce(primaryAccount); + getAccounts.mockReturnValue({ + primary: { [wallet.id]: [primaryAccount] }, + imported: { [importedAccount.id]: importedAccount }, + } as Accounts); + + getPrimaryWalletsDetails.mockResolvedValue([wallet]); const request = { id: '123', method: DAppProviderRequest.ACCOUNTS_DELETE, - params: [['accountId']], + params: [[primaryAccount.id]], site: { domain: 'core.app', tabId: 1, }, } as any; - jest.mocked(canSkipApproval).mockResolvedValueOnce(true); - const account = { id: 'accountId', name: 'old name' } as Account; - getAccountByID.mockReturnValueOnce(account); - const result = await handler.handleAuthenticated(buildRpcCall(request)); expect(openApprovalWindow).not.toHaveBeenCalled(); expect(deleteAccounts).toHaveBeenCalledTimes(1); - expect(deleteAccounts).toHaveBeenCalledWith(['accountId']); + expect(deleteAccounts).toHaveBeenCalledWith([primaryAccount.id]); expect(result).toEqual({ ...request, result: null }); }); it('returns error when deleting accounts fails', async () => { jest.mocked(canSkipApproval).mockResolvedValueOnce(true); - const handler = new AvalancheDeleteAccountsHandler({ - getAccountByID: jest.fn().mockReturnValueOnce({}), - deleteAccounts: jest.fn().mockRejectedValueOnce(new Error('some error')), - } as any); + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + getAccountByID.mockReturnValueOnce(primaryAccount); + getAccounts.mockReturnValue({ + primary: { [wallet.id]: [primaryAccount] }, + imported: { [importedAccount.id]: importedAccount }, + } as Accounts); + + getPrimaryWalletsDetails.mockResolvedValue([wallet]); const request = { id: '123', method: DAppProviderRequest.ACCOUNTS_DELETE, - params: [['accountId']], + params: [[primaryAccount.id]], site: { domain: 'core.app', tabId: 1, }, } as any; - + deleteAccounts.mockRejectedValueOnce(new Error('some error')); const result = await handler.handleAuthenticated(buildRpcCall(request)); expect(result).toEqual({ ...request, error: ethErrors.rpc.internal('Account removing failed'), }); }); + + it('returns error when no accounts were found using account IDs in request param', async () => { + jest.mocked(canSkipApproval).mockResolvedValueOnce(true); + + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + getAccountByID.mockReturnValueOnce(undefined); + + const request = { + id: '123', + method: DAppProviderRequest.ACCOUNTS_DELETE, + params: [['wrongId']], + site: { + domain: 'core.app', + tabId: 1, + }, + } as any; + const result = await handler.handleAuthenticated(buildRpcCall(request)); + expect(result).toEqual({ + ...request, + error: ethErrors.rpc.internal('No account with specified IDs'), + }); + }); + + it('returns error when all accounts will be deleted', async () => { + jest.mocked(canSkipApproval).mockResolvedValueOnce(true); + + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + getAccountByID.mockImplementation((id) => { + if (id === primaryAccount.id) { + return primaryAccount; + } + return importedAccount; + }); + getAccounts.mockReturnValue({ + primary: { [wallet.id]: [primaryAccount] }, + imported: { [importedAccount.id]: importedAccount }, + } as Accounts); + + const request = { + id: '123', + method: DAppProviderRequest.ACCOUNTS_DELETE, + params: [[primaryAccount.id, importedAccount.id]], + site: { + domain: 'core.app', + tabId: 1, + }, + } as any; + const result = await handler.handleAuthenticated(buildRpcCall(request)); + expect(result).toEqual({ + ...request, + error: ethErrors.rpc.internal('Cannot delete all accounts'), + }); + }); + + it('returns error when requested account is not the last index in the wallet', async () => { + const handler = new AvalancheDeleteAccountsHandler( + accountServiceMock, + secretsServiceMock + ); + + jest.mocked(canSkipApproval).mockResolvedValueOnce(false); + + getAccountByID.mockReturnValue(primaryAccount); + getAccounts.mockReturnValue({ + primary: { [wallet.id]: [primaryAccount, primaryAccount2] }, + imported: {}, + } as Accounts); + + getPrimaryWalletsDetails.mockResolvedValue([wallet]); + + const request = { + id: '123', + method: DAppProviderRequest.ACCOUNTS_DELETE, + params: [[primaryAccount.id]], + site: { + domain: 'google.com', + tabId: 1, + }, + } as any; + + const result = await handler.handleAuthenticated(buildRpcCall(request)); + expect(result).toEqual({ + ...request, + error: ethErrors.rpc.internal( + 'Only the last account of the wallet can be removed' + ), + }); + }); }); diff --git a/src/background/services/accounts/handlers/avalanche_deleteAccounts.ts b/src/background/services/accounts/handlers/avalanche_deleteAccounts.ts index e98f8e0b..78d19c2f 100644 --- a/src/background/services/accounts/handlers/avalanche_deleteAccounts.ts +++ b/src/background/services/accounts/handlers/avalanche_deleteAccounts.ts @@ -4,15 +4,15 @@ import { ethErrors } from 'eth-rpc-errors'; import { DAppRequestHandler } from '@src/background/connections/dAppConnection/DAppRequestHandler'; import { DAppProviderRequest, - JsonRpcRequestParams, + type JsonRpcRequestParams, } from '@src/background/connections/dAppConnection/models'; import { DEFERRED_RESPONSE } from '@src/background/connections/middlewares/models'; import { openApprovalWindow } from '@src/background/runtime/openApprovalWindow'; import { canSkipApproval } from '@src/utils/canSkipApproval'; import { AccountsService } from '../AccountsService'; -import { Action } from '../../actions/models'; -import { ImportedAccount, PrimaryAccount } from '../models'; +import type { Action } from '../../actions/models'; +import type { ImportedAccount, PrimaryAccount } from '../models'; import { isPrimaryAccount } from '../utils/typeGuards'; import { SecretsService } from '../../secrets/SecretsService'; @@ -50,10 +50,6 @@ export class AvalancheDeleteAccountsHandler extends DAppRequestHandler< const { request, scope } = rpcCall; const [accountIds] = request.params; - //TODO DELETE THIS - accountIds.unshift('1d6ad089-52b3-40b9-a3e5-ac5a970ba063'); - accountIds.push('75b6b83d-2836-4997-a21f-3ee42f8e6a38'); - if (!request.site?.domain || !request.site?.tabId) { return { ...request, @@ -62,10 +58,6 @@ export class AvalancheDeleteAccountsHandler extends DAppRequestHandler< }), }; } - console.log({ accountIds }); - - const currentAccounts = this.accountsService.getAccounts(); - console.log({ currentAccounts }); const primaryWalletAccounts: PrimaryWalletAccounts = {}; const importedAccounts: ImportedAccount[] = []; @@ -98,17 +90,46 @@ export class AvalancheDeleteAccountsHandler extends DAppRequestHandler< }; } - //Checking if the accounts to be deleted has the latest index + //Check to make sure that all accounts are not being deleted + const allAccounts = this.accountsService.getAccounts(); + const primaryAccountCount = Object.values(allAccounts.primary).flat() + .length; + const importedAccountCount = Object.values(allAccounts.imported).length; + const allAccountCount = primaryAccountCount + importedAccountCount; + + const primaryAccountToDeleteCount = Object.values( + primaryWalletAccounts + ).flat().length; + + const importedAccountCountToDeleteCount = importedAccounts.length; + const deleteAccountCount = + primaryAccountToDeleteCount + importedAccountCountToDeleteCount; + + if (allAccountCount === deleteAccountCount) { + return { + ...request, + error: ethErrors.rpc.invalidParams({ + message: 'Cannot delete all accounts', + }), + }; + } + + //Validating to ensure that the accounts to be deleted has the latest index in the wallet for (const [walletId, accountsInWallet] of Object.entries( primaryWalletAccounts )) { - console.log(`${walletId}: ${accountsInWallet}`); accountsInWallet.sort((a, b) => b.index - a.index); - console.log({ sorted: accountsInWallet }); - - const walletAccounts = - this.accountsService.getPrimaryAccountsByWalletId(walletId); - console.log({ walletAccounts }); + const walletAccounts = allAccounts.primary[walletId]; + + // This should not happen in normal cases. But need it to satisfy typescript + if (!walletAccounts || !walletAccounts.length) { + return { + ...request, + error: ethErrors.rpc.invalidParams({ + message: 'Unable to find the account', + }), + }; + } for (let i = 0; i < accountsInWallet.length; i++) { const accountToDelete = accountsInWallet[i]; @@ -128,26 +149,27 @@ export class AvalancheDeleteAccountsHandler extends DAppRequestHandler< } } - //TODO Uncomment this block - // if (await canSkipApproval(request.site.domain, request.site.tabId)) { - // try { - // await this.accountsService.deleteAccounts(Object.keys(accounts)); - - // return { - // ...request, - // result: null, - // }; - // } catch (err) { - // console.log(err); - // return { - // ...request, - // error: ethErrors.rpc.internal('Account removing failed'), - // }; - // } - // } + if (await canSkipApproval(request.site.domain, request.site.tabId)) { + const allPrimaryAccounts = Object.values(primaryWalletAccounts).flat(); + const allAccount = [...allPrimaryAccounts, ...importedAccounts]; + const allAccountIds = allAccount.map((account) => account.id); + try { + await this.accountsService.deleteAccounts(allAccountIds); + + return { + ...request, + result: null, + }; + } catch { + return { + ...request, + error: ethErrors.rpc.internal('Account removing failed'), + }; + } + } + // Getting the wallet names const walletNames: Record = {}; - const primaryWallets = await this.secretsService.getPrimaryWalletsDetails(); for (const walletId of Object.keys(primaryWalletAccounts)) { @@ -191,16 +213,19 @@ export class AvalancheDeleteAccountsHandler extends DAppRequestHandler< onActionApproved = async ( pendingAction: Action<{ - accounts: Record; + accounts: DeleteAccountsDisplayData; }>, _, onSuccess, onError ) => { try { - const { accounts } = pendingAction.displayData; + const { primary, imported } = pendingAction.displayData.accounts; + const allPrimaryAccounts = Object.values(primary).flat(); + const allAccount = [...allPrimaryAccounts, ...imported]; + const allAccountIds = allAccount.map((account) => account.id); - await this.accountsService.deleteAccounts(Object.keys(accounts)); + await this.accountsService.deleteAccounts(allAccountIds); onSuccess(null); } catch (e) { diff --git a/src/components/common/approval/ApprovalSection.tsx b/src/components/common/approval/ApprovalSection.tsx index 684b60c7..31d70db4 100644 --- a/src/components/common/approval/ApprovalSection.tsx +++ b/src/components/common/approval/ApprovalSection.tsx @@ -30,7 +30,9 @@ export const ApprovalSectionHeader: React.FC = ({ }} > - {label} + + {label} + {tooltip && ( {tooltipIcon} diff --git a/src/pages/Wallet/DeleteAccounts.tsx b/src/pages/Wallet/DeleteAccounts.tsx index 2ec3479b..d2697d25 100644 --- a/src/pages/Wallet/DeleteAccounts.tsx +++ b/src/pages/Wallet/DeleteAccounts.tsx @@ -17,6 +17,7 @@ import { WebsiteDetails } from '../SignTransaction/components/ApprovalTxDetails' import type { DomainMetadata } from '@src/background/models'; import { truncateAddress } from '@src/utils/truncateAddress'; import type { DeleteAccountsDisplayData } from '@src/background/services/accounts/handlers/avalanche_deleteAccounts'; +import { AccountType } from '@src/background/services/accounts/models'; export function DeleteAccount() { const { t } = useTranslation(); @@ -53,11 +54,27 @@ export function DeleteAccount() { name: t('Unknown website'), }; + const getImportedAccountType = ( + type: + | AccountType.IMPORTED + | AccountType.WALLET_CONNECT + | AccountType.FIREBLOCKS + ) => { + switch (type) { + case AccountType.IMPORTED: + return t('From Private Key'); + case AccountType.WALLET_CONNECT: + return t('From Wallet Connect'); + default: + return t('From Fireblocks'); + } + }; + return ( - {accountsCount === 1 ? t('Delete Account?') : t('Delete Accounts?')} + {accountsCount === 1 ? t('Remove Account?') : t('Remove Accounts?')} @@ -69,29 +86,38 @@ export function DeleteAccount() { {Object.keys(action.displayData.accounts.primary) && Object.entries(action.displayData.accounts.primary).map( ([walletId, primaryAccounts], i) => ( - - - - {action.displayData.accounts.wallet[walletId]} - - + {primaryAccounts.map((primaryAccount) => ( - - {primaryAccount.name} - - {truncateAddress(primaryAccount.addressC)} + + + + {primaryAccount.name} + + + {truncateAddress(primaryAccount.addressC)} + + + + {t('From')}{' '} + {action.displayData.accounts.wallet[walletId]} ))} @@ -100,12 +126,7 @@ export function DeleteAccount() { )} {action.displayData.accounts.imported.length && ( - - - {action.displayData.accounts.imported.length === 1 - ? t('Imported Account') - : t('Imported Accounts')} - + {action.displayData.accounts.imported.map((importedAccount) => ( - {importedAccount.name} - + + {importedAccount.name} + + {truncateAddress(importedAccount.addressC)} + + {getImportedAccountType(importedAccount.type)} + ))} @@ -163,7 +193,7 @@ export function DeleteAccount() { }} fullWidth > - {t('Delete')} + {t('Remove')}