From 5578510f32fedbc70ea30e9c7e136a8461c1f3bb Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Wed, 29 Jan 2025 12:29:02 -0800 Subject: [PATCH] address feedback --- .../TokenList/PortfolioBalance/index.tsx | 26 +++++----- .../useMultichainBalances.test.ts | 50 +++++++++---------- .../useMultichainBalances.ts | 2 +- .../useMultichainBalances.types.ts | 2 +- app/core/Engine/Engine.ts | 32 ++++++++---- app/core/Engine/constants.ts | 5 +- .../AccountsController/utils.test.ts | 3 +- .../RatesController/subscriptions.ts | 20 +++++--- .../controllers/SnapController/constants.ts | 38 ++++++++++++++ app/core/Snaps/SnapsMethodMiddleware.ts | 16 ++++-- app/core/Snaps/utils.ts | 6 ++- 11 files changed, 135 insertions(+), 65 deletions(-) create mode 100644 app/core/Engine/controllers/SnapController/constants.ts diff --git a/app/components/UI/Tokens/TokenList/PortfolioBalance/index.tsx b/app/components/UI/Tokens/TokenList/PortfolioBalance/index.tsx index 8378465dc43..29cd1c7a5eb 100644 --- a/app/components/UI/Tokens/TokenList/PortfolioBalance/index.tsx +++ b/app/components/UI/Tokens/TokenList/PortfolioBalance/index.tsx @@ -39,13 +39,13 @@ export const PortfolioBalance = React.memo(() => { const styles = createStyles(colors); const browserTabs = useSelector((state: RootState) => state.browser.tabs); const privacyMode = useSelector(selectPrivacyMode); - const ismultchainBalancesCollectionForMarketingEnabled = useSelector( + const isMultichainBalancesCollectionForMarketingEnabled = useSelector( (state: RootState) => state.security.dataCollectionForMarketing, ); const navigation = useNavigation(); const { trackEvent, isEnabled, createEventBuilder } = useMetrics(); - const { multchainBalances } = useMultichainBalances(); + const { multichainBalances } = useMultichainBalances(); const onOpenPortfolio = useCallback(() => { const existingPortfolioTab = browserTabs.find(({ url }: BrowserTab) => @@ -69,7 +69,7 @@ export const PortfolioBalance = React.memo(() => { ); portfolioUrl.searchParams.append( 'marketingEnabled', - String(!!ismultchainBalancesCollectionForMarketingEnabled), + String(!!isMultichainBalancesCollectionForMarketingEnabled), ); newTabUrl = portfolioUrl.href; @@ -95,22 +95,22 @@ export const PortfolioBalance = React.memo(() => { trackEvent, createEventBuilder, isEnabled, - ismultchainBalancesCollectionForMarketingEnabled, + isMultichainBalancesCollectionForMarketingEnabled, browserTabs, ]); const renderAggregatedPercentage = () => { - if (!multchainBalances.shouldShowAggregatedPercentage) { + if (!multichainBalances.shouldShowAggregatedPercentage) { return null; } - if (multchainBalances.isPortfolioVieEnabled) { + if (multichainBalances.isPortfolioVieEnabled) { return ( ); @@ -119,10 +119,10 @@ export const PortfolioBalance = React.memo(() => { return ( ); }; @@ -145,7 +145,7 @@ export const PortfolioBalance = React.memo(() => { testID={WalletViewSelectorsIDs.TOTAL_BALANCE_TEXT} variant={TextVariant.DisplayMD} > - {multchainBalances.displayBalance} + {multichainBalances.displayBalance} toggleIsBalanceAndAssetsHidden(!privacyMode)} diff --git a/app/components/hooks/useMultichainBalances/useMultichainBalances.test.ts b/app/components/hooks/useMultichainBalances/useMultichainBalances.test.ts index c84027ec863..1ea9a35122c 100644 --- a/app/components/hooks/useMultichainBalances/useMultichainBalances.test.ts +++ b/app/components/hooks/useMultichainBalances/useMultichainBalances.test.ts @@ -75,48 +75,48 @@ describe('useMultichainBalances', () => { }); it('returns default values when no balances are available', () => { - (Engine.getTotalFiatAccountBalance as jest.Mock).mockReturnValue({ + const aggregatedBalance = { ethFiat: 0, tokenFiat: 0, tokenFiat1dAgo: 0, ethFiat1dAgo: 0, - }); + }; + + (Engine.getTotalFiatAccountBalance as jest.Mock).mockReturnValue( + aggregatedBalance, + ); const { result } = renderHook(() => useMultichainBalances()); - expect(result.current.multchainBalances).toEqual({ + expect(result.current.multichainBalances).toEqual({ displayBalance: '0 USD', tokenFiatBalancesCrossChains: [], totalFiatBalance: 0, totalTokenFiat: 0, shouldShowAggregatedPercentage: true, isPortfolioVieEnabled: false, - aggregatedBalance: { - ethFiat: 0, - tokenFiat: 0, - tokenFiat1dAgo: 0, - ethFiat1dAgo: 0, - }, + aggregatedBalance, }); }); it('calculates display balance correctly with ETH and token balances', () => { - (Engine.getTotalFiatAccountBalance as jest.Mock).mockReturnValue({ + const aggregatedBalance = { ethFiat: 100, tokenFiat: 50, tokenFiat1dAgo: 45, ethFiat1dAgo: 95, - }); + }; + + (Engine.getTotalFiatAccountBalance as jest.Mock).mockReturnValue( + aggregatedBalance, + ); const { result } = renderHook(() => useMultichainBalances()); - expect(result.current.multchainBalances.displayBalance).toBe('150 USD'); - expect(result.current.multchainBalances.aggregatedBalance).toEqual({ - ethFiat: 100, - tokenFiat: 50, - tokenFiat1dAgo: 45, - ethFiat1dAgo: 95, - }); + expect(result.current.multichainBalances.displayBalance).toBe('150 USD'); + expect(result.current.multichainBalances.aggregatedBalance).toEqual( + aggregatedBalance, + ); }); it('handles portfolio view mode correctly', () => { @@ -142,13 +142,13 @@ describe('useMultichainBalances', () => { { chainId: '0x1', nativeFiatValue: 500, - tokenFiatBalances: [500], + tokenFiatBalances: [mockTokenFiatBalance], tokensWithBalances: mockTokensWithBalances, }, { chainId: '0x89', nativeFiatValue: 500, - tokenFiatBalances: [500], + tokenFiatBalances: [mockTokenFiatBalance], tokensWithBalances: mockTokensWithBalances, }, ], @@ -161,14 +161,14 @@ describe('useMultichainBalances', () => { const { result } = renderHook(() => useMultichainBalances()); - expect(result.current.multchainBalances.isPortfolioVieEnabled).toBe(true); - expect(result.current.multchainBalances.totalFiatBalance).toBe( + expect(result.current.multichainBalances.isPortfolioVieEnabled).toBe(true); + expect(result.current.multichainBalances.totalFiatBalance).toBe( mockTotalFiatBalance, ); - expect(result.current.multchainBalances.totalTokenFiat).toBe( + expect(result.current.multichainBalances.totalTokenFiat).toBe( mockTokenFiatBalance, ); - expect(result.current.multchainBalances.displayBalance).toBe('1000 USD'); + expect(result.current.multichainBalances.displayBalance).toBe('1000 USD'); }); it('does not show aggregated percentage on test networks', () => { @@ -177,7 +177,7 @@ describe('useMultichainBalances', () => { const { result } = renderHook(() => useMultichainBalances()); expect( - result.current.multchainBalances.shouldShowAggregatedPercentage, + result.current.multichainBalances.shouldShowAggregatedPercentage, ).toBe(false); }); }); diff --git a/app/components/hooks/useMultichainBalances/useMultichainBalances.ts b/app/components/hooks/useMultichainBalances/useMultichainBalances.ts index bdddcd01dd9..32226c370e1 100644 --- a/app/components/hooks/useMultichainBalances/useMultichainBalances.ts +++ b/app/components/hooks/useMultichainBalances/useMultichainBalances.ts @@ -159,7 +159,7 @@ const useMultichainBalances = (): UseMultichainBalancesHook => { }; return { - multchainBalances: { + multichainBalances: { displayBalance: getDisplayBalance(), tokenFiatBalancesCrossChains: totalFiatBalancesCrossChain[selectedInternalAccount?.address as string] diff --git a/app/components/hooks/useMultichainBalances/useMultichainBalances.types.ts b/app/components/hooks/useMultichainBalances/useMultichainBalances.types.ts index 21223db6d62..76c18315901 100644 --- a/app/components/hooks/useMultichainBalances/useMultichainBalances.types.ts +++ b/app/components/hooks/useMultichainBalances/useMultichainBalances.types.ts @@ -12,5 +12,5 @@ export interface MultichainBalancesData { } export interface UseMultichainBalancesHook { - multchainBalances: MultichainBalancesData; + multichainBalances: MultichainBalancesData; } diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 6f05578243b..8ff816f39f3 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -223,6 +223,14 @@ import { getGlobalNetworkClientId, } from '../../util/networks/global-network'; import { logEngineCreation } from './utils/logger'; +import { + SnapControllerClearSnapStateAction, + SnapControllerGetSnapAction, + SnapControllerGetSnapStateAction, + SnapControllerHandleRequestAction, + SnapControllerStateChangeAction, + SnapControllerUpdateSnapStateAction, +} from './controllers/SnapController/constants'; const NON_EMPTY = 'NON_EMPTY'; @@ -370,7 +378,7 @@ export class Engine { this.controllerMessenger.getRestricted({ name: 'AccountsController', allowedEvents: [ - 'SnapController:stateChange', + SnapControllerStateChangeAction, 'KeyringController:accountRemoved', 'KeyringController:stateChange', ], @@ -395,7 +403,7 @@ export class Engine { ], allowedActions: [ AccountsControllerListMultichainAccountsAction, - 'SnapController:handleRequest', + SnapControllerHandleRequestAction, ], }); @@ -417,7 +425,10 @@ export class Engine { }); // Set up currency rate sync - setupCurrencyRateSync(this.controllerMessenger, multichainRatesController); + setupCurrencyRateSync( + multichainRatesControllerMessenger, + multichainRatesController, + ); ///: END:ONLY_INCLUDE_IF const nftController = new NftController({ @@ -677,13 +688,13 @@ export class Engine { // @ts-ignore clearSnapState: this.controllerMessenger.call.bind( this.controllerMessenger, - 'SnapController:clearSnapState', + SnapControllerClearSnapStateAction, ), getMnemonic: getPrimaryKeyringMnemonic.bind(this), getUnlockPromise: getAppState.bind(this), getSnap: this.controllerMessenger.call.bind( this.controllerMessenger, - 'SnapController:get', + SnapControllerGetSnapAction, ), handleSnapRpcRequest: async (args: HandleSnapRequestArgs) => await handleSnapRequest(this.controllerMessenger, args), @@ -691,13 +702,13 @@ export class Engine { // @ts-ignore getSnapState: this.controllerMessenger.call.bind( this.controllerMessenger, - 'SnapController:getSnapState', + SnapControllerGetSnapStateAction, ), // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore updateSnapState: this.controllerMessenger.call.bind( this.controllerMessenger, - 'SnapController:updateSnapState', + SnapControllerUpdateSnapStateAction, ), maybeUpdatePhishingList: this.controllerMessenger.call.bind( this.controllerMessenger, @@ -1029,7 +1040,7 @@ export class Engine { 'KeyringController:getState', 'KeyringController:getAccounts', - 'SnapController:handleRequest', + SnapControllerHandleRequestAction, 'UserStorageController:enableProfileSyncing', ], allowedEvents: ['KeyringController:unlock', 'KeyringController:lock'], @@ -1076,7 +1087,7 @@ export class Engine { messenger: this.controllerMessenger.getRestricted({ name: 'UserStorageController', allowedActions: [ - 'SnapController:handleRequest', + SnapControllerHandleRequestAction, 'KeyringController:getState', 'KeyringController:addNewAccount', 'AuthenticationController:getBearerToken', @@ -2229,7 +2240,8 @@ export default { keyringState: KeyringControllerState | null = null, metaMetricsId?: string, ) { - instance = Engine.instance || new Engine(state, keyringState, metaMetricsId); + instance = + Engine.instance || new Engine(state, keyringState, metaMetricsId); Object.freeze(instance); return instance; }, diff --git a/app/core/Engine/constants.ts b/app/core/Engine/constants.ts index c30a196f85b..b53cd1f060b 100644 --- a/app/core/Engine/constants.ts +++ b/app/core/Engine/constants.ts @@ -1,3 +1,6 @@ +///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps) +import { SnapControllerStateChangeAction } from './controllers/SnapController/constants'; +///: END:ONLY_INCLUDE_IF /** * Messageable modules that are part of the Engine's context, but are not defined with state. * TODO: Replace with type guard once consistent inheritance for non-controllers is implemented. See: https://github.com/MetaMask/decisions/pull/41 @@ -34,7 +37,7 @@ export const BACKGROUND_STATE_CHANGE_EVENT_NAMES = [ 'TokensController:stateChange', 'TransactionController:stateChange', ///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps) - 'SnapController:stateChange', + SnapControllerStateChangeAction, 'SnapsRegistry:stateChange', 'SubjectMetadataController:stateChange', 'AuthenticationController:stateChange', diff --git a/app/core/Engine/controllers/AccountsController/utils.test.ts b/app/core/Engine/controllers/AccountsController/utils.test.ts index 7165e1239a3..29031f5cbd2 100644 --- a/app/core/Engine/controllers/AccountsController/utils.test.ts +++ b/app/core/Engine/controllers/AccountsController/utils.test.ts @@ -11,6 +11,7 @@ import { withScope } from '@sentry/react-native'; import { AGREED, METRICS_OPT_IN } from '../../../../constants/storage'; import StorageWrapper from '../../../../store/storage-wrapper'; import { logAccountsControllerCreation } from './logger'; +import { SnapControllerStateChangeAction } from '../SnapController/constants'; jest.mock('@sentry/react-native', () => ({ withScope: jest.fn(), @@ -33,7 +34,7 @@ describe('accountControllersUtils', () => { accountsControllerMessenger = globalMessenger.getRestricted({ name: 'AccountsController', allowedEvents: [ - 'SnapController:stateChange', + SnapControllerStateChangeAction, 'KeyringController:accountRemoved', 'KeyringController:stateChange', ], diff --git a/app/core/Engine/controllers/RatesController/subscriptions.ts b/app/core/Engine/controllers/RatesController/subscriptions.ts index 7bc476c1837..cd9494818cf 100644 --- a/app/core/Engine/controllers/RatesController/subscriptions.ts +++ b/app/core/Engine/controllers/RatesController/subscriptions.ts @@ -2,8 +2,19 @@ import { RatesController, CurrencyRateState, CurrencyRateController, + CurrencyRateControllerEvents, } from '@metamask/assets-controllers'; import Logger from '../../../../util/Logger'; +import { RestrictedControllerMessenger } from '@metamask/base-controller'; + +// FIXME: This messenger type is not exported on `@metamask/assets-controllers`, so declare it here for now: +type CurrencyRateControllerMessenger = RestrictedControllerMessenger< + CurrencyRateController['name'], + never, + CurrencyRateControllerEvents, + never, + CurrencyRateControllerEvents['type'] +>; /** * Sets up subscription to sync CurrencyRateController changes with RatesController @@ -11,16 +22,11 @@ import Logger from '../../../../util/Logger'; * @param ratesController - The RatesController instance to sync with */ export const setupCurrencyRateSync = ( - controllerMessenger: { - subscribe: ( - eventName: string, - handler: (state: CurrencyRateState) => void, - ) => void; - }, + controllerMessenger: CurrencyRateControllerMessenger, ratesController: RatesController, ): void => { controllerMessenger.subscribe( - `${CurrencyRateController.name}:stateChange`, + 'CurrencyRateController:stateChange', (state: CurrencyRateState) => { if (state.currentCurrency) { ratesController diff --git a/app/core/Engine/controllers/SnapController/constants.ts b/app/core/Engine/controllers/SnapController/constants.ts new file mode 100644 index 00000000000..b6e78c237d3 --- /dev/null +++ b/app/core/Engine/controllers/SnapController/constants.ts @@ -0,0 +1,38 @@ +import { + HandleSnapRequest as SnapControllerHandleRequestActionType, + SnapStateChange as SnapControllerStateChangeActionType, + ClearSnapState as SnapControllerClearSnapStateActionType, + GetSnap as SnapControllerGetSnapActionType, + GetSnapState as SnapControllerGetSnapStateActionType, + UpdateSnapState as SnapControllerUpdateSnapStateActionType, + GetPermittedSnaps as SnapControllerGetPermittedSnapsActionType, + InstallSnaps as SnapControllerInstallSnapsActionType, + GetSnapFile as SnapControllerGetSnapFileActionType, +} from '@metamask/snaps-controllers'; + +export const SnapControllerHandleRequestAction: SnapControllerHandleRequestActionType['type'] = + 'SnapController:handleRequest'; + +export const SnapControllerStateChangeAction: SnapControllerStateChangeActionType['type'] = + 'SnapController:stateChange'; + +export const SnapControllerClearSnapStateAction: SnapControllerClearSnapStateActionType['type'] = + 'SnapController:clearSnapState'; + +export const SnapControllerGetSnapAction: SnapControllerGetSnapActionType['type'] = + 'SnapController:get'; + +export const SnapControllerGetSnapStateAction: SnapControllerGetSnapStateActionType['type'] = + 'SnapController:getSnapState'; + +export const SnapControllerUpdateSnapStateAction: SnapControllerUpdateSnapStateActionType['type'] = + 'SnapController:updateSnapState'; + +export const SnapControllerGetPermittedSnapsAction: SnapControllerGetPermittedSnapsActionType['type'] = + 'SnapController:getPermitted'; + +export const SnapControllerInstallSnapsAction: SnapControllerInstallSnapsActionType['type'] = + 'SnapController:install'; + +export const SnapControllerGetSnapFileAction: SnapControllerGetSnapFileActionType['type'] = + 'SnapController:getFile'; diff --git a/app/core/Snaps/SnapsMethodMiddleware.ts b/app/core/Snaps/SnapsMethodMiddleware.ts index c2ad0d4f3ae..a1929e9d780 100644 --- a/app/core/Snaps/SnapsMethodMiddleware.ts +++ b/app/core/Snaps/SnapsMethodMiddleware.ts @@ -10,12 +10,18 @@ import { keyringSnapPermissionsBuilder } from '../SnapKeyring/keyringSnapsPermis import { SnapId } from '@metamask/snaps-sdk'; import { EngineContext } from '../Engine'; import { handleSnapRequest } from './utils'; +import { + SnapControllerGetPermittedSnapsAction, + SnapControllerGetSnapAction, + SnapControllerGetSnapFileAction, + SnapControllerInstallSnapsAction, +} from '../Engine/controllers/SnapController/constants'; export function getSnapIdFromRequest( request: Record, ): SnapId | null { const { snapId } = request; - return typeof snapId === 'string' ? snapId as SnapId : null; + return typeof snapId === 'string' ? (snapId as SnapId) : null; } // Snaps middleware /* @@ -33,7 +39,7 @@ const snapMethodMiddlewareBuilder = ( getUnlockPromise: () => Promise.resolve(), getSnaps: controllerMessenger.call.bind( controllerMessenger, - 'SnapController:getPermitted', + SnapControllerGetPermittedSnapsAction, origin, ), requestPermissions: async (requestedPermissions: RequestedPermissions) => @@ -52,12 +58,12 @@ const snapMethodMiddlewareBuilder = ( getAllowedKeyringMethods: keyringSnapPermissionsBuilder(origin), getSnapFile: controllerMessenger.call.bind( controllerMessenger, - 'SnapController:getFile', + SnapControllerGetSnapFileAction, origin, ), installSnaps: controllerMessenger.call.bind( controllerMessenger, - 'SnapController:install', + SnapControllerInstallSnapsAction, origin, ), invokeSnap: engineContext.PermissionController.executeRestrictedMethod.bind( @@ -67,7 +73,7 @@ const snapMethodMiddlewareBuilder = ( ), getSnap: controllerMessenger.call.bind( controllerMessenger, - 'SnapController:get', + SnapControllerGetSnapAction, ), handleSnapRpcRequest: async (request: Omit) => { const snapId = getSnapIdFromRequest(request); diff --git a/app/core/Snaps/utils.ts b/app/core/Snaps/utils.ts index ac2157dc785..b06fc9adc0f 100644 --- a/app/core/Snaps/utils.ts +++ b/app/core/Snaps/utils.ts @@ -1,4 +1,5 @@ import { ControllerMessenger } from '../Engine'; +import { SnapControllerHandleRequestAction } from '../Engine/controllers/SnapController/constants'; import { HandleSnapRequestArgs } from './types'; /** @@ -15,5 +16,8 @@ export async function handleSnapRequest( controllerMessenger: ControllerMessenger, args: HandleSnapRequestArgs, ) { - return await controllerMessenger.call('SnapController:handleRequest', args); + return await controllerMessenger.call( + SnapControllerHandleRequestAction, + args, + ); }