From 9b370bd06d3937ed43b1e770a249fe6178f0a8bf Mon Sep 17 00:00:00 2001 From: digiwand <20778143+digiwand@users.noreply.github.com> Date: Fri, 18 Oct 2024 22:06:01 +0800 Subject: [PATCH 01/11] =?UTF-8?q?feat:=20add=20=E2=80=9CIncomplete=20Asset?= =?UTF-8?q?=20Displayed=E2=80=9D=20metric=20&=20fix:=20should=20only=20set?= =?UTF-8?q?=20default=20decimals=20if=20ERC20=20(#27494)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** ### Overall - Adds "Incomplete Asset Displayed" metric when token detail decimals are not found for Permit Simulations of ERC20 tokens - Fixes issue where decimals may default to 18 when the token is not identified as an ERC20 token ### Details - Refactors fetchErc20Decimals - Defines types in token.ts - Create new useGetTokenStandardAndDetails - Includes new `decimalsNumber` value in return to be used instead of `decimals` (string type) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/27333 ## **Manual testing steps** NA ## **Screenshots/Recordings** NA ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] 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. --------- Co-authored-by: Jyoti Puri --- .../confirmations/signatures/permit.spec.ts | 2 +- .../confirmations/signatures/permit.test.tsx | 2 +- .../permit-simulation.test.tsx | 12 ++- .../__snapshots__/value-display.test.tsx.snap | 46 --------- .../value-display/value-display.test.tsx | 21 ++-- .../value-display/value-display.tsx | 21 ++-- .../components/confirm/row/dataTree.tsx | 18 ++-- .../typedSignDataV1.test.tsx | 12 ++- .../useBalanceChanges.test.ts | 4 +- .../confirmations/confirm/confirm.test.tsx | 12 +-- .../useGetTokenStandardAndDetails.test.ts | 50 ++++++++++ .../hooks/useGetTokenStandardAndDetails.ts | 42 ++++++++ ...rackERC20WithoutDecimalInformation.test.ts | 40 ++++++++ .../useTrackERC20WithoutDecimalInformation.ts | 58 +++++++++++ ui/pages/confirmations/utils/token.test.ts | 7 +- ui/pages/confirmations/utils/token.ts | 97 +++++++++++++++---- 16 files changed, 336 insertions(+), 108 deletions(-) create mode 100644 ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.test.ts create mode 100644 ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.ts create mode 100644 ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.test.ts create mode 100644 ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.ts diff --git a/test/e2e/tests/confirmations/signatures/permit.spec.ts b/test/e2e/tests/confirmations/signatures/permit.spec.ts index 5c52d1f029ee..8da5e411a2f4 100644 --- a/test/e2e/tests/confirmations/signatures/permit.spec.ts +++ b/test/e2e/tests/confirmations/signatures/permit.spec.ts @@ -126,7 +126,7 @@ async function assertInfoValues(driver: Driver) { css: '.name__value', text: '0x5B38D...eddC4', }); - const value = driver.findElement({ text: '<0.000001' }); + const value = driver.findElement({ text: '3,000' }); const nonce = driver.findElement({ text: '0' }); const deadline = driver.findElement({ text: '09 June 3554, 16:53' }); diff --git a/test/integration/confirmations/signatures/permit.test.tsx b/test/integration/confirmations/signatures/permit.test.tsx index e11f206d1996..8e9c979562f2 100644 --- a/test/integration/confirmations/signatures/permit.test.tsx +++ b/test/integration/confirmations/signatures/permit.test.tsx @@ -73,7 +73,7 @@ describe('Permit Confirmation', () => { jest.resetAllMocks(); mockedBackgroundConnection.submitRequestToBackground.mockImplementation( createMockImplementation({ - getTokenStandardAndDetails: { decimals: '2' }, + getTokenStandardAndDetails: { decimals: '2', standard: 'ERC20' }, }), ); }); diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.test.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.test.tsx index e89efb3c0dc1..0d67715867d9 100644 --- a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.test.tsx +++ b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.test.tsx @@ -8,15 +8,25 @@ import { permitNFTSignatureMsg, permitSignatureMsg, } from '../../../../../../../../test/data/confirmations/typed_sign'; +import { memoizedGetTokenStandardAndDetails } from '../../../../../utils/token'; import PermitSimulation from './permit-simulation'; jest.mock('../../../../../../../store/actions', () => { return { - getTokenStandardAndDetails: jest.fn().mockResolvedValue({ decimals: 2 }), + getTokenStandardAndDetails: jest + .fn() + .mockResolvedValue({ decimals: 2, standard: 'ERC20' }), }; }); describe('PermitSimulation', () => { + afterEach(() => { + jest.clearAllMocks(); + + /** Reset memoized function using getTokenStandardAndDetails for each test */ + memoizedGetTokenStandardAndDetails?.cache?.clear?.(); + }); + it('renders component correctly', async () => { const state = getMockTypedSignConfirmStateForRequest(permitSignatureMsg); const mockStore = configureMockStore([])(state); diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/__snapshots__/value-display.test.tsx.snap b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/__snapshots__/value-display.test.tsx.snap index 9c4134aa1b2d..26def806c6fa 100644 --- a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/__snapshots__/value-display.test.tsx.snap +++ b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/__snapshots__/value-display.test.tsx.snap @@ -56,49 +56,3 @@ exports[`PermitSimulationValueDisplay renders component correctly 1`] = ` `; - -exports[`PermitSimulationValueDisplay renders component correctly for NFT token 1`] = ` -
-
-
-
-
-

- #4321 -

-
-
-
-
- -

- 0xA0b86...6eB48 -

-
-
-
-
-
-
-`; diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.test.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.test.tsx index da86d497aac1..e8e48c1ca6f9 100644 --- a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.test.tsx +++ b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.test.tsx @@ -4,14 +4,24 @@ import configureMockStore from 'redux-mock-store'; import mockState from '../../../../../../../../../test/data/mock-state.json'; import { renderWithProvider } from '../../../../../../../../../test/lib/render-helpers'; +import useTrackERC20WithoutDecimalInformation from '../../../../../../hooks/useTrackERC20WithoutDecimalInformation'; import PermitSimulationValueDisplay from './value-display'; jest.mock('../../../../../../../../store/actions', () => { return { - getTokenStandardAndDetails: jest.fn().mockResolvedValue({ decimals: 4 }), + getTokenStandardAndDetails: jest + .fn() + .mockResolvedValue({ decimals: 4, standard: 'ERC20' }), }; }); +jest.mock( + '../../../../../../hooks/useTrackERC20WithoutDecimalInformation', + () => { + return jest.fn(); + }, +); + describe('PermitSimulationValueDisplay', () => { it('renders component correctly', async () => { const mockStore = configureMockStore([])(mockState); @@ -30,20 +40,19 @@ describe('PermitSimulationValueDisplay', () => { }); }); - it('renders component correctly for NFT token', async () => { + it('should invoke method to track missing decimal information for ERC20 tokens', async () => { const mockStore = configureMockStore([])(mockState); await act(async () => { - const { container, findByText } = renderWithProvider( + renderWithProvider( , mockStore, ); - expect(await findByText('#4321')).toBeInTheDocument(); - expect(container).toMatchSnapshot(); + expect(useTrackERC20WithoutDecimalInformation).toHaveBeenCalled(); }); }); }); diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.tsx index 360559493596..e95edc03087b 100644 --- a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.tsx +++ b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/value-display/value-display.tsx @@ -2,8 +2,9 @@ import React, { useMemo } from 'react'; import { NameType } from '@metamask/name-controller'; import { Hex } from '@metamask/utils'; import { captureException } from '@sentry/browser'; -import { shortenString } from '../../../../../../../../helpers/utils/util'; +import { MetaMetricsEventLocation } from '../../../../../../../../../shared/constants/metametrics'; +import { shortenString } from '../../../../../../../../helpers/utils/util'; import { calcTokenAmount } from '../../../../../../../../../shared/lib/transactions-controller-utils'; import useTokenExchangeRate from '../../../../../../../../components/app/currency-input/hooks/useTokenExchangeRate'; import { IndividualFiatDisplay } from '../../../../../simulation-details/fiat-display'; @@ -11,7 +12,8 @@ import { formatAmount, formatAmountMaxPrecision, } from '../../../../../simulation-details/formatAmount'; -import { useAsyncResult } from '../../../../../../../../hooks/useAsyncResult'; +import { useGetTokenStandardAndDetails } from '../../../../../../hooks/useGetTokenStandardAndDetails'; +import useTrackERC20WithoutDecimalInformation from '../../../../../../hooks/useTrackERC20WithoutDecimalInformation'; import { Box, @@ -27,7 +29,7 @@ import { TextAlign, } from '../../../../../../../../helpers/constants/design-system'; import Name from '../../../../../../../../components/app/name/name'; -import { fetchErc20Decimals } from '../../../../../../utils/token'; +import { TokenDetailsERC20 } from '../../../../../../utils/token'; type PermitSimulationValueDisplayParams = { /** The primaryType of the typed sign message */ @@ -52,12 +54,13 @@ const PermitSimulationValueDisplay: React.FC< > = ({ primaryType, tokenContract, value, tokenId }) => { const exchangeRate = useTokenExchangeRate(tokenContract); - const { value: tokenDecimals } = useAsyncResult(async () => { - if (tokenId) { - return undefined; - } - return await fetchErc20Decimals(tokenContract); - }, [tokenContract]); + const tokenDetails = useGetTokenStandardAndDetails(tokenContract); + useTrackERC20WithoutDecimalInformation( + tokenContract, + tokenDetails as TokenDetailsERC20, + MetaMetricsEventLocation.SignatureConfirmation, + ); + const { decimalsNumber: tokenDecimals } = tokenDetails; const fiatValue = useMemo(() => { if (exchangeRate && value && !tokenId) { diff --git a/ui/pages/confirmations/components/confirm/row/dataTree.tsx b/ui/pages/confirmations/components/confirm/row/dataTree.tsx index 26c91baed3a6..b295f337deb4 100644 --- a/ui/pages/confirmations/components/confirm/row/dataTree.tsx +++ b/ui/pages/confirmations/components/confirm/row/dataTree.tsx @@ -11,7 +11,6 @@ import { isValidHexAddress } from '../../../../../../shared/modules/hexstring-ut import { sanitizeString } from '../../../../../helpers/utils/util'; import { Box } from '../../../../../components/component-library'; import { BlockSize } from '../../../../../helpers/constants/design-system'; -import { useAsyncResult } from '../../../../../hooks/useAsyncResult'; import { useI18nContext } from '../../../../../hooks/useI18nContext'; import { ConfirmInfoRow, @@ -20,7 +19,7 @@ import { ConfirmInfoRowText, ConfirmInfoRowTextTokenUnits, } from '../../../../../components/app/confirm/info/row'; -import { fetchErc20Decimals } from '../../../utils/token'; +import { useGetTokenStandardAndDetails } from '../../../hooks/useGetTokenStandardAndDetails'; type ValueType = string | Record | TreeData[]; @@ -78,9 +77,9 @@ const NONE_DATE_VALUE = -1; * * @param dataTreeData */ -const getTokenDecimalsOfDataTree = async ( +const getTokenContractInDataTree = ( dataTreeData: Record | TreeData[], -): Promise => { +): Hex | undefined => { if (Array.isArray(dataTreeData)) { return undefined; } @@ -91,7 +90,7 @@ const getTokenDecimalsOfDataTree = async ( return undefined; } - return await fetchErc20Decimals(tokenContract); + return tokenContract; }; export const DataTree = ({ @@ -103,13 +102,10 @@ export const DataTree = ({ primaryType?: PrimaryType; tokenDecimals?: number; }) => { - const { value: decimalsResponse } = useAsyncResult( - async () => await getTokenDecimalsOfDataTree(data), - [data], - ); - + const tokenContract = getTokenContractInDataTree(data); + const { decimalsNumber } = useGetTokenStandardAndDetails(tokenContract); const tokenDecimals = - typeof decimalsResponse === 'number' ? decimalsResponse : tokenDecimalsProp; + typeof decimalsNumber === 'number' ? decimalsNumber : tokenDecimalsProp; return ( diff --git a/ui/pages/confirmations/components/confirm/row/typed-sign-data-v1/typedSignDataV1.test.tsx b/ui/pages/confirmations/components/confirm/row/typed-sign-data-v1/typedSignDataV1.test.tsx index 9563b5523f39..ecf55e3b574d 100644 --- a/ui/pages/confirmations/components/confirm/row/typed-sign-data-v1/typedSignDataV1.test.tsx +++ b/ui/pages/confirmations/components/confirm/row/typed-sign-data-v1/typedSignDataV1.test.tsx @@ -1,22 +1,28 @@ import React from 'react'; -import { render } from '@testing-library/react'; +import configureMockStore from 'redux-mock-store'; +import mockState from '../../../../../../../test/data/mock-state.json'; +import { renderWithProvider } from '../../../../../../../test/lib/render-helpers'; import { unapprovedTypedSignMsgV1 } from '../../../../../../../test/data/confirmations/typed_sign'; import { TypedSignDataV1Type } from '../../../../types/confirm'; import { ConfirmInfoRowTypedSignDataV1 } from './typedSignDataV1'; +const mockStore = configureMockStore([])(mockState); + describe('ConfirmInfoRowTypedSignData', () => { it('should match snapshot', () => { - const { container } = render( + const { container } = renderWithProvider( , + mockStore, ); expect(container).toMatchSnapshot(); }); it('should return null if data is not defined', () => { - const { container } = render( + const { container } = renderWithProvider( , + mockStore, ); expect(container).toBeEmptyDOMElement(); }); diff --git a/ui/pages/confirmations/components/simulation-details/useBalanceChanges.test.ts b/ui/pages/confirmations/components/simulation-details/useBalanceChanges.test.ts index 10e4cca518b7..5dc0be870538 100644 --- a/ui/pages/confirmations/components/simulation-details/useBalanceChanges.test.ts +++ b/ui/pages/confirmations/components/simulation-details/useBalanceChanges.test.ts @@ -9,7 +9,7 @@ import { TokenStandard } from '../../../../../shared/constants/transaction'; import { getConversionRate } from '../../../../ducks/metamask/metamask'; import { getTokenStandardAndDetails } from '../../../../store/actions'; import { fetchTokenExchangeRates } from '../../../../helpers/utils/util'; -import { fetchErc20Decimals } from '../../utils/token'; +import { memoizedGetTokenStandardAndDetails } from '../../utils/token'; import { useBalanceChanges } from './useBalanceChanges'; import { FIAT_UNAVAILABLE } from './types'; @@ -92,7 +92,7 @@ describe('useBalanceChanges', () => { afterEach(() => { /** Reset memoized function for each test */ - fetchErc20Decimals?.cache?.clear?.(); + memoizedGetTokenStandardAndDetails?.cache?.clear?.(); }); describe('pending states', () => { diff --git a/ui/pages/confirmations/confirm/confirm.test.tsx b/ui/pages/confirmations/confirm/confirm.test.tsx index d6b2dd704fb8..939ca8768afe 100644 --- a/ui/pages/confirmations/confirm/confirm.test.tsx +++ b/ui/pages/confirmations/confirm/confirm.test.tsx @@ -17,7 +17,7 @@ import mockState from '../../../../test/data/mock-state.json'; import { renderWithConfirmContextProvider } from '../../../../test/lib/confirmations/render-helpers'; import * as actions from '../../../store/actions'; import { SignatureRequestType } from '../types/confirm'; -import { fetchErc20Decimals } from '../utils/token'; +import { memoizedGetTokenStandardAndDetails } from '../utils/token'; import Confirm from './confirm'; jest.mock('react-router-dom', () => ({ @@ -34,7 +34,7 @@ describe('Confirm', () => { jest.resetAllMocks(); /** Reset memoized function using getTokenStandardAndDetails for each test */ - fetchErc20Decimals?.cache?.clear?.(); + memoizedGetTokenStandardAndDetails?.cache?.clear?.(); }); it('should render', () => { @@ -59,7 +59,7 @@ describe('Confirm', () => { jest.spyOn(actions, 'getTokenStandardAndDetails').mockResolvedValue({ decimals: '2', - standard: 'erc20', + standard: 'ERC20', }); const mockStore = configureMockStore(middleware)(mockStateTypedSign); @@ -103,7 +103,7 @@ describe('Confirm', () => { jest.spyOn(actions, 'getTokenStandardAndDetails').mockResolvedValue({ decimals: '2', - standard: 'erc20', + standard: 'ERC20', }); const mockStore = configureMockStore(middleware)(mockStateTypedSign); @@ -146,7 +146,7 @@ describe('Confirm', () => { jest.spyOn(actions, 'getTokenStandardAndDetails').mockResolvedValue({ decimals: '2', - standard: 'erc20', + standard: 'ERC20', }); await act(async () => { @@ -170,7 +170,7 @@ describe('Confirm', () => { jest.spyOn(actions, 'getTokenStandardAndDetails').mockResolvedValue({ decimals: '2', - standard: 'erc20', + standard: 'ERC20', }); await act(async () => { diff --git a/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.test.ts b/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.test.ts new file mode 100644 index 000000000000..7cd217db3a85 --- /dev/null +++ b/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.test.ts @@ -0,0 +1,50 @@ +import { renderHook } from '@testing-library/react-hooks'; +import { waitFor } from '@testing-library/react'; + +import * as TokenActions from '../utils/token'; +import { useGetTokenStandardAndDetails } from './useGetTokenStandardAndDetails'; + +jest.mock('react-redux', () => ({ + ...jest.requireActual('react-redux'), + useSelector: () => 0x1, +})); + +jest.mock('react', () => ({ + ...jest.requireActual('react'), + useContext: jest.fn(), +})); + +jest.mock('../../../store/actions', () => { + return { + getTokenStandardAndDetails: jest + .fn() + .mockResolvedValue({ decimals: 2, standard: 'ERC20' }), + }; +}); + +describe('useGetTokenStandardAndDetails', () => { + it('should return token details', () => { + const { result } = renderHook(() => useGetTokenStandardAndDetails('0x5')); + expect(result.current).toEqual({ decimalsNumber: undefined }); + }); + + it('should return token details obtained from getTokenStandardAndDetails action', async () => { + jest + .spyOn(TokenActions, 'memoizedGetTokenStandardAndDetails') + .mockResolvedValue({ + standard: 'ERC20', + } as TokenActions.TokenDetailsERC20); + const { result, rerender } = renderHook(() => + useGetTokenStandardAndDetails('0x5'), + ); + + rerender(); + + await waitFor(() => { + expect(result.current).toEqual({ + decimalsNumber: 18, + standard: 'ERC20', + }); + }); + }); +}); diff --git a/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.ts b/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.ts new file mode 100644 index 000000000000..88dfb0a12b9d --- /dev/null +++ b/ui/pages/confirmations/hooks/useGetTokenStandardAndDetails.ts @@ -0,0 +1,42 @@ +import { Hex } from '@metamask/utils'; + +import { TokenStandard } from '../../../../shared/constants/transaction'; +import { useAsyncResult } from '../../../hooks/useAsyncResult'; +import { + ERC20_DEFAULT_DECIMALS, + parseTokenDetailDecimals, + memoizedGetTokenStandardAndDetails, + TokenDetailsERC20, +} from '../utils/token'; + +/** + * Returns token details for a given token contract + * + * @param tokenAddress + * @returns + */ +export const useGetTokenStandardAndDetails = ( + tokenAddress: Hex | string | undefined, +) => { + const { value: details } = useAsyncResult( + async () => + (await memoizedGetTokenStandardAndDetails( + tokenAddress, + )) as TokenDetailsERC20, + [tokenAddress], + ); + + if (!details) { + return { decimalsNumber: undefined }; + } + + const { decimals, standard } = details || {}; + + if (standard === TokenStandard.ERC20) { + const parsedDecimals = + parseTokenDetailDecimals(decimals) ?? ERC20_DEFAULT_DECIMALS; + details.decimalsNumber = parsedDecimals; + } + + return details; +}; diff --git a/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.test.ts b/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.test.ts new file mode 100644 index 000000000000..dff0103fbe21 --- /dev/null +++ b/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.test.ts @@ -0,0 +1,40 @@ +import { renderHook } from '@testing-library/react-hooks'; +import { useContext } from 'react'; + +import { TokenStandard } from '../../../../shared/constants/transaction'; +import { MetaMetricsContext } from '../../../contexts/metametrics'; +import { TokenDetailsERC20 } from '../utils/token'; +import useTrackERC20WithoutDecimalInformation from './useTrackERC20WithoutDecimalInformation'; + +jest.mock('react-redux', () => ({ + ...jest.requireActual('react-redux'), + useSelector: () => 0x1, +})); + +jest.mock('react', () => ({ + ...jest.requireActual('react'), + useContext: jest.fn(), +})); + +describe('useTrackERC20WithoutDecimalInformation', () => { + const useContextMock = jest.mocked(useContext); + + const trackEventMock = jest.fn(); + + it('should invoke trackEvent method', () => { + useContextMock.mockImplementation((context) => { + if (context === MetaMetricsContext) { + return trackEventMock; + } + return undefined; + }); + + renderHook(() => + useTrackERC20WithoutDecimalInformation('0x5', { + standard: TokenStandard.ERC20, + } as TokenDetailsERC20), + ); + + expect(trackEventMock).toHaveBeenCalled(); + }); +}); diff --git a/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.ts b/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.ts new file mode 100644 index 000000000000..fa6a5e620fc4 --- /dev/null +++ b/ui/pages/confirmations/hooks/useTrackERC20WithoutDecimalInformation.ts @@ -0,0 +1,58 @@ +import { useSelector } from 'react-redux'; +import { useContext, useEffect } from 'react'; +import { Hex } from '@metamask/utils'; + +import { + MetaMetricsEventCategory, + MetaMetricsEventLocation, + MetaMetricsEventName, + MetaMetricsEventUiCustomization, +} from '../../../../shared/constants/metametrics'; +import { TokenStandard } from '../../../../shared/constants/transaction'; +import { MetaMetricsContext } from '../../../contexts/metametrics'; +import { getCurrentChainId } from '../../../selectors'; +import { parseTokenDetailDecimals, TokenDetailsERC20 } from '../utils/token'; + +/** + * Track event that number of decimals in ERC20 is not obtained + * + * @param tokenAddress + * @param tokenDetails + * @param metricLocation + */ +const useTrackERC20WithoutDecimalInformation = ( + tokenAddress: Hex | string | undefined, + tokenDetails?: TokenDetailsERC20, + metricLocation = MetaMetricsEventLocation.SignatureConfirmation, +) => { + const trackEvent = useContext(MetaMetricsContext); + const chainId = useSelector(getCurrentChainId); + + useEffect(() => { + if (chainId === undefined || tokenDetails === undefined) { + return; + } + const { decimals, standard } = tokenDetails || {}; + if (standard === TokenStandard.ERC20) { + const parsedDecimals = parseTokenDetailDecimals(decimals); + if (parsedDecimals === undefined) { + trackEvent({ + event: MetaMetricsEventName.SimulationIncompleteAssetDisplayed, + category: MetaMetricsEventCategory.Confirmations, + properties: { + token_decimals_available: false, + asset_address: tokenAddress, + asset_type: TokenStandard.ERC20, + chain_id: chainId, + location: metricLocation, + ui_customizations: [ + MetaMetricsEventUiCustomization.RedesignedConfirmation, + ], + }, + }); + } + } + }, [tokenDetails, chainId, tokenAddress, trackEvent]); +}; + +export default useTrackERC20WithoutDecimalInformation; diff --git a/ui/pages/confirmations/utils/token.test.ts b/ui/pages/confirmations/utils/token.test.ts index e71813713d79..250bff90c07c 100644 --- a/ui/pages/confirmations/utils/token.test.ts +++ b/ui/pages/confirmations/utils/token.test.ts @@ -1,6 +1,9 @@ import { getTokenStandardAndDetails } from '../../../store/actions'; import { ERC20_DEFAULT_DECIMALS } from '../constants/token'; -import { fetchErc20Decimals } from './token'; +import { + fetchErc20Decimals, + memoizedGetTokenStandardAndDetails, +} from './token'; const MOCK_ADDRESS = '0x514910771af9ca656af840dff83e8264ecf986ca'; const MOCK_DECIMALS = 36; @@ -14,7 +17,7 @@ describe('fetchErc20Decimals', () => { jest.clearAllMocks(); /** Reset memoized function using getTokenStandardAndDetails for each test */ - fetchErc20Decimals?.cache?.clear?.(); + memoizedGetTokenStandardAndDetails?.cache?.clear?.(); }); it(`should return the default number, ${ERC20_DEFAULT_DECIMALS}, if no decimals were found from details`, async () => { diff --git a/ui/pages/confirmations/utils/token.ts b/ui/pages/confirmations/utils/token.ts index 1f94280129a9..3a8c3a2a671e 100644 --- a/ui/pages/confirmations/utils/token.ts +++ b/ui/pages/confirmations/utils/token.ts @@ -1,32 +1,89 @@ import { memoize } from 'lodash'; import { Hex } from '@metamask/utils'; +import { AssetsContractController } from '@metamask/assets-controllers'; import { getTokenStandardAndDetails } from '../../../store/actions'; +export type TokenDetailsERC20 = Awaited< + ReturnType< + ReturnType['getDetails'] + > +> & { decimalsNumber: number }; + +export type TokenDetailsERC721 = Awaited< + ReturnType< + ReturnType['getDetails'] + > +>; + +export type TokenDetailsERC1155 = Awaited< + ReturnType< + ReturnType['getDetails'] + > +>; + +export type TokenDetails = + | TokenDetailsERC20 + | TokenDetailsERC721 + | TokenDetailsERC1155; + export const ERC20_DEFAULT_DECIMALS = 18; -/** - * Fetches the decimals for the given token address. - * - * @param {Hex | string} address - The ethereum token contract address. It is expected to be in hex format. - * We currently accept strings since we have a patch that accepts a custom string - * {@see .yarn/patches/@metamask-eth-json-rpc-middleware-npm-14.0.1-b6c2ccbe8c.patch} - */ -export const fetchErc20Decimals = memoize( - async (address: Hex | string): Promise => { +export const parseTokenDetailDecimals = ( + decStr?: string, +): number | undefined => { + if (!decStr) { + return undefined; + } + + for (const radix of [10, 16]) { + const parsedDec = parseInt(decStr, radix); + if (isFinite(parsedDec)) { + return parsedDec; + } + } + return undefined; +}; + +export const memoizedGetTokenStandardAndDetails = memoize( + async ( + tokenAddress?: Hex | string, + userAddress?: string, + tokenId?: string, + ): Promise> => { try { - const { decimals: decStr } = await getTokenStandardAndDetails(address); - if (!decStr) { - return ERC20_DEFAULT_DECIMALS; - } - for (const radix of [10, 16]) { - const parsedDec = parseInt(decStr, radix); - if (isFinite(parsedDec)) { - return parsedDec; - } + if (!tokenAddress) { + return {}; } - return ERC20_DEFAULT_DECIMALS; + + return (await getTokenStandardAndDetails( + tokenAddress, + userAddress, + tokenId, + )) as TokenDetails; } catch { - return ERC20_DEFAULT_DECIMALS; + return {}; } }, ); + +/** + * Fetches the decimals for the given token address. + * + * @param address - The ethereum token contract address. It is expected to be in hex format. + * We currently accept strings since we have a patch that accepts a custom string + * {@see .yarn/patches/@metamask-eth-json-rpc-middleware-npm-14.0.1-b6c2ccbe8c.patch} + */ +export const fetchErc20Decimals = async ( + address: Hex | string, +): Promise => { + try { + const { decimals: decStr } = (await memoizedGetTokenStandardAndDetails( + address, + )) as TokenDetailsERC20; + const decimals = parseTokenDetailDecimals(decStr); + + return decimals ?? ERC20_DEFAULT_DECIMALS; + } catch { + return ERC20_DEFAULT_DECIMALS; + } +}; From c44fb0b2d507e42de099e4fcbd5c360c7d812da1 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Fri, 18 Oct 2024 17:30:07 +0200 Subject: [PATCH 02/11] fix: lint-lockfile flaky job by changing resources from medium to medium-plus (#27950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The lint-lockfile job is flaky. It seems it's under-resourced peaking max cpu and ram values, as @Gudahtt pointed out: ![Screenshot from 2024-10-18 09-14-33](https://github.com/user-attachments/assets/bf9c4d06-31c5-47e0-885a-dde85e49def2) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27950?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/27806 ## **Manual testing steps** 1. Check lint-lockfile job ## **Screenshots/Recordings** Increasing resources from medium to medium-plus https://circleci.com/pricing/price-list/ ![Screenshot from 2024-10-18 09-39-13](https://github.com/user-attachments/assets/d473d8f5-8c9d-442e-8267-71deca2834dc) Resource usage after the change: ![Screenshot from 2024-10-18 09-33-41](https://github.com/user-attachments/assets/0e8dfd50-275c-4c1e-a33e-169923cda987) ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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. --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 2bf244b9bf8a..74815818582f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1008,7 +1008,7 @@ jobs: command: ./development/shellcheck.sh test-lint-lockfile: - executor: node-browsers-medium + executor: node-browsers-medium-plus steps: - run: *shallow-git-clone-and-enable-vnc - run: sudo corepack enable From 9ac03643934f42c138bdbca0345809942a8a1ea0 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Fri, 18 Oct 2024 17:36:43 +0200 Subject: [PATCH 03/11] fix: flaky test `Confirmation Redesign ERC721 Approve Component Submit an Approve transaction @no-mmi Sends a type 2 transaction (EIP1559)` (#27928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Removing anti-patterns from erc721 tests. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27928?quickstart=1) ## **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 Extension 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. --- .../erc721-approve-redesign.spec.ts | 14 +++++------ .../tokens/nft/erc721-interaction.spec.js | 23 ++++++------------- 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts index f91b1e8ba1d2..c7ceb6c42c94 100644 --- a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ import { MockttpServer } from 'mockttp'; -import { veryLargeDelayMs, WINDOW_TITLES } from '../../../helpers'; +import { WINDOW_TITLES } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; import { scrollAndConfirmAndAssertConfirm } from '../helpers'; import { @@ -119,8 +119,6 @@ async function createMintTransaction(driver: Driver) { } export async function confirmMintTransaction(driver: Driver) { - await driver.waitUntilXWindowHandles(3); - await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); await driver.waitForSelector({ @@ -129,6 +127,12 @@ export async function confirmMintTransaction(driver: Driver) { }); await scrollAndConfirmAndAssertConfirm(driver); + + // Verify Mint Transaction is Confirmed before proceeding + await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); + await driver.clickElement('[data-testid="account-overview__activity-tab"]'); + await driver.waitForSelector('.transaction-status-label--confirmed'); + await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); } async function createApproveTransaction(driver: Driver) { @@ -137,8 +141,6 @@ async function createApproveTransaction(driver: Driver) { } async function assertApproveDetails(driver: Driver) { - await driver.delay(veryLargeDelayMs); - await driver.waitUntilXWindowHandles(3); await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); await driver.waitForSelector({ @@ -191,8 +193,6 @@ async function assertApproveDetails(driver: Driver) { async function confirmApproveTransaction(driver: Driver) { await scrollAndConfirmAndAssertConfirm(driver); - - await driver.delay(veryLargeDelayMs); await driver.waitUntilXWindowHandles(2); await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); diff --git a/test/e2e/tests/tokens/nft/erc721-interaction.spec.js b/test/e2e/tests/tokens/nft/erc721-interaction.spec.js index 9ebc247ea795..35750bae6d2c 100644 --- a/test/e2e/tests/tokens/nft/erc721-interaction.spec.js +++ b/test/e2e/tests/tokens/nft/erc721-interaction.spec.js @@ -51,19 +51,17 @@ describe('ERC721 NFTs testdapp interaction', function () { await driver.clickElement( '[data-testid="account-overview__activity-tab"]', ); - const transactionItem = await driver.waitForSelector({ + await driver.waitForSelector({ css: '[data-testid="activity-list-item-action"]', text: 'Deposit', }); - assert.equal(await transactionItem.isDisplayed(), true); // verify the mint transaction has finished await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); - const nftsMintStatus = await driver.findElement({ + await driver.waitForSelector({ css: '#nftsStatus', text: 'Mint completed', }); - assert.equal(await nftsMintStatus.isDisplayed(), true); await driver.switchToWindowWithTitle( WINDOW_TITLES.ExtensionInFullScreenView, @@ -116,11 +114,10 @@ describe('ERC721 NFTs testdapp interaction', function () { await driver.clickElement( '[data-testid="account-overview__activity-tab"]', ); - const transactionItem = await driver.waitForSelector({ + await driver.waitForSelector({ css: '[data-testid="activity-list-item-action"]', text: 'Deposit', }); - assert.equal(await transactionItem.isDisplayed(), true); // verify the mint transaction has finished await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); @@ -138,7 +135,6 @@ describe('ERC721 NFTs testdapp interaction', function () { await driver.fill('#watchNFTInput', '3'); await driver.clickElement({ text: 'Watch NFT', tag: 'button' }); - await driver.waitUntilXWindowHandles(3); await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); // avoid race condition @@ -251,11 +247,10 @@ describe('ERC721 NFTs testdapp interaction', function () { }); // verify the mint transaction has finished await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); - const nftsMintStatus = await driver.findElement({ + await driver.waitForSelector({ css: '#nftsStatus', text: 'Mint completed', }); - assert.equal(await nftsMintStatus.isDisplayed(), true); // watch all nfts await driver.clickElement({ text: 'Watch all NFTs', tag: 'button' }); @@ -322,7 +317,6 @@ describe('ERC721 NFTs testdapp interaction', function () { // Click Transfer await driver.fill('#transferTokenInput', '1'); await driver.clickElement('#transferFromButton'); - await driver.waitUntilXWindowHandles(3); await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); // Confirm transfer @@ -407,11 +401,10 @@ describe('ERC721 NFTs testdapp interaction', function () { ); // Verify transaction - const completedTx = await driver.waitForSelector({ + await driver.waitForSelector({ css: '[data-testid="activity-list-item-action"]', text: 'Approve TDN spending cap', }); - assert.equal(await completedTx.isDisplayed(), true); }, ); }); @@ -474,11 +467,10 @@ describe('ERC721 NFTs testdapp interaction', function () { ); // Verify transaction - const completedTx = await driver.waitForSelector({ + await driver.waitForSelector({ css: '[data-testid="activity-list-item-action"]', text: 'Approve TDN with no spend limit', }); - assert.equal(await completedTx.isDisplayed(), true); }, ); }); @@ -544,11 +536,10 @@ describe('ERC721 NFTs testdapp interaction', function () { ); // Verify transaction - const completedTx = await driver.waitForSelector({ + await driver.waitForSelector({ css: '[data-testid="activity-list-item-action"]', text: 'Approve TDN with no spend limit', }); - assert.equal(await completedTx.isDisplayed(), true); }, ); }); From 7ae2c9409531cab077182c5e1c020c731e475e1b Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 18 Oct 2024 23:38:01 +0800 Subject: [PATCH 04/11] feat: add BTC send flow (#27964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR enables the send feature for `@metamask/bitcoin-wallet-snap` ## **Related issues** Fixes: https://github.com/MetaMask/accounts-planning/issues/564 ## **Manual testing steps** 1. Create a btc account 2. Switch to the btc account 3. Click send ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension 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. --- package.json | 2 +- shared/lib/accounts/bitcoin-wallet-snap.ts | 1 - .../flask/btc/btc-account-overview.spec.ts | 2 +- .../app/wallet-overview/btc-overview.test.tsx | 9 +- .../app/wallet-overview/btc-overview.tsx | 2 +- .../app/wallet-overview/coin-buttons.tsx | 96 +++++++++++++++---- ui/store/actions.ts | 20 ++++ yarn.lock | 10 +- 8 files changed, 109 insertions(+), 33 deletions(-) diff --git a/package.json b/package.json index 6c52604d6b84..d01735a7e755 100644 --- a/package.json +++ b/package.json @@ -302,7 +302,7 @@ "@metamask/approval-controller": "^7.0.0", "@metamask/assets-controllers": "patch:@metamask/assets-controllers@npm%3A38.3.0#~/.yarn/patches/@metamask-assets-controllers-npm-38.3.0-57b3d695bb.patch", "@metamask/base-controller": "^7.0.0", - "@metamask/bitcoin-wallet-snap": "^0.7.0", + "@metamask/bitcoin-wallet-snap": "^0.8.1", "@metamask/browser-passworder": "^4.3.0", "@metamask/contract-metadata": "^2.5.0", "@metamask/controller-utils": "^11.2.0", diff --git a/shared/lib/accounts/bitcoin-wallet-snap.ts b/shared/lib/accounts/bitcoin-wallet-snap.ts index 58f367b173e1..c068e4e8e35c 100644 --- a/shared/lib/accounts/bitcoin-wallet-snap.ts +++ b/shared/lib/accounts/bitcoin-wallet-snap.ts @@ -3,7 +3,6 @@ import { SnapId } from '@metamask/snaps-sdk'; // the Snap is being pre-installed only for Flask build (for the moment). import BitcoinWalletSnap from '@metamask/bitcoin-wallet-snap/dist/preinstalled-snap.json'; -// export const BITCOIN_WALLET_SNAP_ID: SnapId = 'local:http://localhost:8080'; export const BITCOIN_WALLET_SNAP_ID: SnapId = BitcoinWalletSnap.snapId as SnapId; diff --git a/test/e2e/flask/btc/btc-account-overview.spec.ts b/test/e2e/flask/btc/btc-account-overview.spec.ts index 24eedb60b6a2..f32a48d9c4a8 100644 --- a/test/e2e/flask/btc/btc-account-overview.spec.ts +++ b/test/e2e/flask/btc/btc-account-overview.spec.ts @@ -16,7 +16,7 @@ describe('BTC Account - Overview', function (this: Suite) { await driver.waitForSelector({ text: 'Send', tag: 'button', - css: '[disabled]', + css: '[data-testid="coin-overview-send"]', }); await driver.waitForSelector({ diff --git a/ui/components/app/wallet-overview/btc-overview.test.tsx b/ui/components/app/wallet-overview/btc-overview.test.tsx index c7bb501ee98f..abff2cb2b239 100644 --- a/ui/components/app/wallet-overview/btc-overview.test.tsx +++ b/ui/components/app/wallet-overview/btc-overview.test.tsx @@ -19,7 +19,6 @@ const BTC_OVERVIEW_BUY = 'coin-overview-buy'; const BTC_OVERVIEW_BRIDGE = 'coin-overview-bridge'; const BTC_OVERVIEW_RECEIVE = 'coin-overview-receive'; const BTC_OVERVIEW_SWAP = 'token-overview-button-swap'; -const BTC_OVERVIEW_SEND = 'coin-overview-send'; const BTC_OVERVIEW_PRIMARY_CURRENCY = 'coin-overview__primary-currency'; const mockMetaMetricsId = 'deadbeef'; @@ -158,14 +157,10 @@ describe('BtcOverview', () => { expect(spinner).toBeInTheDocument(); }); - it('buttons Send/Swap/Bridge are disabled', () => { + it('buttons Swap/Bridge are disabled', () => { const { queryByTestId } = renderWithProvider(, getStore()); - for (const buttonTestId of [ - BTC_OVERVIEW_SEND, - BTC_OVERVIEW_SWAP, - BTC_OVERVIEW_BRIDGE, - ]) { + for (const buttonTestId of [BTC_OVERVIEW_SWAP, BTC_OVERVIEW_BRIDGE]) { const button = queryByTestId(buttonTestId); expect(button).toBeInTheDocument(); expect(button).toBeDisabled(); diff --git a/ui/components/app/wallet-overview/btc-overview.tsx b/ui/components/app/wallet-overview/btc-overview.tsx index e5d0b0103805..dc47df7567b5 100644 --- a/ui/components/app/wallet-overview/btc-overview.tsx +++ b/ui/components/app/wallet-overview/btc-overview.tsx @@ -27,7 +27,7 @@ const BtcOverview = ({ className }: BtcOverviewProps) => { balanceIsCached={false} className={className} chainId={chainId} - isSigningEnabled={false} + isSigningEnabled={true} isSwapsChain={false} ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) isBridgeChain={false} diff --git a/ui/components/app/wallet-overview/coin-buttons.tsx b/ui/components/app/wallet-overview/coin-buttons.tsx index 63bcdd2f58e6..bac7872c79e3 100644 --- a/ui/components/app/wallet-overview/coin-buttons.tsx +++ b/ui/components/app/wallet-overview/coin-buttons.tsx @@ -1,4 +1,11 @@ -import React, { useCallback, useContext, useState } from 'react'; +import React, { + useCallback, + useContext, + useState, + ///: BEGIN:ONLY_INCLUDE_IF(build-flask) + useEffect, + ///: END:ONLY_INCLUDE_IF +} from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { useHistory, @@ -16,6 +23,9 @@ import { CaipChainId, } from '@metamask/utils'; +///: BEGIN:ONLY_INCLUDE_IF(build-flask) +import { BtcAccountType } from '@metamask/keyring-api'; +///: END:ONLY_INCLUDE_IF ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) import { ChainId } from '../../../../shared/constants/network'; ///: END:ONLY_INCLUDE_IF @@ -27,6 +37,9 @@ import { ///: END:ONLY_INCLUDE_IF import { I18nContext } from '../../../contexts/i18n'; import { + ///: BEGIN:ONLY_INCLUDE_IF(build-flask) + CONFIRMATION_V_NEXT_ROUTE, + ///: END:ONLY_INCLUDE_IF ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) PREPARE_SWAP_ROUTE, ///: END:ONLY_INCLUDE_IF @@ -39,6 +52,9 @@ import { ///: END:ONLY_INCLUDE_IF getUseExternalServices, getSelectedAccount, + ///: BEGIN:ONLY_INCLUDE_IF(build-flask) + getMemoizedUnapprovedTemplatedConfirmations, + ///: END:ONLY_INCLUDE_IF } from '../../../selectors'; import Tooltip from '../../ui/tooltip'; ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) @@ -67,6 +83,13 @@ import useRamps from '../../../hooks/ramps/useRamps/useRamps'; import useBridging from '../../../hooks/bridge/useBridging'; ///: END:ONLY_INCLUDE_IF import { ReceiveModal } from '../../multichain/receive-modal'; +///: BEGIN:ONLY_INCLUDE_IF(build-flask) +import { + sendMultichainTransaction, + setDefaultHomeActiveTabName, +} from '../../../store/actions'; +import { BITCOIN_WALLET_SNAP_ID } from '../../../../shared/lib/accounts/bitcoin-wallet-snap'; +///: END:ONLY_INCLUDE_IF const CoinButtons = ({ chainId, @@ -99,7 +122,8 @@ const CoinButtons = ({ const trackEvent = useContext(MetaMetricsContext); const [showReceiveModal, setShowReceiveModal] = useState(false); - const { address: selectedAddress } = useSelector(getSelectedAccount); + const account = useSelector(getSelectedAccount); + const { address: selectedAddress } = account; const history = useHistory(); ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) const location = useLocation(); @@ -230,23 +254,61 @@ const CoinButtons = ({ const { openBridgeExperience } = useBridging(); ///: END:ONLY_INCLUDE_IF - const handleSendOnClick = useCallback(async () => { - trackEvent( - { - event: MetaMetricsEventName.NavSendButtonClicked, - category: MetaMetricsEventCategory.Navigation, - properties: { - token_symbol: 'ETH', - location: 'Home', - text: 'Send', - chain_id: chainId, - }, + ///: BEGIN:ONLY_INCLUDE_IF(build-flask) + const unapprovedTemplatedConfirmations = useSelector( + getMemoizedUnapprovedTemplatedConfirmations, + ); + + useEffect(() => { + const templatedSnapApproval = unapprovedTemplatedConfirmations.find( + (approval) => { + return ( + approval.type === 'snap_dialog' && + approval.origin === BITCOIN_WALLET_SNAP_ID + ); }, - { excludeMetaMetricsId: false }, ); - await dispatch(startNewDraftTransaction({ type: AssetType.native })); - history.push(SEND_ROUTE); - }, [chainId]); + + if (templatedSnapApproval) { + history.push(`${CONFIRMATION_V_NEXT_ROUTE}/${templatedSnapApproval.id}`); + } + }, [unapprovedTemplatedConfirmations, history]); + ///: END:ONLY_INCLUDE_IF + + const handleSendOnClick = useCallback(async () => { + switch (account.type) { + ///: BEGIN:ONLY_INCLUDE_IF(build-flask) + case BtcAccountType.P2wpkh: { + await sendMultichainTransaction( + BITCOIN_WALLET_SNAP_ID, + account.id, + chainId as CaipChainId, + ); + + // We automatically switch to the activity tab once the transaction has been sent. + dispatch(setDefaultHomeActiveTabName('activity')); + break; + } + ///: END:ONLY_INCLUDE_IF + default: { + trackEvent( + { + event: MetaMetricsEventName.NavSendButtonClicked, + category: MetaMetricsEventCategory.Navigation, + properties: { + token_symbol: 'ETH', + location: 'Home', + text: 'Send', + chain_id: chainId, + }, + }, + { excludeMetaMetricsId: false }, + ); + await dispatch(startNewDraftTransaction({ type: AssetType.native })); + history.push(SEND_ROUTE); + } + } + }, [chainId, account]); const handleSwapOnClick = useCallback(async () => { ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) diff --git a/ui/store/actions.ts b/ui/store/actions.ts index a81dabb5e5c6..f7d839946635 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -43,6 +43,7 @@ import { InterfaceState } from '@metamask/snaps-sdk'; import { KeyringTypes } from '@metamask/keyring-controller'; import type { NotificationServicesController } from '@metamask/notification-services-controller'; import { Patch } from 'immer'; +import { HandlerType } from '@metamask/snaps-utils'; import switchDirection from '../../shared/lib/switch-direction'; import { ENVIRONMENT_TYPE_NOTIFICATION, @@ -5841,3 +5842,22 @@ function applyPatches( return newState; } + +export async function sendMultichainTransaction( + snapId: string, + account: string, + scope: string, +) { + await handleSnapRequest({ + snapId, + origin: 'metamask', + handler: HandlerType.OnRpcRequest, + request: { + method: 'startSendTransactionFlow', + params: { + account, + scope, + }, + }, + }); +} diff --git a/yarn.lock b/yarn.lock index 52894233bfab..af059f8960e9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4982,10 +4982,10 @@ __metadata: languageName: node linkType: hard -"@metamask/bitcoin-wallet-snap@npm:^0.7.0": - version: 0.7.0 - resolution: "@metamask/bitcoin-wallet-snap@npm:0.7.0" - checksum: 10/be4eceef1715c5e6d33d095d5b4aaa974656d945ff0ed0304fdc1244eb8940eb8978f304378367642aa8fd60d6b375eecc2a4653c38ba62ec306c03955c96682 +"@metamask/bitcoin-wallet-snap@npm:^0.8.1": + version: 0.8.1 + resolution: "@metamask/bitcoin-wallet-snap@npm:0.8.1" + checksum: 10/0fff706a98c6f798ae0ae78bf9a8913c0b056b18aff64f994e521c5005ab7e326fafe1d383b2b7c248456948eaa263df3b31a081d620d82ed7c266857c94a955 languageName: node linkType: hard @@ -26098,7 +26098,7 @@ __metadata: "@metamask/assets-controllers": "patch:@metamask/assets-controllers@npm%3A38.3.0#~/.yarn/patches/@metamask-assets-controllers-npm-38.3.0-57b3d695bb.patch" "@metamask/auto-changelog": "npm:^2.1.0" "@metamask/base-controller": "npm:^7.0.0" - "@metamask/bitcoin-wallet-snap": "npm:^0.7.0" + "@metamask/bitcoin-wallet-snap": "npm:^0.8.1" "@metamask/browser-passworder": "npm:^4.3.0" "@metamask/build-utils": "npm:^3.0.0" "@metamask/contract-metadata": "npm:^2.5.0" From e8bc6a5abb2b1ffda5bd645d9ce283508a05c6eb Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 18 Oct 2024 13:21:15 -0400 Subject: [PATCH 05/11] perf: Create custom trace to measure performance of opening the account list (#27907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27907?quickstart=1) Adds custom Sentry trace "`Account List`" that starts when the `AccountPicker` component is clicked, and ends when the `AccountListMenu` component has finished rendering. - Baseline performance: Screenshot 2024-10-16 at 9 45 28 AM - Load testing via revert of https://github.com/MetaMask/metamask-extension/pull/23933: Screenshot 2024-10-16 at 9 45 40 AM ## **Related issues** - Closes https://github.com/MetaMask/MetaMask-planning/issues/3399 ## **Manual testing steps** ## **Screenshots/Recordings** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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. --- shared/lib/trace.ts | 1 + .../account-list-menu/account-list-menu.tsx | 12 +++++++++++- .../multichain/account-picker/account-picker.js | 6 +++++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/shared/lib/trace.ts b/shared/lib/trace.ts index ab1deefd1cc5..1dd50b736222 100644 --- a/shared/lib/trace.ts +++ b/shared/lib/trace.ts @@ -9,6 +9,7 @@ import { log as sentryLogger } from '../../app/scripts/lib/setupSentry'; * The supported trace names. */ export enum TraceName { + AccountList = 'Account List', BackgroundConnect = 'Background Connect', DeveloperTest = 'Developer Test', FirstRender = 'First Render', diff --git a/ui/components/multichain/account-list-menu/account-list-menu.tsx b/ui/components/multichain/account-list-menu/account-list-menu.tsx index 2e5925dbf9cf..19d313aedf54 100644 --- a/ui/components/multichain/account-list-menu/account-list-menu.tsx +++ b/ui/components/multichain/account-list-menu/account-list-menu.tsx @@ -1,4 +1,10 @@ -import React, { useContext, useState, useMemo, useCallback } from 'react'; +import React, { + useContext, + useState, + useMemo, + useCallback, + useEffect, +} from 'react'; import PropTypes from 'prop-types'; import { useHistory } from 'react-router-dom'; import Fuse from 'fuse.js'; @@ -99,6 +105,7 @@ import { AccountConnections, MergedInternalAccount, } from '../../../selectors/selectors.types'; +import { endTrace, TraceName } from '../../../../shared/lib/trace'; import { HiddenAccountList } from './hidden-account-list'; const ACTION_MODES = { @@ -198,6 +205,9 @@ export const AccountListMenu = ({ }: AccountListMenuProps) => { const t = useI18nContext(); const trackEvent = useContext(MetaMetricsContext); + useEffect(() => { + endTrace({ name: TraceName.AccountList }); + }, []); const accounts: InternalAccountWithBalance[] = useSelector( getMetaMaskAccountsOrdered, ); diff --git a/ui/components/multichain/account-picker/account-picker.js b/ui/components/multichain/account-picker/account-picker.js index 18c516cf3cab..20d382923190 100644 --- a/ui/components/multichain/account-picker/account-picker.js +++ b/ui/components/multichain/account-picker/account-picker.js @@ -31,6 +31,7 @@ import { shortenAddress } from '../../../helpers/utils/util'; ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) import { getCustodianIconForAddress } from '../../../selectors/institutional/selectors'; ///: END:ONLY_INCLUDE_IF +import { trace, TraceName } from '../../../../shared/lib/trace'; export const AccountPicker = ({ address, @@ -58,7 +59,10 @@ export const AccountPicker = ({ { + trace({ name: TraceName.AccountList }); + onClick(); + }} backgroundColor={BackgroundColor.transparent} borderRadius={BorderRadius.LG} ellipsis From a9df78b942df10c8eb25572feff22f045bee0a62 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 18 Oct 2024 13:44:34 -0500 Subject: [PATCH 06/11] test: Remove delays from onboarding tests (#27961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** .delay() calls were errantly left in the onboarding tests. Apologies to @seaona for not having addressed these sooner! [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27961?quickstart=1) ## **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 Extension 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. --- test/e2e/tests/onboarding/onboarding.spec.js | 8 ------- .../tests/privacy/basic-functionality.spec.js | 21 +++++++++---------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/test/e2e/tests/onboarding/onboarding.spec.js b/test/e2e/tests/onboarding/onboarding.spec.js index b5e273b7e978..de040f825ee6 100644 --- a/test/e2e/tests/onboarding/onboarding.spec.js +++ b/test/e2e/tests/onboarding/onboarding.spec.js @@ -20,8 +20,6 @@ const { onboardingCompleteWalletCreation, regularDelayMs, unlockWallet, - tinyDelayMs, - largeDelayMs, } = require('../../helpers'); const FixtureBuilder = require('../../fixture-builder'); const { @@ -287,7 +285,6 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement({ text: 'General', }); - await driver.delay(largeDelayMs); await driver.clickElement({ text: 'Add a network' }); await driver.waitForSelector( @@ -311,9 +308,7 @@ describe('MetaMask onboarding @no-mmi', function () { const rpcUrlInputDropDown = await driver.waitForSelector( '[data-testid="test-add-rpc-drop-down"]', ); - await driver.delay(tinyDelayMs); await rpcUrlInputDropDown.click(); - await driver.delay(tinyDelayMs); await driver.clickElement({ text: 'Add RPC URL', tag: 'button', @@ -371,7 +366,6 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement( `[data-rbd-draggable-id="${toHex(chainId)}"]`, ); - await driver.delay(largeDelayMs); // Check localhost 8546 is selected and its balance value is correct await driver.findElement({ css: '[data-testid="network-display"]', @@ -530,8 +524,6 @@ describe('MetaMask onboarding @no-mmi', function () { // pin extension walkthrough screen await driver.clickElement('[data-testid="pin-extension-next"]'); - await driver.delay(regularDelayMs); - for (let i = 0; i < mockedEndpoints.length; i += 1) { const mockedEndpoint = await mockedEndpoints[i]; const isPending = await mockedEndpoint.isPending(); diff --git a/test/e2e/tests/privacy/basic-functionality.spec.js b/test/e2e/tests/privacy/basic-functionality.spec.js index 6ae14ca660be..674ba8772e29 100644 --- a/test/e2e/tests/privacy/basic-functionality.spec.js +++ b/test/e2e/tests/privacy/basic-functionality.spec.js @@ -4,9 +4,6 @@ const { withFixtures, importSRPOnboardingFlow, WALLET_PASSWORD, - tinyDelayMs, - regularDelayMs, - largeDelayMs, defaultGanacheOptions, } = require('../../helpers'); const { METAMASK_STALELIST_URL } = require('../phishing-controller/helpers'); @@ -65,8 +62,6 @@ describe('MetaMask onboarding @no-mmi', function () { }); await driver.clickElement('[data-testid="category-item-General"]'); - await driver.delay(regularDelayMs); - await driver.clickElement( '[data-testid="basic-functionality-toggle"] .toggle-button', ); @@ -74,9 +69,7 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement('[id="basic-configuration-checkbox"]'); await driver.clickElement({ text: 'Turn off', tag: 'button' }); await driver.clickElement('[data-testid="category-back-button"]'); - await driver.delay(regularDelayMs); await driver.clickElement('[data-testid="category-item-Assets"]'); - await driver.delay(regularDelayMs); await driver.clickElement( '[data-testid="currency-rate-check-toggle"] .toggle-button', ); @@ -114,7 +107,6 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement('[data-testid="network-display"]'); await driver.clickElement({ text: 'Ethereum Mainnet', tag: 'p' }); - await driver.delay(tinyDelayMs); // Wait until network is fully switched and refresh tokens before asserting to mitigate flakiness await driver.assertElementNotPresent('.loading-overlay'); @@ -154,13 +146,20 @@ describe('MetaMask onboarding @no-mmi', function () { tag: 'button', }); await driver.clickElement('[data-testid="category-item-General"]'); - await driver.delay(largeDelayMs); + // Wait until the onboarding carousel has stopped moving + // otherwise the click has no effect. + await driver.waitForElementToStopMoving( + '[data-testid="category-back-button"]', + ); await driver.clickElement('[data-testid="category-back-button"]'); - await driver.delay(largeDelayMs); + // Wait until the onboarding carousel has stopped moving + // otherwise the click has no effect. + await driver.waitForElementToStopMoving( + '[data-testid="privacy-settings-back-button"]', + ); await driver.clickElement( '[data-testid="privacy-settings-back-button"]', ); - await driver.delay(largeDelayMs); await driver.clickElement({ text: 'Done', tag: 'button' }); await driver.clickElement('[data-testid="pin-extension-next"]'); await driver.clickElement({ text: 'Done', tag: 'button' }); From 6794a109454fedb734c6d2debf6f7eed2f79024a Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Sat, 19 Oct 2024 02:33:30 -0230 Subject: [PATCH 07/11] chore: Disable account syncing in prod (#27943) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR disables account syncing in prod until we have e2e tests and documentation of proper testing [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27943?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. `yarn build` 2. Account syncing should not occur ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension 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. --------- Co-authored-by: Mark Stacey --- app/scripts/metamask-controller.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index fae6eedc2ab8..0566afc5067a 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -156,6 +156,7 @@ import { NotificationServicesPushController, NotificationServicesController, } from '@metamask/notification-services-controller'; +import { isProduction } from '../../shared/modules/environment'; import { methodsRequiringNetworkSwitch } from '../../shared/constants/methods-tags'; ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) @@ -1561,7 +1562,7 @@ export default class MetamaskController extends EventEmitter { }, }, env: { - isAccountSyncingEnabled: isManifestV3, + isAccountSyncingEnabled: !isProduction() && isManifestV3, }, messenger: this.controllerMessenger.getRestricted({ name: 'UserStorageController', From 4eaeb686acc34bda3d78cbcde357869ef7cadb76 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Sat, 19 Oct 2024 12:09:35 +0200 Subject: [PATCH 08/11] fix(snaps): Remove arrows of custom UI inputs (#27953) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR removes the HTML arrows in custom UI inputs of type `number`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27953?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** ```ts import { Box, Button, Container, Footer, Heading, Input, Text, type SnapComponent, } from '@metamask/snaps-sdk/jsx'; /** * A custom dialog component. * * @returns The custom dialog component. */ export const CustomDialog: SnapComponent = () => (
); ``` ## **Screenshots/Recordings** ### **Before** ![Screenshot from 2024-10-18 13-00-54](https://github.com/user-attachments/assets/eb96f33d-0cd9-4b26-ad42-ccfae207d0f5) ### **After** ![Screenshot from 2024-10-18 12-57-41](https://github.com/user-attachments/assets/08a0cf66-369f-49f1-a51a-7b93cf70e016) ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension 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. --- ui/components/app/snaps/snap-ui-input/index.scss | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ui/components/app/snaps/snap-ui-input/index.scss b/ui/components/app/snaps/snap-ui-input/index.scss index 8dfc06f10fcc..abe966e822a1 100644 --- a/ui/components/app/snaps/snap-ui-input/index.scss +++ b/ui/components/app/snaps/snap-ui-input/index.scss @@ -3,6 +3,17 @@ gap: 8px; } + & .mm-text-field > input { + &::-webkit-outer-spin-button, + &::-webkit-inner-spin-button { + -webkit-appearance: none; + margin: 0; + } + + &[type=number] { + -moz-appearance: textfield; + } + } & .snap-ui-renderer__image { From f416f1e20b9f505d4b906e92358d9cd257ab4765 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Mon, 21 Oct 2024 16:51:53 +0800 Subject: [PATCH 09/11] feat: add migration 131 (#27364) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR introduces a new migration that updates the `selectedAccount` to the first account if available or the default state. ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/26377 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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. --------- Co-authored-by: Charly Chevalier --- app/scripts/migrations/131.test.ts | 244 +++++++++++++++++++++++++++++ app/scripts/migrations/131.ts | 147 +++++++++++++++++ app/scripts/migrations/index.js | 1 + 3 files changed, 392 insertions(+) create mode 100644 app/scripts/migrations/131.test.ts create mode 100644 app/scripts/migrations/131.ts diff --git a/app/scripts/migrations/131.test.ts b/app/scripts/migrations/131.test.ts new file mode 100644 index 000000000000..ab359ff7283c --- /dev/null +++ b/app/scripts/migrations/131.test.ts @@ -0,0 +1,244 @@ +import { AccountsControllerState } from '@metamask/accounts-controller'; +import { cloneDeep } from 'lodash'; +import { createMockInternalAccount } from '../../../test/jest/mocks'; +import { migrate, version } from './131'; + +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + +const oldVersion = 130; + +const mockInternalAccount = createMockInternalAccount(); +const mockAccountsControllerState: AccountsControllerState = { + internalAccounts: { + accounts: { + [mockInternalAccount.id]: mockInternalAccount, + }, + selectedAccount: mockInternalAccount.id, + }, +}; + +describe(`migration #${version}`, () => { + afterEach(() => jest.resetAllMocks()); + + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: mockAccountsControllerState, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('updates selected account if it is not found in the list of accounts', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: { + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + selectedAccount: 'unknown id', + }, + }, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data.AccountsController).toStrictEqual({ + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + selectedAccount: mockInternalAccount.id, + }, + }); + }); + + it('does nothing if the selectedAccount is found in the list of accounts', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: mockAccountsControllerState, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if AccountsController state is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + OtherController: {}, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('sets selected account to default state if there are no accounts', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: { + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + accounts: {}, + }, + }, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data.AccountsController).toStrictEqual({ + ...oldStorage.data.AccountsController, + internalAccounts: { + ...oldStorage.data.AccountsController.internalAccounts, + selectedAccount: '', + }, + }); + }); + + it('does nothing if selectedAccount is unset', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: { + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + selectedAccount: '', + }, + }, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + const invalidState = [ + { + errorMessage: `Migration ${version}: Invalid AccountsController state of type 'string'`, + label: 'AccountsController type', + state: { AccountsController: 'invalid' }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController state, missing internalAccounts`, + label: 'Missing internalAccounts', + state: { AccountsController: {} }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state of type 'string'`, + label: 'Invalid internalAccounts', + state: { AccountsController: { internalAccounts: 'invalid' } }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`, + label: 'Missing selectedAccount', + state: { AccountsController: { internalAccounts: { accounts: {} } } }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type 'object'`, + label: 'Invalid selectedAccount', + state: { + AccountsController: { + internalAccounts: { accounts: {}, selectedAccount: {} }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`, + label: 'Missing accounts', + state: { + AccountsController: { + internalAccounts: { selectedAccount: '' }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type 'string'`, + label: 'Invalid accounts', + state: { + AccountsController: { + internalAccounts: { accounts: 'invalid', selectedAccount: '' }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type 'string'`, + label: 'Invalid accounts entry', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: 'invalid' }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`, + label: 'Missing ID in accounts entry', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: {} }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type 'object'`, + label: 'Invalid ID for accounts entry', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: { id: {} } }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + ]; + + // @ts-expect-error 'each' function missing from type definitions, but it does exist + it.each(invalidState)( + 'captures error when state is invalid due to: $label', + async ({ + errorMessage, + state, + }: { + errorMessage: string; + state: Record; + }) => { + const oldStorage = { + meta: { version: oldVersion }, + data: state, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(errorMessage), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }, + ); +}); diff --git a/app/scripts/migrations/131.ts b/app/scripts/migrations/131.ts new file mode 100644 index 000000000000..9d2ebf970fbd --- /dev/null +++ b/app/scripts/migrations/131.ts @@ -0,0 +1,147 @@ +import { hasProperty } from '@metamask/utils'; +import { cloneDeep, isObject } from 'lodash'; +import log from 'loglevel'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 131; + +/** + * Fix AccountsController state corruption, where the `selectedAccount` state is set to an invalid + * ID. + * + * @param originalVersionedData - Versioned MetaMask extension state, exactly + * what we persist to dist. + * @param originalVersionedData.meta - State metadata. + * @param originalVersionedData.meta.version - The current state version. + * @param originalVersionedData.data - The persisted MetaMask state, keyed by + * controller. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record): void { + if (!hasProperty(state, 'AccountsController')) { + return; + } + + const accountsControllerState = state.AccountsController; + + if (!isObject(accountsControllerState)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController state of type '${typeof accountsControllerState}'`, + ), + ); + return; + } else if (!hasProperty(accountsControllerState, 'internalAccounts')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController state, missing internalAccounts`, + ), + ); + return; + } else if (!isObject(accountsControllerState.internalAccounts)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state of type '${typeof accountsControllerState.internalAccounts}'`, + ), + ); + return; + } else if ( + !hasProperty(accountsControllerState.internalAccounts, 'selectedAccount') + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`, + ), + ); + return; + } else if ( + typeof accountsControllerState.internalAccounts.selectedAccount !== 'string' + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type '${typeof accountsControllerState + .internalAccounts.selectedAccount}'`, + ), + ); + return; + } else if ( + !hasProperty(accountsControllerState.internalAccounts, 'accounts') + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`, + ), + ); + return; + } else if (!isObject(accountsControllerState.internalAccounts.accounts)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type '${typeof accountsControllerState + .internalAccounts.accounts}'`, + ), + ); + return; + } + + if ( + Object.keys(accountsControllerState.internalAccounts.accounts).length === 0 + ) { + // In this case since there aren't any accounts, we set the selected account to the default state to unblock the extension. + accountsControllerState.internalAccounts.selectedAccount = ''; + return; + } else if (accountsControllerState.internalAccounts.selectedAccount === '') { + log.warn(`Migration ${version}: Skipping, no selected account set`); + return; + } + + // Safe to use index 0, we already check for the length before. + const firstAccount = Object.values( + accountsControllerState.internalAccounts.accounts, + )[0]; + if (!isObject(firstAccount)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type '${typeof firstAccount}'`, + ), + ); + return; + } else if (!hasProperty(firstAccount, 'id')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`, + ), + ); + return; + } else if (typeof firstAccount.id !== 'string') { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type '${typeof firstAccount.id}'`, + ), + ); + return; + } + + // If the currently selected account ID is not on the `accounts` object, then + // we fallback to first account of the wallet. + if ( + !hasProperty( + accountsControllerState.internalAccounts.accounts, + accountsControllerState.internalAccounts.selectedAccount, + ) + ) { + accountsControllerState.internalAccounts.selectedAccount = firstAccount.id; + } +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index a72fd34c3c28..d2c63eb2e35c 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -151,6 +151,7 @@ const migrations = [ require('./128'), require('./129'), require('./130'), + require('./131'), ]; export default migrations; From 15962f7fa05e5185401abeb1ad6fe01ae2940c90 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang <7315988+dawnseeker8@users.noreply.github.com> Date: Mon, 21 Oct 2024 17:28:12 +0800 Subject: [PATCH 10/11] feat(metametrics): use specific `account_hardware_type` for OneKey devices (#27296) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Currently extension supports connecting to OneKey via Trezor, but we don't have specific metrics to log this when importing the accounts. Now, the `account_hardware_type` will be set to `OneKey via Trezor` for `AccountAdded` metric when using OneKey devices. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27296?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/accounts-planning/issues/586 ## **Manual testing steps** N/A ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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: legobeat <109787230+legobeat@users.noreply.github.com> Co-authored-by: Charly Chevalier --- .../trezor-offscreen-bridge.ts | 5 +- app/scripts/metamask-controller.js | 28 +++++++- app/scripts/metamask-controller.test.js | 71 ++++++++++++++++++- offscreen/scripts/trezor.ts | 5 +- shared/constants/hardware-wallets.ts | 1 + .../create-account/connect-hardware/index.js | 19 +++-- .../connect-hardware/index.test.tsx | 2 + ui/store/actions.test.js | 44 ++++++++++++ ui/store/actions.ts | 27 +++++++ 9 files changed, 194 insertions(+), 8 deletions(-) diff --git a/app/scripts/lib/offscreen-bridge/trezor-offscreen-bridge.ts b/app/scripts/lib/offscreen-bridge/trezor-offscreen-bridge.ts index 83272015ae2d..0f94627f2836 100644 --- a/app/scripts/lib/offscreen-bridge/trezor-offscreen-bridge.ts +++ b/app/scripts/lib/offscreen-bridge/trezor-offscreen-bridge.ts @@ -30,6 +30,8 @@ import { export class TrezorOffscreenBridge implements TrezorBridge { model: string | undefined; + minorVersion: number | undefined; + init( settings: { manifest: Manifest; @@ -40,7 +42,8 @@ export class TrezorOffscreenBridge implements TrezorBridge { msg.target === OffscreenCommunicationTarget.extension && msg.event === OffscreenCommunicationEvents.trezorDeviceConnect ) { - this.model = msg.payload; + this.model = msg.payload.model; + this.minorVersion = msg.payload.minorVersion; } }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 0566afc5067a..c33485f665b7 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -387,6 +387,9 @@ export const METAMASK_CONTROLLER_EVENTS = { // stream channels const PHISHING_SAFELIST = 'metamask-phishing-safelist'; +// OneKey devices can connect to Metamask using Trezor USB transport. They use a specific device minor version (99) to differentiate between genuine Trezor and OneKey devices. +export const ONE_KEY_VIA_TREZOR_MINOR_VERSION = 99; + export default class MetamaskController extends EventEmitter { /** * @param {object} opts @@ -3400,6 +3403,7 @@ export default class MetamaskController extends EventEmitter { connectHardware: this.connectHardware.bind(this), forgetDevice: this.forgetDevice.bind(this), checkHardwareStatus: this.checkHardwareStatus.bind(this), + getDeviceNameForMetric: this.getDeviceNameForMetric.bind(this), unlockHardwareWalletAccount: this.unlockHardwareWalletAccount.bind(this), attemptLedgerTransportCreation: this.attemptLedgerTransportCreation.bind(this), @@ -4684,6 +4688,26 @@ export default class MetamaskController extends EventEmitter { return keyring.isUnlocked(); } + /** + * Get hardware device name for metric logging. + * + * @param deviceName - HardwareDeviceNames + * @param hdPath - string + * @returns {Promise} + */ + async getDeviceNameForMetric(deviceName, hdPath) { + if (deviceName === HardwareDeviceNames.trezor) { + const keyring = await this.getKeyringForDevice(deviceName, hdPath); + const { minorVersion } = keyring.bridge; + // Specific case for OneKey devices, see `ONE_KEY_VIA_TREZOR_MINOR_VERSION` for further details. + if (minorVersion && minorVersion === ONE_KEY_VIA_TREZOR_MINOR_VERSION) { + return HardwareDeviceNames.oneKeyViaTrezor; + } + } + + return deviceName; + } + /** * Clear * @@ -4756,9 +4780,11 @@ export default class MetamaskController extends EventEmitter { /** * get hardware account label * + * @param name + * @param index + * @param hdPathDescription * @returns string label */ - getAccountLabel(name, index, hdPathDescription) { return `${name[0].toUpperCase()}${name.slice(1)} ${ parseInt(index, 10) + 1 diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 77b062bcfdc7..750f8771568b 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -45,7 +45,9 @@ import { } from './lib/accounts/BalancesController'; import { BalancesTracker as MultichainBalancesTracker } from './lib/accounts/BalancesTracker'; import { deferredPromise } from './lib/util'; -import MetaMaskController from './metamask-controller'; +import MetaMaskController, { + ONE_KEY_VIA_TREZOR_MINOR_VERSION, +} from './metamask-controller'; const { Ganache } = require('../../test/e2e/seeder/ganache'); @@ -894,6 +896,73 @@ describe('MetaMaskController', () => { ); }); + describe('getHardwareDeviceName', () => { + const hdPath = "m/44'/60'/0'/0/0"; + + it('should return the correct device name for Ledger', async () => { + const deviceName = 'ledger'; + + const result = await metamaskController.getDeviceNameForMetric( + deviceName, + hdPath, + ); + expect(result).toBe('ledger'); + }); + + it('should return the correct device name for Lattice', async () => { + const deviceName = 'lattice'; + + const result = await metamaskController.getDeviceNameForMetric( + deviceName, + hdPath, + ); + expect(result).toBe('lattice'); + }); + + it('should return the correct device name for Trezor', async () => { + const deviceName = 'trezor'; + jest + .spyOn(metamaskController, 'getKeyringForDevice') + .mockResolvedValue({ + bridge: { + minorVersion: 1, + model: 'T', + }, + }); + const result = await metamaskController.getDeviceNameForMetric( + deviceName, + hdPath, + ); + expect(result).toBe('trezor'); + }); + + it('should return undefined for unknown device name', async () => { + const deviceName = 'unknown'; + const result = await metamaskController.getDeviceNameForMetric( + deviceName, + hdPath, + ); + expect(result).toBe(deviceName); + }); + + it('should handle special case for OneKeyDevice via Trezor', async () => { + const deviceName = 'trezor'; + jest + .spyOn(metamaskController, 'getKeyringForDevice') + .mockResolvedValue({ + bridge: { + model: 'T', + minorVersion: ONE_KEY_VIA_TREZOR_MINOR_VERSION, + }, + }); + const result = await metamaskController.getDeviceNameForMetric( + deviceName, + hdPath, + ); + expect(result).toBe('OneKey via Trezor'); + }); + }); + describe('forgetDevice', () => { it('should throw if it receives an unknown device name', async () => { const result = metamaskController.forgetDevice( diff --git a/offscreen/scripts/trezor.ts b/offscreen/scripts/trezor.ts index a6c1b5b2788e..22e03048fb93 100644 --- a/offscreen/scripts/trezor.ts +++ b/offscreen/scripts/trezor.ts @@ -40,7 +40,10 @@ export default function init() { chrome.runtime.sendMessage({ target: OffscreenCommunicationTarget.extension, event: OffscreenCommunicationEvents.trezorDeviceConnect, - payload: event.payload.features.model, + payload: { + model: event.payload.features.model, + minorVersion: event.payload.features.minor_version, + }, }); } }); diff --git a/shared/constants/hardware-wallets.ts b/shared/constants/hardware-wallets.ts index 96e50ed7c17e..6fdfbedd9c04 100644 --- a/shared/constants/hardware-wallets.ts +++ b/shared/constants/hardware-wallets.ts @@ -18,6 +18,7 @@ export enum HardwareKeyringNames { export enum HardwareDeviceNames { ledger = 'ledger', trezor = 'trezor', + oneKeyViaTrezor = 'OneKey via Trezor', lattice = 'lattice', qr = 'QR Hardware', } diff --git a/ui/pages/create-account/connect-hardware/index.js b/ui/pages/create-account/connect-hardware/index.js index 2884dacb77b6..85464baccb69 100644 --- a/ui/pages/create-account/connect-hardware/index.js +++ b/ui/pages/create-account/connect-hardware/index.js @@ -28,8 +28,8 @@ import { } from '../../../components/component-library'; import ZENDESK_URLS from '../../../helpers/constants/zendesk-url'; import { TextColor } from '../../../helpers/constants/design-system'; -import SelectHardware from './select-hardware'; import AccountList from './account-list'; +import SelectHardware from './select-hardware'; const U2F_ERROR = 'U2F'; const LEDGER_ERRORS_CODES = { @@ -277,7 +277,7 @@ class ConnectHardwareForm extends Component { }); }; - onUnlockAccounts = (device, path) => { + onUnlockAccounts = async (device, path) => { const { history, mostRecentOverviewPage, unlockHardwareWalletAccounts } = this.props; const { selectedAccounts } = this.state; @@ -290,6 +290,13 @@ class ConnectHardwareForm extends Component { MEW_PATH === path ? this.context.t('hardwareWalletLegacyDescription') : ''; + + // Get preferred device name for metrics. + const metricDeviceName = await this.props.getDeviceNameForMetric( + device, + path, + ); + return unlockHardwareWalletAccounts( selectedAccounts, device, @@ -302,7 +309,7 @@ class ConnectHardwareForm extends Component { event: MetaMetricsEventName.AccountAdded, properties: { account_type: MetaMetricsEventAccountType.Hardware, - account_hardware_type: device, + account_hardware_type: metricDeviceName, }, }); history.push(mostRecentOverviewPage); @@ -313,7 +320,7 @@ class ConnectHardwareForm extends Component { event: MetaMetricsEventName.AccountAddFailed, properties: { account_type: MetaMetricsEventAccountType.Hardware, - account_hardware_type: device, + account_hardware_type: metricDeviceName, error: e.message, }, }); @@ -439,6 +446,7 @@ class ConnectHardwareForm extends Component { ConnectHardwareForm.propTypes = { connectHardware: PropTypes.func, checkHardwareStatus: PropTypes.func, + getDeviceNameForMetric: PropTypes.func, forgetDevice: PropTypes.func, showAlert: PropTypes.func, hideAlert: PropTypes.func, @@ -472,6 +480,9 @@ const mapDispatchToProps = (dispatch) => { connectHardware: (deviceName, page, hdPath, t) => { return dispatch(actions.connectHardware(deviceName, page, hdPath, t)); }, + getDeviceNameForMetric: (deviceName, hdPath) => { + return dispatch(actions.getDeviceNameForMetric(deviceName, hdPath)); + }, checkHardwareStatus: (deviceName, hdPath) => { return dispatch(actions.checkHardwareStatus(deviceName, hdPath)); }, diff --git a/ui/pages/create-account/connect-hardware/index.test.tsx b/ui/pages/create-account/connect-hardware/index.test.tsx index 0b8585fd0b5c..3f7782c1416d 100644 --- a/ui/pages/create-account/connect-hardware/index.test.tsx +++ b/ui/pages/create-account/connect-hardware/index.test.tsx @@ -13,10 +13,12 @@ import ConnectHardwareForm from '.'; const mockConnectHardware = jest.fn(); const mockCheckHardwareStatus = jest.fn().mockResolvedValue(false); +const mockGetgetDeviceNameForMetric = jest.fn().mockResolvedValue('ledger'); jest.mock('../../../store/actions', () => ({ connectHardware: () => mockConnectHardware, checkHardwareStatus: () => mockCheckHardwareStatus, + getDeviceNameForMetric: () => mockGetgetDeviceNameForMetric, })); jest.mock('../../../selectors', () => ({ diff --git a/ui/store/actions.test.js b/ui/store/actions.test.js index 8d72ce63e32d..d86ea20f845c 100644 --- a/ui/store/actions.test.js +++ b/ui/store/actions.test.js @@ -483,6 +483,50 @@ describe('Actions', () => { }); }); + describe('#getDeviceNameForMetric', () => { + const deviceName = 'ledger'; + const hdPath = "m/44'/60'/0'/0/0"; + + afterEach(() => { + sinon.restore(); + }); + + it('calls getDeviceNameForMetric in background', async () => { + const store = mockStore(); + + const mockGetDeviceName = background.getDeviceNameForMetric.callsFake( + (_, __, cb) => cb(), + ); + + setBackgroundConnection(background); + + await store.dispatch(actions.getDeviceNameForMetric(deviceName, hdPath)); + expect(mockGetDeviceName.callCount).toStrictEqual(1); + }); + + it('shows loading indicator and displays error', async () => { + const store = mockStore(); + + background.getDeviceNameForMetric.callsFake((_, __, cb) => + cb(new Error('error')), + ); + + setBackgroundConnection(background); + + const expectedActions = [ + { type: 'SHOW_LOADING_INDICATION', payload: undefined }, + { type: 'DISPLAY_WARNING', payload: 'error' }, + { type: 'HIDE_LOADING_INDICATION' }, + ]; + + await expect( + store.dispatch(actions.getDeviceNameForMetric(deviceName, hdPath)), + ).rejects.toThrow('error'); + + expect(store.getActions()).toStrictEqual(expectedActions); + }); + }); + describe('#forgetDevice', () => { afterEach(() => { sinon.restore(); diff --git a/ui/store/actions.ts b/ui/store/actions.ts index f7d839946635..a051bb15d5fd 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -508,6 +508,33 @@ export function checkHardwareStatus( }; } +export function getDeviceNameForMetric( + deviceName: HardwareDeviceNames, + hdPath: string, +): ThunkAction { + log.debug(`background.getDeviceNameForMetric`, deviceName, hdPath); + return async (dispatch: MetaMaskReduxDispatch) => { + dispatch(showLoadingIndication()); + + let result: string; + try { + result = await submitRequestToBackground( + 'getDeviceNameForMetric', + [deviceName, hdPath], + ); + } catch (error) { + logErrorWithMessage(error); + dispatch(displayWarning(error)); + throw error; + } finally { + dispatch(hideLoadingIndication()); + } + + await forceUpdateMetamaskState(dispatch); + return result; + }; +} + export function forgetDevice( deviceName: HardwareDeviceNames, ): ThunkAction { From 29e1c5b31f1decbc3e6d50eac82d972727b6d81d Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Mon, 21 Oct 2024 12:13:05 +0200 Subject: [PATCH 11/11] fix: Automatically expand first insight (#27872) ## **Description** Automatically expands the first insight on the confirmation page, if any. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27872?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/27869 ## **Screenshots/Recordings** ![image](https://github.com/user-attachments/assets/a23f811f-ab9d-456d-9572-183e953b8802) --- test/e2e/snaps/test-snap-siginsights.spec.js | 44 ++----------------- .../snaps/snaps-section/snap-insight.tsx | 3 ++ .../snaps-section/snaps-section.test.tsx | 5 --- .../snaps/snaps-section/snaps-section.tsx | 3 +- 4 files changed, 8 insertions(+), 47 deletions(-) diff --git a/test/e2e/snaps/test-snap-siginsights.spec.js b/test/e2e/snaps/test-snap-siginsights.spec.js index d40cbc83ae35..b72d6e248ff0 100644 --- a/test/e2e/snaps/test-snap-siginsights.spec.js +++ b/test/e2e/snaps/test-snap-siginsights.spec.js @@ -79,22 +79,15 @@ describe('Test Snap Signature Insights', function () { tag: 'p', }); - // click down arrow - await driver.waitForSelector({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - await driver.clickElement({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - // look for returned signature insights data await driver.waitForSelector({ text: '0x4578616d706c652060706572736f6e616c5f7369676e60206d657373616765', tag: 'p', }); + // Click down arrow + await driver.clickElementSafe('[aria-label="Scroll down"]'); + // click sign button await driver.clickElementAndWaitForWindowToClose( '[data-testid="confirm-footer-button"]', @@ -125,22 +118,11 @@ describe('Test Snap Signature Insights', function () { }); // click down arrow - // await driver.waitForSelector('[aria-label="Scroll down"]'); await driver.clickElementSafe('[aria-label="Scroll down"]'); // required: delay for scroll to render await driver.delay(500); - // click down arrow - await driver.waitForSelector({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - await driver.clickElement({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - // look for returned signature insights data await driver.waitForSelector({ text: '1', @@ -188,16 +170,6 @@ describe('Test Snap Signature Insights', function () { // required: delay for scroll to render await driver.delay(500); - // click signature insights - await driver.waitForSelector({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - await driver.clickElement({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - // look for returned signature insights data await driver.waitForSelector({ text: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC has been identified as a malicious verifying contract.', @@ -245,16 +217,6 @@ describe('Test Snap Signature Insights', function () { // required: delay for scroll to render await driver.delay(500); - // click signature insights - await driver.waitForSelector({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - await driver.clickElement({ - text: 'Signature Insights Example Snap', - tag: 'span', - }); - // look for returned signature insights data await driver.waitForSelector({ text: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC has been identified as a malicious verifying contract.', diff --git a/ui/pages/confirmations/components/confirm/snaps/snaps-section/snap-insight.tsx b/ui/pages/confirmations/components/confirm/snaps/snaps-section/snap-insight.tsx index b428ee8d158d..7102970b4f84 100644 --- a/ui/pages/confirmations/components/confirm/snaps/snaps-section/snap-insight.tsx +++ b/ui/pages/confirmations/components/confirm/snaps/snaps-section/snap-insight.tsx @@ -16,12 +16,14 @@ export type SnapInsightProps = { snapId: string; interfaceId: string; loading: boolean; + isExpanded?: boolean | undefined; }; export const SnapInsight: React.FunctionComponent = ({ snapId, interfaceId, loading, + isExpanded, }) => { const t = useI18nContext(); const { name: snapName } = useSelector((state) => @@ -57,6 +59,7 @@ export const SnapInsight: React.FunctionComponent = ({ { mockStore, ); - fireEvent.click(getByText('Insights from')); - expect(container).toMatchSnapshot(); expect(getByText('Hello world!')).toBeDefined(); }); @@ -79,8 +76,6 @@ describe('SnapsSection', () => { mockStore, ); - fireEvent.click(getByText('Insights from')); - expect(container).toMatchSnapshot(); expect(getByText('Hello world again!')).toBeDefined(); }); diff --git a/ui/pages/confirmations/components/confirm/snaps/snaps-section/snaps-section.tsx b/ui/pages/confirmations/components/confirm/snaps/snaps-section/snaps-section.tsx index 896411ad4dec..b838255237e6 100644 --- a/ui/pages/confirmations/components/confirm/snaps/snaps-section/snaps-section.tsx +++ b/ui/pages/confirmations/components/confirm/snaps/snaps-section/snaps-section.tsx @@ -23,12 +23,13 @@ export const SnapsSection = () => { gap={4} marginBottom={4} > - {data.map(({ snapId, interfaceId, loading }) => ( + {data.map(({ snapId, interfaceId, loading }, index) => ( ))}