From eeb397936d7e6aa9245116dac8586d0c5012845c Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 09:47:00 +0100 Subject: [PATCH 1/8] feat!: make MoneyAccountUpgradeController own its bootstrap The controller now subscribes to RemoteFeatureFlagController and KeyringController state, parses the moneyAccountVaultConfig flag, and runs its own serialized bootstrap, replacing the client-side services that previously drove init() externally. Client-specific concerns (version-gated enable flag, eligibility checks, adding the vault chain, error reporting) are supplied as constructor hooks. - BREAKING: init() is now the no-argument lifecycle entry point (subscribe and sync); the config-arming routine is internal and driven by the moneyAccountVaultConfig remote feature flag - BREAKING: the constructor requires a hooks option with an isEnabled hook - upgradeAccount() now waits for an in-flight bootstrap instead of throwing, and refuses to run against a stale config after the feature is disabled - Add vault-config parsing and comparison helpers to @metamask/money-account-utils, shared by the controller and both clients Co-Authored-By: Claude Fable 5 --- .../package.json | 2 + ...ntUpgradeController-method-action-types.ts | 4 + .../src/MoneyAccountUpgradeController.test.ts | 781 ++++++++++++++---- .../src/MoneyAccountUpgradeController.ts | 319 ++++++- .../src/errors.ts | 15 + .../src/index.ts | 2 + .../tsconfig.build.json | 6 + .../tsconfig.json | 6 + packages/money-account-utils/src/index.ts | 7 + .../src/vault-config.test.ts | 155 ++++ .../money-account-utils/src/vault-config.ts | 121 +++ yarn.lock | 25 +- 12 files changed, 1261 insertions(+), 182 deletions(-) create mode 100644 packages/money-account-utils/src/vault-config.test.ts create mode 100644 packages/money-account-utils/src/vault-config.ts diff --git a/packages/money-account-upgrade-controller/package.json b/packages/money-account-upgrade-controller/package.json index a1cb6ce7539..8ecb848d0b7 100644 --- a/packages/money-account-upgrade-controller/package.json +++ b/packages/money-account-upgrade-controller/package.json @@ -63,7 +63,9 @@ "@metamask/delegation-deployments": "^1.4.0", "@metamask/keyring-controller": "^27.1.1", "@metamask/messenger": "^2.0.0", + "@metamask/money-account-utils": "^1.1.0", "@metamask/network-controller": "^36.0.0", + "@metamask/remote-feature-flag-controller": "^6.1.0", "@metamask/utils": "^11.12.0" }, "devDependencies": { diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts index 7aa0061afdc..53a75996643 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts @@ -19,6 +19,10 @@ import type { MoneyAccountUpgradeController } from './MoneyAccountUpgradeControl * active config no longer matches the recorded fingerprint, the sequence * re-runs. * + * A call that arrives while the bootstrap is still in flight waits for it + * rather than failing; it only throws when no bootstrap has armed a config + * (feature disabled, wallet locked, or the last bootstrap failed). + * * @param address - The Money Account address to upgrade. */ export type MoneyAccountUpgradeControllerUpgradeAccountAction = { diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 50f9932e884..91d268d50b6 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -1,10 +1,13 @@ import { DELEGATOR_CONTRACTS } from '@metamask/delegation-deployments'; +import type { KeyringControllerState } from '@metamask/keyring-controller'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { MockAnyNamespace, MessengerActions, MessengerEvents, } from '@metamask/messenger'; +import type { MoneyAccountVaultConfig } from '@metamask/money-account-utils'; +import type { RemoteFeatureFlagControllerState } from '@metamask/remote-feature-flag-controller'; import { hexToNumber } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; @@ -14,6 +17,7 @@ import type { MoneyAccountUpgradeStepError, } from './index.js'; import { + MissingMoneyAccountVaultConfigError, MoneyAccountUpgradeController, getDefaultMoneyAccountUpgradeControllerState, isMoneyAccountUpgradeStepError, @@ -27,6 +31,22 @@ const MOCK_ACCOUNT_ADDRESS = const MOCK_BORING_VAULT_ADDRESS = '0xA20f97813014129E7609171d2D3AA3da5206259e' as Hex; +const VAULT_CONFIG: MoneyAccountVaultConfig = { + chainId: MOCK_CHAIN_ID, + boringVault: MOCK_BORING_VAULT_ADDRESS, + tellerAddress: '0x2D49EA58A4C70b62c8B56DE971310d9e999c8117', + accountantAddress: '0x7382c5b8B51B8C4f127B3123C1039581BAA5A06B', + lensAddress: '0xA816ECd922de94c6879AD23B9A884dB257F20947', + underlyingToken: '0xacA92E438df0B2401fF60dA7E4337B687a2435DA', +}; + +// The same vault, but published under a fresher flag payload with a different +// vmUSD token address — must be treated as a config change. +const CHANGED_VAULT_CONFIG: MoneyAccountVaultConfig = { + ...VAULT_CONFIG, + underlyingToken: '0x1111111111111111111111111111111111111111', +}; + // CHOMP-API-derived values. const MOCK_DELEGATE_ADDRESS = '0x1111111111111111111111111111111111111111' as Hex; @@ -37,8 +57,8 @@ const MOCK_VEDA_VAULT_ADAPTER_ADDRESS = // Delegation Framework deployment for mainnet @ 1.3.0 — the controller resolves // these from `@metamask/delegation-deployments` rather than accepting them via -// `init()`. We re-read from the same source here so the test does not drift if -// the deployment registry is bumped. +// the vault config. We re-read from the same source here so the test does not +// drift if the deployment registry is bumped. const MAINNET_CONTRACTS = DELEGATOR_CONTRACTS['1.3.0'][hexToNumber(MOCK_CHAIN_ID)]; @@ -69,6 +89,13 @@ type AllEvents = MessengerEvents; type RootMessenger = Messenger; +/** + * Flush the microtask queue so scheduled bootstraps settle. + */ +const flushPromises = async (): Promise => { + await new Promise((resolve) => setImmediate(resolve)); +}; + type Mocks = { getServiceDetails: jest.Mock; signPersonalMessage: jest.Mock; @@ -85,18 +112,58 @@ type Mocks = { verifyDelegation: jest.Mock; getIntentsByAddress: jest.Mock; createIntents: jest.Mock; + isEnabled: jest.Mock; + isEligible: jest.Mock; + ensureChainConfigured: jest.Mock; + onBootstrapError: jest.Mock; +}; + +/** + * The mutable gate state the messenger and hook mocks read on every call, so + * tests can flip a gate and re-trigger a sync. + */ +type GateConfig = { + isEnabled: boolean; + isUnlocked: boolean; + hasHdKeyring: boolean; + isEligible: boolean; + vaultConfig: unknown; }; function setup({ state, + isEnabled = true, + isUnlocked = true, + hasHdKeyring = true, + isEligible = true, + vaultConfig = VAULT_CONFIG, + withOptionalHooks = true, }: { state?: Partial; + isEnabled?: boolean; + isUnlocked?: boolean; + hasHdKeyring?: boolean; + isEligible?: boolean; + vaultConfig?: unknown; + withOptionalHooks?: boolean; } = {}): { controller: MoneyAccountUpgradeController; rootMessenger: RootMessenger; messenger: MoneyAccountUpgradeControllerMessenger; mocks: Mocks; + config: GateConfig; + bootstrap: () => Promise; + triggerFlagChange: () => Promise; + triggerKeyringChange: () => Promise; } { + const config: GateConfig = { + isEnabled, + isUnlocked, + hasHdKeyring, + isEligible, + vaultConfig, + }; + // 65-byte signature — r (32 bytes) + s (32 bytes) + v = 0x1c (28). const signature = `0x${'1'.repeat(64)}${'2'.repeat(64)}1c`; @@ -146,12 +213,37 @@ function setup({ verifyDelegation: jest.fn().mockResolvedValue({ valid: true }), getIntentsByAddress: jest.fn().mockResolvedValue([]), createIntents: jest.fn().mockResolvedValue([]), + isEnabled: jest.fn().mockImplementation(() => config.isEnabled), + isEligible: jest.fn().mockImplementation(async () => config.isEligible), + ensureChainConfigured: jest.fn().mockResolvedValue(undefined), + onBootstrapError: jest.fn(), }; const rootMessenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE, }); + rootMessenger.registerActionHandler( + 'RemoteFeatureFlagController:getState', + (): RemoteFeatureFlagControllerState => + ({ + remoteFeatureFlags: { + moneyAccountVaultConfig: config.vaultConfig, + }, + cacheTimestamp: 0, + }) as RemoteFeatureFlagControllerState, + ); + rootMessenger.registerActionHandler( + 'KeyringController:getState', + (): KeyringControllerState => + ({ + isUnlocked: config.isUnlocked, + keyrings: + config.isUnlocked && config.hasHdKeyring + ? [{ type: 'HD Key Tree', accounts: [], metadata: { id: 'hd' } }] + : [], + }) as unknown as KeyringControllerState, + ); rootMessenger.registerActionHandler( 'ChompApiService:getServiceDetails', mocks.getServiceDetails, @@ -217,6 +309,7 @@ function setup({ rootMessenger.delegate({ actions: [ 'ChompApiService:getServiceDetails', + 'KeyringController:getState', 'KeyringController:signPersonalMessage', 'ChompApiService:associateAddress', 'ChompApiService:getAssociatedAddresses', @@ -230,17 +323,61 @@ function setup({ 'ChompApiService:verifyDelegation', 'ChompApiService:getIntentsByAddress', 'ChompApiService:createIntents', + 'RemoteFeatureFlagController:getState', + ], + events: [ + 'KeyringController:stateChanged', + 'RemoteFeatureFlagController:stateChanged', ], - events: [], messenger, }); const controller = new MoneyAccountUpgradeController({ messenger, state, + hooks: withOptionalHooks + ? { + isEnabled: mocks.isEnabled, + isEligible: mocks.isEligible, + ensureChainConfigured: mocks.ensureChainConfigured, + onBootstrapError: mocks.onBootstrapError, + } + : { isEnabled: mocks.isEnabled }, }); - return { controller, rootMessenger, messenger, mocks }; + const bootstrap = async (): Promise => { + controller.init(); + await flushPromises(); + }; + + const triggerFlagChange = async (): Promise => { + rootMessenger.publish( + 'RemoteFeatureFlagController:stateChanged', + {} as RemoteFeatureFlagControllerState, + [], + ); + await flushPromises(); + }; + + const triggerKeyringChange = async (): Promise => { + rootMessenger.publish( + 'KeyringController:stateChanged', + {} as KeyringControllerState, + [], + ); + await flushPromises(); + }; + + return { + controller, + rootMessenger, + messenger, + mocks, + config, + bootstrap, + triggerFlagChange, + triggerKeyringChange, + }; } /** @@ -258,10 +395,11 @@ function clearMockCalls(mocks: Mocks): void { describe('MoneyAccountUpgradeController', () => { describe('constructor', () => { - it('does not make async init calls when constructed', () => { + it('makes no messenger calls before init()', () => { const { mocks } = setup(); expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + expect(mocks.isEnabled).not.toHaveBeenCalled(); }); it('starts with the default empty state', () => { @@ -286,75 +424,390 @@ describe('MoneyAccountUpgradeController', () => { }); }); - describe('init', () => { - it('fetches service details and builds config', async () => { - const { controller, mocks } = setup(); + describe('bootstrap gating', () => { + it('bootstraps at init when the gates are open', async () => { + const { mocks, bootstrap } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + await bootstrap(); expect(mocks.getServiceDetails).toHaveBeenCalledWith([MOCK_CHAIN_ID]); }); - it('throws when the chain has no Delegation Framework deployment', async () => { - const { controller, mocks } = setup(); + it('configures the chain before fetching service details', async () => { + const order: string[] = []; + const { mocks, bootstrap } = setup(); + mocks.ensureChainConfigured.mockImplementation(async () => { + order.push('ensureChainConfigured'); + }); + mocks.getServiceDetails.mockImplementation(async () => { + order.push('getServiceDetails'); + return MOCK_SERVICE_DETAILS_RESPONSE; + }); - await expect( - controller.init({ - chainId: UNSUPPORTED_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + await bootstrap(); + + expect(order).toStrictEqual(['ensureChainConfigured', 'getServiceDetails']); + expect(mocks.ensureChainConfigured).toHaveBeenCalledWith(VAULT_CONFIG); + }); + + it('is idempotent: a second init() does not re-subscribe or re-bootstrap', async () => { + const { controller, mocks, bootstrap, triggerFlagChange } = setup(); + await bootstrap(); + + controller.init(); + await flushPromises(); + await triggerFlagChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('does not bootstrap when the isEnabled hook returns false', async () => { + const { mocks, bootstrap } = setup({ isEnabled: false }); + + await bootstrap(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + }); + + it('passes the current remote feature flags to the isEnabled hook', async () => { + const { mocks, bootstrap } = setup(); + + await bootstrap(); + + expect(mocks.isEnabled).toHaveBeenCalledWith( + expect.objectContaining({ + moneyAccountVaultConfig: VAULT_CONFIG, }), - ).rejects.toThrow( - `Delegation Framework 1.3.0 is not deployed on chain ${UNSUPPORTED_CHAIN_ID}`, ); + }); + + it('does not bootstrap while the wallet is locked', async () => { + const { mocks, bootstrap } = setup({ isUnlocked: false }); + + await bootstrap(); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); }); - it('uses the supplied boring vault address as the withdrawal-side delegation token', async () => { - const { controller, mocks } = setup(); + it('does not bootstrap while the keyring list has no HD keyring', async () => { + const { mocks, bootstrap } = setup({ hasHdKeyring: false }); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + await bootstrap(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + }); + + it('bootstraps on unlock via the keyring state change', async () => { + const { config, mocks, bootstrap, triggerKeyringChange } = setup({ + isUnlocked: false, }); - await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + await bootstrap(); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); - // Both delegations were signed; the boring-vault address shows up in the - // ABI-encoded ERC20TransferAmount caveat terms of one of them. - expect(mocks.signDelegation).toHaveBeenCalledTimes(2); - const allCaveatTerms = mocks.verifyDelegation.mock.calls - .flatMap(([{ signedDelegation }]) => signedDelegation.caveats) - .map((caveat) => caveat.terms.toLowerCase()); + config.isUnlocked = true; + await triggerKeyringChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('bootstraps when the isEnabled hook flips to true on a flag change', async () => { + const { config, mocks, bootstrap, triggerFlagChange } = setup({ + isEnabled: false, + }); + await bootstrap(); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + + config.isEnabled = true; + await triggerFlagChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('bootstraps when an external sync() reports a client gate reopened', async () => { + const { config, controller, mocks, bootstrap } = setup({ + isEnabled: false, + }); + await bootstrap(); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + + config.isEnabled = true; + controller.sync(); + await flushPromises(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('does not bootstrap while ineligible, then bootstraps once eligible', async () => { + const { config, mocks, bootstrap, triggerFlagChange } = setup({ + isEligible: false, + }); + await bootstrap(); + + expect(mocks.ensureChainConfigured).not.toHaveBeenCalled(); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + + config.isEligible = true; + await triggerFlagChange(); + + expect(mocks.ensureChainConfigured).toHaveBeenCalledTimes(1); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('skips the CHOMP call when the wallet locks while the chain is being configured', async () => { + let resolveEnsure: (value?: unknown) => void = () => undefined; + const { config, mocks, bootstrap, triggerKeyringChange } = setup(); + mocks.ensureChainConfigured + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveEnsure = resolve; + }), + ) + .mockResolvedValue(undefined); + + await bootstrap(); + expect(mocks.ensureChainConfigured).toHaveBeenCalledTimes(1); + + config.isUnlocked = false; + resolveEnsure(); + await flushPromises(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + + config.isUnlocked = true; + await triggerKeyringChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('skips the CHOMP call when isEnabled flips off while the chain is being configured', async () => { + let resolveEnsure: (value?: unknown) => void = () => undefined; + const { config, mocks, bootstrap, triggerFlagChange } = setup(); + mocks.ensureChainConfigured + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveEnsure = resolve; + }), + ) + .mockResolvedValue(undefined); + + await bootstrap(); + + config.isEnabled = false; + resolveEnsure(); + await flushPromises(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + + config.isEnabled = true; + await triggerFlagChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('reports a missing vault config through onBootstrapError only once', async () => { + const { mocks, bootstrap, triggerFlagChange } = setup({ + vaultConfig: null, + }); + await bootstrap(); + await triggerFlagChange(); + await triggerFlagChange(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + expect(mocks.onBootstrapError).toHaveBeenCalledTimes(1); + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + expect.any(MissingMoneyAccountVaultConfigError), + ); + }); + + it('reports a malformed vault config the same as a missing one', async () => { + const { mocks, bootstrap } = setup({ + vaultConfig: { ...VAULT_CONFIG, chainId: 'not-hex' }, + }); + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + expect.any(MissingMoneyAccountVaultConfigError), + ); + }); + + it('does not re-bootstrap when triggers repeat with the same config', async () => { + const { mocks, bootstrap, triggerFlagChange, triggerKeyringChange } = + setup(); + await bootstrap(); + await triggerFlagChange(); + await triggerKeyringChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + }); + + it('re-bootstraps when the vault config changes', async () => { + const { config, mocks, bootstrap, triggerFlagChange } = setup(); + await bootstrap(); + + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + + it('serializes a config-change bootstrap after the in-flight one', async () => { + let resolveFirst: (value?: unknown) => void = () => undefined; + const { config, mocks, bootstrap, triggerFlagChange } = setup(); + mocks.getServiceDetails + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve; + }), + ) + .mockResolvedValue(MOCK_SERVICE_DETAILS_RESPONSE); + + await bootstrap(); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + + resolveFirst(MOCK_SERVICE_DETAILS_RESPONSE); + await flushPromises(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + + it('keeps a newer scheduled config when the superseded bootstrap fails', async () => { + let rejectFirst: (error: Error) => void = () => undefined; + const { config, mocks, bootstrap, triggerFlagChange } = setup(); + mocks.getServiceDetails + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirst = reject; + }), + ) + .mockResolvedValue(MOCK_SERVICE_DETAILS_RESPONSE); + + await bootstrap(); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + + rejectFirst(new Error('CHOMP outage')); + await flushPromises(); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + + // The failure of the superseded run must not forget the newer config: + // a repeat trigger with the same config schedules nothing new. + await triggerFlagChange(); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + + it('retries a failed bootstrap on the next trigger and reports the failure', async () => { + const { mocks, bootstrap, triggerKeyringChange } = setup(); + const failure = new Error('CHOMP outage'); + mocks.getServiceDetails + .mockRejectedValueOnce(failure) + .mockResolvedValue(MOCK_SERVICE_DETAILS_RESPONSE); + + await bootstrap(); + await triggerKeyringChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + expect(mocks.onBootstrapError).toHaveBeenCalledWith(failure); + }); + + it('reports a bootstrap failure from the chain configuration hook', async () => { + const { mocks, bootstrap } = setup(); + const failure = new Error('addNetwork failed'); + mocks.ensureChainConfigured.mockRejectedValueOnce(failure); + + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith(failure); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + }); + + it('bootstraps with only the required isEnabled hook, defaulting the others', async () => { + const { controller, mocks, bootstrap } = setup({ + withOptionalHooks: false, + }); + + await bootstrap(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); expect( - allCaveatTerms.some((terms) => - terms.includes(MOCK_BORING_VAULT_ADDRESS.toLowerCase().slice(2)), - ), - ).toBe(true); + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).toBeUndefined(); }); - it('throws when the chain is not found in service details', async () => { - const { controller, mocks } = setup(); + it('swallows a bootstrap failure when no onBootstrapError hook is given', async () => { + const { controller, mocks, bootstrap } = setup({ + withOptionalHooks: false, + }); + mocks.getServiceDetails.mockRejectedValueOnce(new Error('CHOMP outage')); + + await bootstrap(); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); + }); + + it('swallows a missing vault config when no onBootstrapError hook is given', async () => { + const { mocks, bootstrap } = setup({ + withOptionalHooks: false, + vaultConfig: null, + }); + await bootstrap(); + + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + }); + + it('survives a throwing messenger call during sync', async () => { + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); + const failure = new Error('handler not registered'); + mocks.isEnabled.mockImplementationOnce(() => { + throw failure; + }); + + expect(() => controller.sync()).not.toThrow(); + expect(mocks.onBootstrapError).toHaveBeenCalledWith(failure); + }); + }); + + describe('bootstrap failures', () => { + it('reports when the chain has no Delegation Framework deployment', async () => { + const { mocks, bootstrap } = setup({ + vaultConfig: { ...VAULT_CONFIG, chainId: UNSUPPORTED_CHAIN_ID }, + }); + + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + new Error( + `Delegation Framework 1.3.0 is not deployed on chain ${UNSUPPORTED_CHAIN_ID}`, + ), + ); + expect(mocks.getServiceDetails).not.toHaveBeenCalled(); + }); + + it('reports when the chain is not found in service details', async () => { + const { mocks, bootstrap } = setup(); mocks.getServiceDetails.mockResolvedValue({ auth: { message: 'CHOMP Authentication' }, chains: {}, }); - await expect( - controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }), - ).rejects.toThrow( - `Chain ${MOCK_CHAIN_ID} not found in service details response`, + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + new Error(`Chain ${MOCK_CHAIN_ID} not found in service details response`), ); }); - it('throws when vedaProtocol is not found', async () => { - const { controller, mocks } = setup(); - + it('reports when vedaProtocol is not found', async () => { + const { mocks, bootstrap } = setup(); mocks.getServiceDetails.mockResolvedValue({ auth: { message: 'CHOMP Authentication' }, chains: { @@ -365,19 +818,17 @@ describe('MoneyAccountUpgradeController', () => { }, }); - await expect( - controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }), - ).rejects.toThrow( - `vedaProtocol not found for chain ${MOCK_CHAIN_ID} in service details response`, + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + new Error( + `vedaProtocol not found for chain ${MOCK_CHAIN_ID} in service details response`, + ), ); }); - it('throws when supportedTokens is empty', async () => { - const { controller, mocks } = setup(); - + it('reports when supportedTokens is empty', async () => { + const { mocks, bootstrap } = setup(); mocks.getServiceDetails.mockResolvedValue({ auth: { message: 'CHOMP Authentication' }, chains: { @@ -394,54 +845,87 @@ describe('MoneyAccountUpgradeController', () => { }, }); - await expect( - controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }), - ).rejects.toThrow( - `No supported tokens found for vedaProtocol on chain ${MOCK_CHAIN_ID}`, + await bootstrap(); + + expect(mocks.onBootstrapError).toHaveBeenCalledWith( + new Error( + `No supported tokens found for vedaProtocol on chain ${MOCK_CHAIN_ID}`, + ), ); }); }); describe('upgradeAccount', () => { - it('throws when called before init', async () => { - const { controller } = setup(); + it('throws when no bootstrap has been scheduled', async () => { + const { controller } = setup({ isEnabled: false }); + controller.init(); await expect( controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), - ).rejects.toThrow( - 'MoneyAccountUpgradeController must be initialized via init() before upgradeAccount() can be called', - ); + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); }); - it('throws when a previous init attempt failed', async () => { - const { controller, mocks } = setup(); - mocks.getServiceDetails.mockResolvedValueOnce({ + it('throws when the bootstrap failed', async () => { + const { controller, mocks, bootstrap } = setup(); + mocks.getServiceDetails.mockResolvedValue({ auth: { message: 'CHOMP Authentication' }, chains: {}, }); - await expect( - controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }), - ).rejects.toThrow('Chain 0x1 not found in service details response'); + await bootstrap(); await expect( controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), - ).rejects.toThrow( - 'MoneyAccountUpgradeController must be initialized via init() before upgradeAccount() can be called', + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); + }); + + it('waits for an in-flight bootstrap instead of throwing', async () => { + let resolveServiceDetails: (value?: unknown) => void = () => undefined; + const { controller, mocks } = setup(); + mocks.getServiceDetails.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveServiceDetails = resolve; + }), ); + controller.init(); + await flushPromises(); + + const upgrade = controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + resolveServiceDetails(MOCK_SERVICE_DETAILS_RESPONSE); + + expect(await upgrade).toBeUndefined(); + expect(mocks.signPersonalMessage).toHaveBeenCalled(); + }); + + it('throws after the isEnabled hook flips off and a sync disarms the controller', async () => { + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + await bootstrap(); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + + config.isEnabled = false; + await triggerFlagChange(); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); + }); + + it('re-bootstraps from scratch after being disarmed', async () => { + const { config, mocks, bootstrap, triggerFlagChange } = setup(); + await bootstrap(); + + config.isEnabled = false; + await triggerFlagChange(); + config.isEnabled = true; + await triggerFlagChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); }); it('runs each step against the deployment-derived contract addresses', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); @@ -466,12 +950,28 @@ describe('MoneyAccountUpgradeController', () => { ); }); + it('uses the vault config boring vault as the withdrawal-side delegation token', async () => { + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); + + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + // Both delegations were signed; the boring-vault address shows up in the + // ABI-encoded ERC20TransferAmount caveat terms of one of them. + expect(mocks.signDelegation).toHaveBeenCalledTimes(2); + const allCaveatTerms = mocks.verifyDelegation.mock.calls + .flatMap(([{ signedDelegation }]) => signedDelegation.caveats) + .map((caveat) => caveat.terms.toLowerCase()); + expect( + allCaveatTerms.some((terms) => + terms.includes(MOCK_BORING_VAULT_ADDRESS.toLowerCase().slice(2)), + ), + ).toBe(true); + }); + it('is callable via the messenger', async () => { - const { controller, rootMessenger } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { rootMessenger, bootstrap } = setup(); + await bootstrap(); expect( await rootMessenger.call( @@ -482,11 +982,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('propagates errors thrown by a step', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); mocks.signPersonalMessage.mockRejectedValue(new Error('signing failed')); await expect( @@ -495,11 +992,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('wraps a step failure in a MoneyAccountUpgradeStepError that records the step and cause', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); const cause = new Error('signing failed'); // The associate-address step (first in the sequence) signs a personal // message before calling CHOMP, so failing this surfaces that step. @@ -520,11 +1014,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('records the name of the specific step that failed', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); // The first step (associate-address) passes; fail at the second step // (eip-7702-authorization), which signs the authorization. mocks.signEip7702Authorization.mockRejectedValue( @@ -539,11 +1030,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('wraps a non-Error thrown by a step, stringifying it as the cause message', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); mocks.signPersonalMessage.mockRejectedValue('plain string failure'); const error = await controller @@ -560,11 +1048,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('marks the failure terminal when the account is delegated to another implementation', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); // EIP-7702 delegation code pointing at a third-party impl. mocks.providerRequest.mockImplementation( async ({ method }: { method: string }) => { @@ -583,11 +1068,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('marks ordinary step failures as non-terminal', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); mocks.signPersonalMessage.mockRejectedValue(new Error('network down')); const error = await controller @@ -601,11 +1083,8 @@ describe('MoneyAccountUpgradeController', () => { describe('upgrade status tracking', () => { it('records a successful upgrade against the lowercased address', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); const mixedCaseAddress = MOCK_ACCOUNT_ADDRESS.replace( '0xabc', '0xABC', @@ -623,11 +1102,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('skips the steps on a subsequent call for an already-upgraded account', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); clearMockCalls(mocks); @@ -640,11 +1116,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('treats recorded upgrades case-insensitively', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); clearMockCalls(mocks); @@ -657,17 +1130,11 @@ describe('MoneyAccountUpgradeController', () => { it('skips the steps when constructed with state from a previous successful upgrade', async () => { const first = setup(); - await first.controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + await first.bootstrap(); await first.controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); const second = setup({ state: first.controller.state }); - await second.controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + await second.bootstrap(); await second.controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); expect(second.mocks.signPersonalMessage).not.toHaveBeenCalled(); @@ -675,11 +1142,8 @@ describe('MoneyAccountUpgradeController', () => { }); it('does not record the account when a step fails, and re-runs on the next call', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); mocks.signPersonalMessage.mockRejectedValueOnce( new Error('signing failed'), ); @@ -698,17 +1162,16 @@ describe('MoneyAccountUpgradeController', () => { }); it('re-runs the sequence when the active config no longer matches the recorded fingerprint', async () => { - const { controller, mocks } = setup(); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + await bootstrap(); await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); const { configFingerprint: originalFingerprint } = controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS]; - // CHOMP rotates its delegate address — the recorded upgrade no longer - // reflects the active config. + // CHOMP rotates its delegate address, published alongside a vault + // config refresh — the recorded upgrade no longer reflects the active + // config. mocks.getServiceDetails.mockResolvedValue({ ...MOCK_SERVICE_DETAILS_RESPONSE, chains: { @@ -719,10 +1182,8 @@ describe('MoneyAccountUpgradeController', () => { }, }, }); - await controller.init({ - chainId: MOCK_CHAIN_ID, - boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, - }); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); clearMockCalls(mocks); await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index e912f408424..d6ef5461814 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -19,19 +19,35 @@ import type { } from '@metamask/chomp-api-service'; import type { DelegationControllerSignDelegationAction } from '@metamask/delegation-controller'; import { DELEGATOR_CONTRACTS } from '@metamask/delegation-deployments'; +import { KeyringTypes } from '@metamask/keyring-controller'; import type { + KeyringControllerGetStateAction, KeyringControllerSignEip7702AuthorizationAction, KeyringControllerSignPersonalMessageAction, + KeyringControllerState, } from '@metamask/keyring-controller'; import type { Messenger } from '@metamask/messenger'; +import { + areMoneyAccountVaultConfigsEqual, + getMoneyAccountVaultConfig, +} from '@metamask/money-account-utils'; +import type { MoneyAccountVaultConfig } from '@metamask/money-account-utils'; import type { NetworkControllerFindNetworkClientIdByChainIdAction, NetworkControllerGetNetworkClientByIdAction, } from '@metamask/network-controller'; +import type { + FeatureFlags, + RemoteFeatureFlagControllerGetStateAction, + RemoteFeatureFlagControllerState, +} from '@metamask/remote-feature-flag-controller'; import { hexToNumber } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; -import { MoneyAccountUpgradeStepError } from './errors.js'; +import { + MissingMoneyAccountVaultConfigError, + MoneyAccountUpgradeStepError, +} from './errors.js'; import type { MoneyAccountUpgradeControllerMethodActions } from './MoneyAccountUpgradeController-method-action-types.js'; import { associateAddressStep } from './steps/associate-address.js'; import { buildDelegationStep } from './steps/build-delegations.js'; @@ -117,10 +133,12 @@ type AllowedActions = | ChompApiServiceGetServiceDetailsAction | ChompApiServiceVerifyDelegationAction | DelegationControllerSignDelegationAction + | KeyringControllerGetStateAction | KeyringControllerSignEip7702AuthorizationAction | KeyringControllerSignPersonalMessageAction | NetworkControllerFindNetworkClientIdByChainIdAction - | NetworkControllerGetNetworkClientByIdAction; + | NetworkControllerGetNetworkClientByIdAction + | RemoteFeatureFlagControllerGetStateAction; export type MoneyAccountUpgradeControllerStateChangedEvent = ControllerStateChangedEvent< @@ -131,7 +149,12 @@ export type MoneyAccountUpgradeControllerStateChangedEvent = export type MoneyAccountUpgradeControllerEvents = MoneyAccountUpgradeControllerStateChangedEvent; -type AllowedEvents = never; +type AllowedEvents = + | ControllerStateChangedEvent<'KeyringController', KeyringControllerState> + | ControllerStateChangedEvent< + 'RemoteFeatureFlagController', + RemoteFeatureFlagControllerState + >; export type MoneyAccountUpgradeControllerMessenger = Messenger< typeof controllerName, @@ -140,7 +163,70 @@ export type MoneyAccountUpgradeControllerMessenger = Messenger< >; /** - * Controller that orchestrates the Money Account upgrade sequence. + * The hooks a client supplies for the parts of the bootstrap that cannot be + * decided in this package. + */ +export type MoneyAccountUpgradeControllerHooks = { + /** + * Whether the Money Account feature is enabled for this client. Called with + * the current remote feature flags on every sync and re-checked across the + * bootstrap's `await` points; it must re-read any client state it depends + * on (e.g. a version-gated flag, a "basic functionality" toggle) rather + * than caching it. Returning `false` after a successful bootstrap disarms + * the controller: `upgradeAccount` refuses to run until a later sync + * re-bootstraps. + */ + isEnabled: (remoteFeatureFlags: FeatureFlags) => boolean; + + /** + * An asynchronous client gate checked once per bootstrap run, before any + * network is added or external service is called — e.g. a fail-closed + * geolocation check. A run skipped here is forgotten and retried on the + * next sync trigger. Defaults to always eligible. + */ + isEligible?: () => Promise; + + /** + * Ensure the vault chain exists in the client's NetworkController before + * the bootstrap validates it. Adding a network is client-specific (featured + * network lists, enabled-network bookkeeping), so the controller only + * promises to have awaited this before calling + * `NetworkController:findNetworkClientIdByChainId` consumers. Defaults to a + * no-op. + */ + ensureChainConfigured?: (vaultConfig: MoneyAccountVaultConfig) => Promise; + + /** + * Called when a bootstrap run fails or cannot be scheduled. Receives a + * {@link MissingMoneyAccountVaultConfigError} (once per controller + * lifetime) when the enable flag is on but `moneyAccountVaultConfig` is + * unserved or malformed. The controller never throws out of its + * subscriptions; this hook is the only failure signal. + */ + onBootstrapError?: (error: unknown) => void; +}; + +/** + * Controller that owns the Money Account upgrade sequence and its own + * bootstrap. + * + * After {@link MoneyAccountUpgradeController.init} is called (once, after all + * controllers are constructed), the controller watches + * `RemoteFeatureFlagController` and `KeyringController` state and bootstraps + * itself when every gate is open: + * + * 1. the client's `isEnabled` hook returns `true` for the current flags, + * 2. the wallet is unlocked with an HD keyring, + * 3. the client's `isEligible` hook (if any) resolves `true`, and + * 4. the `moneyAccountVaultConfig` flag parses. + * + * The bootstrap awaits the client's `ensureChainConfigured` hook and then + * fetches CHOMP service details to arm the upgrade config. Gates 1–2 are + * re-checked across the `await` points so a lock or an `isEnabled` flip + * mid-bootstrap cannot still produce an external call; a skipped or failed + * run is forgotten so the next trigger retries it. The bootstrap re-runs + * whenever the vault config changes, and `isEnabled` going `false` disarms + * the controller entirely. */ export class MoneyAccountUpgradeController extends BaseController< typeof controllerName, @@ -149,6 +235,28 @@ export class MoneyAccountUpgradeController extends BaseController< > { #config?: UpgradeConfig & { chainId: Hex }; + readonly #isEnabled: MoneyAccountUpgradeControllerHooks['isEnabled']; + + readonly #isEligible: NonNullable< + MoneyAccountUpgradeControllerHooks['isEligible'] + >; + + readonly #ensureChainConfigured: NonNullable< + MoneyAccountUpgradeControllerHooks['ensureChainConfigured'] + >; + + readonly #onBootstrapError: NonNullable< + MoneyAccountUpgradeControllerHooks['onBootstrapError'] + >; + + #initialized = false; + + #bootstrap?: Promise; + + #bootstrappedConfig?: MoneyAccountVaultConfig; + + #missingConfigReported = false; + readonly #steps: Step[] = [ associateAddressStep, eip7702AuthorizationStep, @@ -162,13 +270,17 @@ export class MoneyAccountUpgradeController extends BaseController< * @param options - The options for constructing the controller. * @param options.messenger - The messenger to use for inter-controller communication. * @param options.state - The initial state, merged with the defaults. + * @param options.hooks - The client hooks for the bootstrap; see + * {@link MoneyAccountUpgradeControllerHooks}. */ constructor({ messenger, state, + hooks, }: { messenger: MoneyAccountUpgradeControllerMessenger; state?: Partial; + hooks: MoneyAccountUpgradeControllerHooks; }) { super({ messenger, @@ -180,31 +292,191 @@ export class MoneyAccountUpgradeController extends BaseController< }, }); + this.#isEnabled = hooks.isEnabled; + this.#isEligible = hooks.isEligible ?? (async (): Promise => true); + this.#ensureChainConfigured = + hooks.ensureChainConfigured ?? (async (): Promise => undefined); + this.#onBootstrapError = hooks.onBootstrapError ?? ((): void => undefined); + this.messenger.registerMethodActionHandlers( this, MESSENGER_EXPOSED_METHODS, ); } + /** + * Start the controller's bootstrap: subscribe to the feature-flag and + * keyring triggers and run an initial sync. Call once, after all + * controllers and services the messenger reaches are constructed — this is + * the only method that may be called before the controller is bootstrapped, + * and it is the reason the constructor performs no messenger calls. + */ + init(): void { + if (this.#initialized) { + return; + } + this.#initialized = true; + + this.messenger.subscribe('RemoteFeatureFlagController:stateChanged', () => + this.sync(), + ); + this.messenger.subscribe('KeyringController:stateChanged', () => + this.sync(), + ); + this.sync(); + } + + /** + * Re-evaluate the bootstrap gates against live state and schedule a + * bootstrap run when they are open and the vault config is new. Runs on + * every feature-flag and keyring state change; clients with additional + * gates read inside their `isEnabled` hook (onboarding, preferences) + * should call this when those change too. Never throws: a failure is + * reported through `onBootstrapError` and retried on the next trigger. + */ + sync(): void { + try { + const { remoteFeatureFlags } = this.messenger.call( + 'RemoteFeatureFlagController:getState', + ); + + if (!this.#isEnabled(remoteFeatureFlags)) { + // Disarm so a later `upgradeAccount` cannot run against a config + // armed while the feature was still enabled. Re-enabling re-runs the + // bootstrap from scratch. + this.#config = undefined; + this.#bootstrappedConfig = undefined; + return; + } + + if (!this.#isWalletReady()) { + return; + } + + const vaultConfig = getMoneyAccountVaultConfig(remoteFeatureFlags); + if (!vaultConfig) { + this.#reportMissingConfig(); + return; + } + + if ( + this.#bootstrappedConfig && + areMoneyAccountVaultConfigsEqual(vaultConfig, this.#bootstrappedConfig) + ) { + return; + } + + this.#scheduleBootstrap(vaultConfig); + } catch (error) { + this.#onBootstrapError(error); + } + } + + /** + * Whether the wallet can sign right now: unlocked with an HD keyring. The + * keyring list matters because during a vault restore the unlock flips + * before the keyrings land in state. + * + * @returns Whether the wallet is ready. + */ + #isWalletReady(): boolean { + const { isUnlocked, keyrings } = this.messenger.call( + 'KeyringController:getState', + ); + return ( + isUnlocked && keyrings.some((keyring) => keyring.type === KeyringTypes.hd) + ); + } + + /** + * Whether the synchronous bootstrap gates are open right now. Re-read from + * live state on every call because the bootstrap re-validates them across + * its `await` points. + * + * @returns Whether the bootstrap may proceed. + */ + #areGatesOpen(): boolean { + const { remoteFeatureFlags } = this.messenger.call( + 'RemoteFeatureFlagController:getState', + ); + return this.#isEnabled(remoteFeatureFlags) && this.#isWalletReady(); + } + + #scheduleBootstrap(vaultConfig: MoneyAccountVaultConfig): void { + this.#bootstrappedConfig = vaultConfig; + + const run = async (): Promise => { + // The gates were checked when this run was scheduled, but it may start + // much later, chained behind an in-flight bootstrap. Eligibility comes + // after the gates so a disabled client also skips the (possibly + // external) eligibility lookup. + if (!this.#areGatesOpen() || !(await this.#isEligible())) { + this.#forget(vaultConfig); + return; + } + + await this.#ensureChainConfigured(vaultConfig); + + // Configuring the chain can suspend for a while, so re-check the gates + // before the external CHOMP call. + if (!this.#areGatesOpen()) { + this.#forget(vaultConfig); + return; + } + + await this.#applyVaultConfig(vaultConfig); + }; + + const bootstrap = this.#bootstrap + ? this.#bootstrap.catch(() => undefined).then(run) + : run(); + this.#bootstrap = bootstrap; + + bootstrap.catch((error) => { + this.#onBootstrapError(error); + this.#forget(vaultConfig); + }); + } + + /** + * Forget a scheduled bootstrap that was skipped or failed, so the next + * trigger re-runs it — but only if no newer config has been scheduled + * meanwhile: a newer config supersedes this run, success or failure. + * + * @param vaultConfig - The config the abandoned run was scheduled with. + */ + #forget(vaultConfig: MoneyAccountVaultConfig): void { + if (this.#bootstrappedConfig === vaultConfig) { + this.#bootstrappedConfig = undefined; + } + } + + /** + * Report a served enable flag without a usable `moneyAccountVaultConfig` — + * a flag misconfiguration that silently disables upgrades. Reported once + * per controller lifetime; flag refreshes arrive continuously and would + * otherwise spam. + */ + #reportMissingConfig(): void { + if (!this.#missingConfigReported) { + this.#missingConfigReported = true; + this.#onBootstrapError(new MissingMoneyAccountVaultConfigError()); + } + } + /** * Fetches service details and validates the controller can operate on the - * given chain. Resolves the Delegation Framework contract addresses for the - * chain from `@metamask/delegation-deployments`. + * vault's chain, arming the upgrade config `upgradeAccount` runs against. + * Resolves the Delegation Framework contract addresses for the chain from + * `@metamask/delegation-deployments`. * - * @param params - The parameters for initialization. - * @param params.chainId - The chain to initialize for. - * @param params.boringVaultAddress - The Veda boring vault contract - * (vmUSD) for the given chain. Used as the withdrawal-side delegation - * token. Supplied by the consumer until the CHOMP service-details API - * exposes it. + * @param vaultConfig - The vault config to arm; its `boringVault` is the + * withdrawal-side delegation token (vmUSD), supplied via the flag until + * the CHOMP service-details API exposes it. */ - async init({ - chainId, - boringVaultAddress, - }: { - chainId: Hex; - boringVaultAddress: Hex; - }): Promise { + async #applyVaultConfig(vaultConfig: MoneyAccountVaultConfig): Promise { + const { chainId, boringVault: boringVaultAddress } = vaultConfig; + const contracts = DELEGATOR_CONTRACTS[DELEGATION_FRAMEWORK_VERSION][hexToNumber(chainId)]; if (!contracts) { @@ -263,12 +535,19 @@ export class MoneyAccountUpgradeController extends BaseController< * active config no longer matches the recorded fingerprint, the sequence * re-runs. * + * A call that arrives while the bootstrap is still in flight waits for it + * rather than failing; it only throws when no bootstrap has armed a config + * (feature disabled, wallet locked, or the last bootstrap failed). + * * @param address - The Money Account address to upgrade. */ async upgradeAccount(address: Hex): Promise { + if (!this.#config && this.#bootstrap) { + await this.#bootstrap.catch(() => undefined); + } if (!this.#config) { throw new Error( - 'MoneyAccountUpgradeController must be initialized via init() before upgradeAccount() can be called', + 'MoneyAccountUpgradeController is not bootstrapped: upgradeAccount() requires the feature flag on, the wallet unlocked, and a successful bootstrap', ); } const config = this.#config; diff --git a/packages/money-account-upgrade-controller/src/errors.ts b/packages/money-account-upgrade-controller/src/errors.ts index 1aef34ff3c8..8e648d2ea37 100644 --- a/packages/money-account-upgrade-controller/src/errors.ts +++ b/packages/money-account-upgrade-controller/src/errors.ts @@ -52,6 +52,21 @@ export class TerminalUpgradeError extends Error { } } +/** + * Error reported through the `onBootstrapError` hook when the Money Account + * enable flag is on but the `moneyAccountVaultConfig` flag is unserved or + * malformed — a flag misconfiguration that silently disables upgrades. + * Reported once per controller lifetime. + */ +export class MissingMoneyAccountVaultConfigError extends Error { + constructor() { + super( + 'Money Account upgrade bootstrap skipped: vault configuration is unavailable', + ); + this.name = 'MissingMoneyAccountVaultConfigError'; + } +} + /** * Type guard for {@link MoneyAccountUpgradeStepError}. * diff --git a/packages/money-account-upgrade-controller/src/index.ts b/packages/money-account-upgrade-controller/src/index.ts index ddb674d4114..8db25bd1d9a 100644 --- a/packages/money-account-upgrade-controller/src/index.ts +++ b/packages/money-account-upgrade-controller/src/index.ts @@ -1,5 +1,6 @@ export type { UpgradeConfig } from './types.js'; export { + MissingMoneyAccountVaultConfigError, MoneyAccountUpgradeStepError, TerminalUpgradeError, isMoneyAccountUpgradeStepError, @@ -13,6 +14,7 @@ export type { MoneyAccountUpgradeControllerState, MoneyAccountUpgradeControllerGetStateAction, MoneyAccountUpgradeControllerActions, + MoneyAccountUpgradeControllerHooks, MoneyAccountUpgradeControllerStateChangedEvent, MoneyAccountUpgradeControllerEvents, MoneyAccountUpgradeControllerMessenger, diff --git a/packages/money-account-upgrade-controller/tsconfig.build.json b/packages/money-account-upgrade-controller/tsconfig.build.json index 66f113f221d..dbc68c02926 100644 --- a/packages/money-account-upgrade-controller/tsconfig.build.json +++ b/packages/money-account-upgrade-controller/tsconfig.build.json @@ -24,8 +24,14 @@ { "path": "../messenger/tsconfig.build.json" }, + { + "path": "../money-account-utils/tsconfig.build.json" + }, { "path": "../network-controller/tsconfig.build.json" + }, + { + "path": "../remote-feature-flag-controller/tsconfig.build.json" } ], "include": ["../../types", "./src"] diff --git a/packages/money-account-upgrade-controller/tsconfig.json b/packages/money-account-upgrade-controller/tsconfig.json index 1273483ee1d..171a6c971a2 100644 --- a/packages/money-account-upgrade-controller/tsconfig.json +++ b/packages/money-account-upgrade-controller/tsconfig.json @@ -22,8 +22,14 @@ { "path": "../messenger" }, + { + "path": "../money-account-utils" + }, { "path": "../network-controller" + }, + { + "path": "../remote-feature-flag-controller" } ], "include": ["../../types", "./src"] diff --git a/packages/money-account-utils/src/index.ts b/packages/money-account-utils/src/index.ts index cb74ffbacee..fa6762dd53d 100644 --- a/packages/money-account-utils/src/index.ts +++ b/packages/money-account-utils/src/index.ts @@ -22,6 +22,13 @@ export { getMoneyAccountDepositAssetId, getSharesForWithdrawal, } from './transactions.js'; +export { + MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME, + areMoneyAccountVaultConfigsEqual, + getMoneyAccountVaultConfig, + parseMoneyAccountVaultConfig, +} from './vault-config.js'; +export type { MoneyAccountVaultConfig } from './vault-config.js'; export type { BuildMoneyAccountDepositBatchOptions, BuildMoneyAccountDepositPlaceholderBatchOptions, diff --git a/packages/money-account-utils/src/vault-config.test.ts b/packages/money-account-utils/src/vault-config.test.ts new file mode 100644 index 00000000000..f3769a9189d --- /dev/null +++ b/packages/money-account-utils/src/vault-config.test.ts @@ -0,0 +1,155 @@ +import type { MoneyAccountVaultConfig } from './vault-config.js'; +import { + areMoneyAccountVaultConfigsEqual, + getMoneyAccountVaultConfig, + parseMoneyAccountVaultConfig, +} from './vault-config.js'; + +const VALID_CONFIG = { + chainId: '0x8f', + boringVault: '0xb4563bcD3B7764CCBf497f515585f70B6C3EA5Ae', + tellerAddress: '0x2D49EA58A4C70b62c8B56DE971310d9e999c8117', + accountantAddress: '0x7382c5b8B51B8C4f127B3123C1039581BAA5A06B', + lensAddress: '0xA816ECd922de94c6879AD23B9A884dB257F20947', + underlyingToken: '0xacA92E438df0B2401fF60dA7E4337B687a2435DA', +} as const; + +const ADDRESS_KEYS = [ + 'boringVault', + 'tellerAddress', + 'accountantAddress', + 'lensAddress', + 'underlyingToken', +] as const; + +const INVALID_CHAIN_IDS: [string, unknown][] = [ + ['is not prefixed', '8f'], + ['has non-hex digits', '0xmonad'], + ['is missing', undefined], + ['is a number', 143], + ['is null', null], + ['is empty', ''], +]; + +const INVALID_ADDRESSES: [string, unknown][] = [ + ['is not hex', 'not-an-address'], + ['is truncated', '0xb4563bcD3B7764CCBf497f5'], + // A valid address but with its final byte upper-cased: still 20 hex bytes, + // but no longer a valid ERC-55 checksum. + ['has a bad ERC-55 checksum', '0xb4563bcD3B7764CCBf497f515585f70B6C3EA5AE'], + ['is missing', undefined], + ['is not a string', 1234], + ['is null', null], + ['is empty', ''], +]; + +const NON_OBJECT_FLAGS: [string, unknown][] = [ + ['a string', 'moneyAccountVaultConfig'], + ['a number', 1], + ['null', null], + ['undefined', undefined], + ['an array', [VALID_CONFIG]], + ['an empty object', {}], +]; + +describe('parseMoneyAccountVaultConfig', () => { + it('parses a well-formed config', () => { + expect(parseMoneyAccountVaultConfig(VALID_CONFIG)).toStrictEqual( + VALID_CONFIG, + ); + }); + + it('passes checksummed addresses through unchanged, without normalising', () => { + const parsed = parseMoneyAccountVaultConfig(VALID_CONFIG); + + expect(parsed?.boringVault).toBe(VALID_CONFIG.boringVault); + expect(parsed?.tellerAddress).toBe(VALID_CONFIG.tellerAddress); + expect(parsed?.accountantAddress).toBe(VALID_CONFIG.accountantAddress); + expect(parsed?.lensAddress).toBe(VALID_CONFIG.lensAddress); + expect(parsed?.underlyingToken).toBe(VALID_CONFIG.underlyingToken); + }); + + it('accepts all-lowercase addresses', () => { + const lowercased = { + ...VALID_CONFIG, + boringVault: VALID_CONFIG.boringVault.toLowerCase(), + }; + + expect(parseMoneyAccountVaultConfig(lowercased)).toStrictEqual(lowercased); + }); + + it('ignores unknown extra fields', () => { + expect( + parseMoneyAccountVaultConfig({ ...VALID_CONFIG, someFutureField: 1 }), + ).toStrictEqual(VALID_CONFIG); + }); + + for (const [description, chainId] of INVALID_CHAIN_IDS) { + it(`rejects a config whose chain id ${description}`, () => { + expect( + parseMoneyAccountVaultConfig({ ...VALID_CONFIG, chainId }), + ).toBeUndefined(); + }); + } + + for (const key of ADDRESS_KEYS) { + for (const [description, value] of INVALID_ADDRESSES) { + it(`rejects a config whose ${key} ${description}`, () => { + expect( + parseMoneyAccountVaultConfig({ ...VALID_CONFIG, [key]: value }), + ).toBeUndefined(); + }); + } + } + + for (const [description, raw] of NON_OBJECT_FLAGS) { + it(`rejects a flag that is ${description}`, () => { + expect(parseMoneyAccountVaultConfig(raw)).toBeUndefined(); + }); + } +}); + +describe('getMoneyAccountVaultConfig', () => { + it('parses the config out of the remote feature flags', () => { + expect( + getMoneyAccountVaultConfig({ + moneyAccountVaultConfig: { ...VALID_CONFIG }, + }), + ).toStrictEqual(VALID_CONFIG); + }); + + it('returns undefined when the flag is unserved', () => { + expect(getMoneyAccountVaultConfig({})).toBeUndefined(); + }); + + it('returns undefined when there are no flags at all', () => { + expect(getMoneyAccountVaultConfig(undefined)).toBeUndefined(); + }); + + it('returns undefined when the flag is malformed', () => { + expect( + getMoneyAccountVaultConfig({ + moneyAccountVaultConfig: { ...VALID_CONFIG, lensAddress: '0x0' }, + }), + ).toBeUndefined(); + }); +}); + +describe('areMoneyAccountVaultConfigsEqual', () => { + const config = VALID_CONFIG as MoneyAccountVaultConfig; + + it('treats identical configs as equal', () => { + expect(areMoneyAccountVaultConfigsEqual(config, { ...config })).toBe(true); + }); + + for (const key of ['chainId', ...ADDRESS_KEYS] as const) { + it(`treats configs differing in ${key} as unequal`, () => { + expect( + areMoneyAccountVaultConfigsEqual(config, { + ...config, + [key]: '0x1111111111111111111111111111111111111111', + }), + ).toBe(false); + }); + } +}); diff --git a/packages/money-account-utils/src/vault-config.ts b/packages/money-account-utils/src/vault-config.ts new file mode 100644 index 00000000000..2b20f867807 --- /dev/null +++ b/packages/money-account-utils/src/vault-config.ts @@ -0,0 +1,121 @@ +import { isObject, isStrictHexString, isValidHexAddress } from '@metamask/utils'; +import type { Hex } from '@metamask/utils'; + +/** + * The LaunchDarkly flag carrying the Money Account vault contracts. The same + * flag `@metamask/money-account-balance-service` reads, so the parsed chain id + * here is the chain the balance service talks to. + */ +export const MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME = 'moneyAccountVaultConfig'; + +/** + * The Money Account vault contracts served via remote feature flags, with the + * chain id and every address validated as known-good `Hex`. + */ +export type MoneyAccountVaultConfig = { + chainId: Hex; + boringVault: Hex; + tellerAddress: Hex; + accountantAddress: Hex; + lensAddress: Hex; + underlyingToken: Hex; +}; + +/** + * Parses one raw vault-config field into an address. + * + * `isValidHexAddress` accepts an all-lowercase address or a valid ERC-55 + * checksum, which is what ethers accepts when it encodes the calldata. The + * address is deliberately not normalised, so a checksummed address passes + * through unchanged. + * + * @param value - The raw field value. + * @returns The address, or `undefined` if it is missing or malformed. + */ +const parseAddress = (value: unknown): Hex | undefined => + isStrictHexString(value) && isValidHexAddress(value) ? value : undefined; + +/** + * Parses the raw `moneyAccountVaultConfig` remote feature flag into a config + * whose chain id and addresses are known-good `Hex`. + * + * @param raw - The raw remote feature flag value. + * @returns The parsed vault config, or `undefined` if any field is missing or + * malformed. + */ +export const parseMoneyAccountVaultConfig = ( + raw: unknown, +): MoneyAccountVaultConfig | undefined => { + if (!isObject(raw)) { + return undefined; + } + + const { chainId } = raw; + if (!isStrictHexString(chainId)) { + return undefined; + } + + const boringVault = parseAddress(raw.boringVault); + const tellerAddress = parseAddress(raw.tellerAddress); + const accountantAddress = parseAddress(raw.accountantAddress); + const lensAddress = parseAddress(raw.lensAddress); + const underlyingToken = parseAddress(raw.underlyingToken); + + if ( + !boringVault || + !tellerAddress || + !accountantAddress || + !lensAddress || + !underlyingToken + ) { + return undefined; + } + + return { + chainId, + boringVault, + tellerAddress, + accountantAddress, + lensAddress, + underlyingToken, + }; +}; + +/** + * Reads and parses the Money Account vault config out of the remote feature + * flags. + * + * @param remoteFeatureFlags - The remote feature flags. + * @returns The parsed vault config, or `undefined` when the flag is unserved + * or malformed. + */ +export function getMoneyAccountVaultConfig( + remoteFeatureFlags: Record | undefined, +): MoneyAccountVaultConfig | undefined { + return parseMoneyAccountVaultConfig( + remoteFeatureFlags?.[MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME], + ); +} + +/** + * Compares vault configs field by field. Any difference means the vault the + * config points at has changed and consumers keyed on it (bootstraps, caches) + * must re-run. + * + * @param a - One vault config. + * @param b - The other vault config. + * @returns Whether the configs are equal. + */ +export function areMoneyAccountVaultConfigsEqual( + a: MoneyAccountVaultConfig, + b: MoneyAccountVaultConfig, +): boolean { + return ( + a.chainId === b.chainId && + a.boringVault === b.boringVault && + a.tellerAddress === b.tellerAddress && + a.accountantAddress === b.accountantAddress && + a.lensAddress === b.lensAddress && + a.underlyingToken === b.underlyingToken + ); +} diff --git a/yarn.lock b/yarn.lock index 1e88d4058a7..4d85d17a04a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7792,7 +7792,9 @@ __metadata: "@metamask/delegation-deployments": "npm:^1.4.0" "@metamask/keyring-controller": "npm:^27.1.1" "@metamask/messenger": "npm:^2.0.0" + "@metamask/money-account-utils": "npm:^1.1.0" "@metamask/network-controller": "npm:^36.0.0" + "@metamask/remote-feature-flag-controller": "npm:^6.1.0" "@metamask/utils": "npm:^11.12.0" "@ts-bridge/cli": "npm:^0.6.4" "@types/jest": "npm:^30.0.0" @@ -7807,7 +7809,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/money-account-utils@workspace:packages/money-account-utils": +"@metamask/money-account-utils@npm:^1.1.0, @metamask/money-account-utils@workspace:packages/money-account-utils": version: 0.0.0-use.local resolution: "@metamask/money-account-utils@workspace:packages/money-account-utils" dependencies: @@ -9302,7 +9304,26 @@ __metadata: languageName: unknown linkType: soft -"@metamask/utils@npm:^11.0.1, @metamask/utils@npm:^11.10.0, @metamask/utils@npm:^11.11.0, @metamask/utils@npm:^11.12.0, @metamask/utils@npm:^11.4.0, @metamask/utils@npm:^11.4.2, @metamask/utils@npm:^11.8.1, @metamask/utils@npm:^11.9.0": +"@metamask/utils@npm:^11.0.1, @metamask/utils@npm:^11.10.0, @metamask/utils@npm:^11.11.0, @metamask/utils@npm:^11.4.0, @metamask/utils@npm:^11.4.2, @metamask/utils@npm:^11.8.1, @metamask/utils@npm:^11.9.0": + version: 11.11.0 + resolution: "@metamask/utils@npm:11.11.0" + dependencies: + "@ethereumjs/tx": "npm:^4.2.0" + "@metamask/superstruct": "npm:^3.1.0" + "@noble/hashes": "npm:^1.3.1" + "@scure/base": "npm:^1.1.3" + "@types/debug": "npm:^4.1.7" + "@types/lodash": "npm:^4.17.20" + debug: "npm:^4.3.4" + lodash: "npm:^4.17.21" + pony-cause: "npm:^2.1.10" + semver: "npm:^7.5.4" + uuid: "npm:^9.0.1" + checksum: 10/c4381b9e451a9616bde84ac659bc0d1848ef06b6e605f877bfa065b78c8ed5015706683ea88a3387de5eaeb3a50d1af9af0994f04f9e06258d992598fe2be3bf + languageName: node + linkType: hard + +"@metamask/utils@npm:^11.12.0": version: 11.12.0 resolution: "@metamask/utils@npm:11.12.0" dependencies: From 69d1c07054d058c7862ea4334e18c894b329db3c Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 09:48:56 +0100 Subject: [PATCH 2/8] docs: add changelog entries for the controller-owned bootstrap Co-Authored-By: Claude Fable 5 --- .../money-account-upgrade-controller/CHANGELOG.md | 11 +++++++++++ packages/money-account-utils/CHANGELOG.md | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index d9fe3406497..75bdbfd43f6 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -7,8 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add a public `sync()` method that re-evaluates the bootstrap gates against live state, for clients whose `isEnabled` hook reads client-only signals (onboarding, preferences) that should also re-trigger the bootstrap ([#10072](https://github.com/MetaMask/core/pull/10072)) +- Add `MissingMoneyAccountVaultConfigError`, reported through the `onBootstrapError` hook (once per controller lifetime) when the enable flag is on but `moneyAccountVaultConfig` is unserved or malformed ([#10072](https://github.com/MetaMask/core/pull/10072)) + ### Changed +- **BREAKING:** The controller now owns its bootstrap: it subscribes to `RemoteFeatureFlagController:stateChanged` and `KeyringController:stateChanged`, gates on an unlocked wallet with an HD keyring, parses the `moneyAccountVaultConfig` remote feature flag, and runs a serialized bootstrap that re-checks its gates across `await` points and re-runs when the vault config changes ([#10072](https://github.com/MetaMask/core/pull/10072)) + - `init()` is now the no-argument lifecycle entry point (subscribe and first sync), to be called once after all controllers and services the messenger reaches are constructed. The former `init({ chainId, boringVaultAddress })` config-arming routine is internal and reads the chain id and boring vault from the flag; clients that called it must remove that orchestration. + - The constructor requires a `hooks` option carrying the client-specific parts of the bootstrap: `isEnabled(remoteFeatureFlags)` (required — version-gated flag evaluation depends on the client version, and clients may add gates such as a basic-functionality toggle), plus optional `isEligible()` (an async pre-bootstrap gate, e.g. fail-closed geolocation), `ensureChainConfigured(vaultConfig)` (client-specific network adding), and `onBootstrapError(error)`. + - The messenger must now allow the `RemoteFeatureFlagController:getState` and `KeyringController:getState` actions and the `RemoteFeatureFlagController:stateChanged` and `KeyringController:stateChanged` events. +- **BREAKING:** `upgradeAccount()` now waits for an in-flight bootstrap instead of throwing, throws a new not-bootstrapped error message when no bootstrap has armed a config, and is disarmed when `isEnabled` flips off so it cannot run against a config armed while the feature was still enabled ([#10072](https://github.com/MetaMask/core/pull/10072)) +- Add `@metamask/money-account-utils` and `@metamask/remote-feature-flag-controller` as dependencies ([#10072](https://github.com/MetaMask/core/pull/10072)) - Bump `@metamask/authenticated-user-storage` from `^3.0.1` to `^3.0.2` ([#9972](https://github.com/MetaMask/core/pull/9972)) - Bump `@metamask/chomp-api-service` from `^4.0.0` to `^4.0.1` ([#9972](https://github.com/MetaMask/core/pull/9972)) - Bump `@metamask/utils` from `^11.11.0` to `^11.12.0` ([#10076](https://github.com/MetaMask/core/pull/10076)) diff --git a/packages/money-account-utils/CHANGELOG.md b/packages/money-account-utils/CHANGELOG.md index 5518297bf6b..a093f3e9024 100644 --- a/packages/money-account-utils/CHANGELOG.md +++ b/packages/money-account-utils/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add Money Account vault-config helpers, shared by the upgrade controller and clients ([#10072](https://github.com/MetaMask/core/pull/10072)) + - `getMoneyAccountVaultConfig` and `parseMoneyAccountVaultConfig` read and validate the `moneyAccountVaultConfig` remote feature flag into a `MoneyAccountVaultConfig` whose chain id and addresses are known-good `Hex` + - `areMoneyAccountVaultConfigsEqual` compares configs field by field so consumers keyed on the config (bootstraps, caches) can detect changes + - `MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME` names the flag + ### Changed - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.7.0` ([#9780](https://github.com/MetaMask/core/pull/9780), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823), [#9960](https://github.com/MetaMask/core/pull/9960), [#9969](https://github.com/MetaMask/core/pull/9969), [#10046](https://github.com/MetaMask/core/pull/10046)) From c323e30eab0172a0534f841317fc83328995a38d Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 09:57:07 +0100 Subject: [PATCH 3/8] chore: apply formatting and regenerate README content Co-Authored-By: Claude Fable 5 --- README.md | 2 ++ .../src/MoneyAccountUpgradeController.test.ts | 9 +++++++-- .../src/MoneyAccountUpgradeController.ts | 4 +++- packages/money-account-utils/src/vault-config.ts | 6 +++++- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 27f8d326fe2..4efea42c40f 100644 --- a/README.md +++ b/README.md @@ -451,7 +451,9 @@ linkStyle default opacity:0.5 money_account_upgrade_controller --> delegation_controller; money_account_upgrade_controller --> keyring_controller; money_account_upgrade_controller --> messenger; + money_account_upgrade_controller --> money_account_utils; money_account_upgrade_controller --> network_controller; + money_account_upgrade_controller --> remote_feature_flag_controller; money_account_utils --> transaction_controller; multichain_account_service --> accounts_controller; multichain_account_service --> base_controller; diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 91d268d50b6..7e758a639cf 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -446,7 +446,10 @@ describe('MoneyAccountUpgradeController', () => { await bootstrap(); - expect(order).toStrictEqual(['ensureChainConfigured', 'getServiceDetails']); + expect(order).toStrictEqual([ + 'ensureChainConfigured', + 'getServiceDetails', + ]); expect(mocks.ensureChainConfigured).toHaveBeenCalledWith(VAULT_CONFIG); }); @@ -802,7 +805,9 @@ describe('MoneyAccountUpgradeController', () => { await bootstrap(); expect(mocks.onBootstrapError).toHaveBeenCalledWith( - new Error(`Chain ${MOCK_CHAIN_ID} not found in service details response`), + new Error( + `Chain ${MOCK_CHAIN_ID} not found in service details response`, + ), ); }); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index d6ef5461814..1b273e14a03 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -194,7 +194,9 @@ export type MoneyAccountUpgradeControllerHooks = { * `NetworkController:findNetworkClientIdByChainId` consumers. Defaults to a * no-op. */ - ensureChainConfigured?: (vaultConfig: MoneyAccountVaultConfig) => Promise; + ensureChainConfigured?: ( + vaultConfig: MoneyAccountVaultConfig, + ) => Promise; /** * Called when a bootstrap run fails or cannot be scheduled. Receives a diff --git a/packages/money-account-utils/src/vault-config.ts b/packages/money-account-utils/src/vault-config.ts index 2b20f867807..6ae43b35d02 100644 --- a/packages/money-account-utils/src/vault-config.ts +++ b/packages/money-account-utils/src/vault-config.ts @@ -1,4 +1,8 @@ -import { isObject, isStrictHexString, isValidHexAddress } from '@metamask/utils'; +import { + isObject, + isStrictHexString, + isValidHexAddress, +} from '@metamask/utils'; import type { Hex } from '@metamask/utils'; /** From edddd766ed6ea1c62774d987c838142bdd93ceeb Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 10:20:13 +0100 Subject: [PATCH 4/8] fix: do not re-arm the upgrade config when disarmed mid-bootstrap A disarm (isEnabled flipping off) or a newer scheduled config during the CHOMP service-details call now supersedes the in-flight run instead of the run arming a config the controller had already dropped. Co-Authored-By: Claude Fable 5 --- .../src/MoneyAccountUpgradeController.test.ts | 22 +++++++ .../src/MoneyAccountUpgradeController.ts | 57 +++++++++++-------- .../src/errors.ts | 3 +- 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 7e758a639cf..2c8b421c3f3 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -916,6 +916,28 @@ describe('MoneyAccountUpgradeController', () => { ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); }); + it('does not re-arm when disarmed while the CHOMP call is in flight', async () => { + let resolveServiceDetails: (value?: unknown) => void = () => undefined; + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + mocks.getServiceDetails.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveServiceDetails = resolve; + }), + ); + await bootstrap(); + + config.isEnabled = false; + await triggerFlagChange(); + resolveServiceDetails(MOCK_SERVICE_DETAILS_RESPONSE); + await flushPromises(); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); + }); + it('re-bootstraps from scratch after being disarmed', async () => { const { config, mocks, bootstrap, triggerFlagChange } = setup(); await bootstrap(); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index 1b273e14a03..61294ffeee5 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -163,14 +163,18 @@ export type MoneyAccountUpgradeControllerMessenger = Messenger< >; /** - * The hooks a client supplies for the parts of the bootstrap that cannot be - * decided in this package. + * These hooks must be provided by the client - they provide functions that required to bootstrap + * the controller, which rely on client specific information. */ export type MoneyAccountUpgradeControllerHooks = { /** - * Whether the Money Account feature is enabled for this client. Called with - * the current remote feature flags on every sync and re-checked across the - * bootstrap's `await` points; it must re-read any client state it depends + * Whether the Money Account feature is enabled for this client. + * + * The controller will call the function with the current state of + * the remote feature flags. It gets caleld on every sync, and re-checked + * when the bootstrap `awaits`. + * + * The isEnabled function should re-read any client state it depends * on (e.g. a version-gated flag, a "basic functionality" toggle) rather * than caching it. Returning `false` after a successful bootstrap disarms * the controller: `upgradeAccount` refuses to run until a later sync @@ -182,15 +186,14 @@ export type MoneyAccountUpgradeControllerHooks = { * An asynchronous client gate checked once per bootstrap run, before any * network is added or external service is called — e.g. a fail-closed * geolocation check. A run skipped here is forgotten and retried on the - * next sync trigger. Defaults to always eligible. + * next sync trigger. If the function is not provided we assume the client is eligible. */ isEligible?: () => Promise; /** * Ensure the vault chain exists in the client's NetworkController before - * the bootstrap validates it. Adding a network is client-specific (featured - * network lists, enabled-network bookkeeping), so the controller only - * promises to have awaited this before calling + * the bootstrap validates it. Adding a network is client-specific, so + * the controller awaits this before calling * `NetworkController:findNetworkClientIdByChainId` consumers. Defaults to a * no-op. */ @@ -202,8 +205,7 @@ export type MoneyAccountUpgradeControllerHooks = { * Called when a bootstrap run fails or cannot be scheduled. Receives a * {@link MissingMoneyAccountVaultConfigError} (once per controller * lifetime) when the enable flag is on but `moneyAccountVaultConfig` is - * unserved or malformed. The controller never throws out of its - * subscriptions; this hook is the only failure signal. + * unserved or malformed. */ onBootstrapError?: (error: unknown) => void; }; @@ -212,10 +214,9 @@ export type MoneyAccountUpgradeControllerHooks = { * Controller that owns the Money Account upgrade sequence and its own * bootstrap. * - * After {@link MoneyAccountUpgradeController.init} is called (once, after all - * controllers are constructed), the controller watches + * After {@link MoneyAccountUpgradeController.init} is called, the controller watches * `RemoteFeatureFlagController` and `KeyringController` state and bootstraps - * itself when every gate is open: + * itself when: * * 1. the client's `isEnabled` hook returns `true` for the current flags, * 2. the wallet is unlocked with an HD keyring, @@ -223,12 +224,11 @@ export type MoneyAccountUpgradeControllerHooks = { * 4. the `moneyAccountVaultConfig` flag parses. * * The bootstrap awaits the client's `ensureChainConfigured` hook and then - * fetches CHOMP service details to arm the upgrade config. Gates 1–2 are - * re-checked across the `await` points so a lock or an `isEnabled` flip - * mid-bootstrap cannot still produce an external call; a skipped or failed - * run is forgotten so the next trigger retries it. The bootstrap re-runs - * whenever the vault config changes, and `isEnabled` going `false` disarms - * the controller entirely. + * fetches CHOMP service details to get the upgrade config. We recheck points + * 1–2 when awaiting in the bootstrap so a lock or an `isEnabled` will stop the process. + * + * The bootstrap re-runs whenever the vault config changes. + * `isEnabled` going `false` disables the controller. */ export class MoneyAccountUpgradeController extends BaseController< typeof controllerName, @@ -329,12 +329,12 @@ export class MoneyAccountUpgradeController extends BaseController< } /** - * Re-evaluate the bootstrap gates against live state and schedule a + * Re-evaluate the bootstrap checks against live state and schedule a * bootstrap run when they are open and the vault config is new. Runs on - * every feature-flag and keyring state change; clients with additional - * gates read inside their `isEnabled` hook (onboarding, preferences) - * should call this when those change too. Never throws: a failure is - * reported through `onBootstrapError` and retried on the next trigger. + * every feature-flag and keyring state change. + * + * If sync fails a failure is reported through `onBootstrapError` and retried + * on the next trigger. */ sync(): void { try { @@ -510,6 +510,13 @@ export class MoneyAccountUpgradeController extends BaseController< ); } + // A disarm (isEnabled flipping off) or a newer scheduled config during + // the CHOMP call supersedes this run: arming now would resurrect a config + // the controller just dropped, or briefly shadow the newer one. + if (this.#bootstrappedConfig !== vaultConfig) { + return; + } + this.#config = { chainId, delegateAddress: chain.autoDepositDelegate, diff --git a/packages/money-account-upgrade-controller/src/errors.ts b/packages/money-account-upgrade-controller/src/errors.ts index 8e648d2ea37..a1779905559 100644 --- a/packages/money-account-upgrade-controller/src/errors.ts +++ b/packages/money-account-upgrade-controller/src/errors.ts @@ -55,8 +55,7 @@ export class TerminalUpgradeError extends Error { /** * Error reported through the `onBootstrapError` hook when the Money Account * enable flag is on but the `moneyAccountVaultConfig` flag is unserved or - * malformed — a flag misconfiguration that silently disables upgrades. - * Reported once per controller lifetime. + * malformed. */ export class MissingMoneyAccountVaultConfigError extends Error { constructor() { From 4e0feb96595d645fc3ab577ba592dab385df1374 Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 13:44:44 +0100 Subject: [PATCH 5/8] fix: never run upgradeAccount against a superseded vault config Scheduling a bootstrap for a changed vault config now disarms the armed config, so upgradeAccount waits for the re-bootstrap (or refuses if it failed) instead of signing delegations against the old vault. The wait also follows runs chained onto the bootstrap while waiting, so a call that captured a superseded run no longer throws while its successor is still running. Co-Authored-By: Claude Fable 5 --- .../CHANGELOG.md | 2 +- .../src/MoneyAccountUpgradeController.test.ts | 74 +++++++++++++++++++ .../src/MoneyAccountUpgradeController.ts | 23 ++++-- 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index 75bdbfd43f6..da9eb1aa685 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -18,7 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `init()` is now the no-argument lifecycle entry point (subscribe and first sync), to be called once after all controllers and services the messenger reaches are constructed. The former `init({ chainId, boringVaultAddress })` config-arming routine is internal and reads the chain id and boring vault from the flag; clients that called it must remove that orchestration. - The constructor requires a `hooks` option carrying the client-specific parts of the bootstrap: `isEnabled(remoteFeatureFlags)` (required — version-gated flag evaluation depends on the client version, and clients may add gates such as a basic-functionality toggle), plus optional `isEligible()` (an async pre-bootstrap gate, e.g. fail-closed geolocation), `ensureChainConfigured(vaultConfig)` (client-specific network adding), and `onBootstrapError(error)`. - The messenger must now allow the `RemoteFeatureFlagController:getState` and `KeyringController:getState` actions and the `RemoteFeatureFlagController:stateChanged` and `KeyringController:stateChanged` events. -- **BREAKING:** `upgradeAccount()` now waits for an in-flight bootstrap instead of throwing, throws a new not-bootstrapped error message when no bootstrap has armed a config, and is disarmed when `isEnabled` flips off so it cannot run against a config armed while the feature was still enabled ([#10072](https://github.com/MetaMask/core/pull/10072)) +- **BREAKING:** `upgradeAccount()` now waits for the in-flight bootstrap chain to settle (including runs scheduled while waiting) instead of throwing, and throws a new not-bootstrapped error message when no bootstrap has armed a config. Scheduling a bootstrap for a changed vault config — or `isEnabled` flipping off — disarms the previous config, so an upgrade can never sign against a superseded vault, including after a failed re-bootstrap ([#10072](https://github.com/MetaMask/core/pull/10072)) - Add `@metamask/money-account-utils` and `@metamask/remote-feature-flag-controller` as dependencies ([#10072](https://github.com/MetaMask/core/pull/10072)) - Bump `@metamask/authenticated-user-storage` from `^3.0.1` to `^3.0.2` ([#9972](https://github.com/MetaMask/core/pull/9972)) - Bump `@metamask/chomp-api-service` from `^4.0.0` to `^4.0.1` ([#9972](https://github.com/MetaMask/core/pull/9972)) diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 2c8b421c3f3..b0aff38cfe3 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -902,6 +902,80 @@ describe('MoneyAccountUpgradeController', () => { expect(mocks.signPersonalMessage).toHaveBeenCalled(); }); + it('waits for an in-flight re-bootstrap instead of using the superseded config', async () => { + let resolveSecond: (value?: unknown) => void = () => undefined; + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + await bootstrap(); + mocks.getServiceDetails.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveSecond = resolve; + }), + ); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + + const upgrade = controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + await flushPromises(); + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + + resolveSecond(MOCK_SERVICE_DETAILS_RESPONSE); + + expect(await upgrade).toBeUndefined(); + expect(mocks.signPersonalMessage).toHaveBeenCalled(); + }); + + it('waits for a run chained on while it was already waiting', async () => { + let resolveFirst: (value?: unknown) => void = () => undefined; + const { controller, config, mocks, triggerFlagChange } = setup(); + mocks.getServiceDetails.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve; + }), + ); + controller.init(); + await flushPromises(); + + // The upgrade captures the first run's promise; a config change then + // chains a second run, which supersedes the first. + const upgrade = controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + resolveFirst(MOCK_SERVICE_DETAILS_RESPONSE); + + expect(await upgrade).toBeUndefined(); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + + it('throws instead of using the superseded config when a re-bootstrap fails', async () => { + const { + controller, + config, + mocks, + bootstrap, + triggerFlagChange, + triggerKeyringChange, + } = setup(); + await bootstrap(); + mocks.getServiceDetails.mockRejectedValueOnce(new Error('CHOMP outage')); + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('MoneyAccountUpgradeController is not bootstrapped'); + + // The next trigger retries the bootstrap; once it succeeds the + // upgrade runs against the new config. + await triggerKeyringChange(); + + expect( + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).toBeUndefined(); + }); + it('throws after the isEnabled hook flips off and a sync disarms the controller', async () => { const { controller, config, mocks, bootstrap, triggerFlagChange } = setup(); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index 61294ffeee5..6c22664c4fc 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -407,6 +407,12 @@ export class MoneyAccountUpgradeController extends BaseController< #scheduleBootstrap(vaultConfig: MoneyAccountVaultConfig): void { this.#bootstrappedConfig = vaultConfig; + // Scheduling means the served vault config no longer matches whatever is + // armed, so disarm now: until this run succeeds, `upgradeAccount` must + // wait for it (or refuse if it fails) rather than sign delegations + // against the superseded vault. + this.#config = undefined; + const run = async (): Promise => { // The gates were checked when this run was scheduled, but it may start // much later, chained behind an in-flight bootstrap. Eligibility comes @@ -544,15 +550,22 @@ export class MoneyAccountUpgradeController extends BaseController< * active config no longer matches the recorded fingerprint, the sequence * re-runs. * - * A call that arrives while the bootstrap is still in flight waits for it - * rather than failing; it only throws when no bootstrap has armed a config - * (feature disabled, wallet locked, or the last bootstrap failed). + * A call that arrives while the bootstrap chain is still in flight — + * including runs scheduled while waiting — waits for it to settle rather + * than failing, so the upgrade always runs against the latest armed + * config. Scheduling a bootstrap for a changed vault config disarms the + * previous one, so it only throws when no bootstrap has armed a config: + * feature disabled, wallet locked, or the last bootstrap failed. * * @param address - The Money Account address to upgrade. */ async upgradeAccount(address: Hex): Promise { - if (!this.#config && this.#bootstrap) { - await this.#bootstrap.catch(() => undefined); + let bootstrap = this.#bootstrap; + while (bootstrap) { + await bootstrap.catch(() => undefined); + // A newer run may have been chained on while we waited; wait for that + // one too, until the chain settles. + bootstrap = this.#bootstrap === bootstrap ? undefined : this.#bootstrap; } if (!this.#config) { throw new Error( From 766f72701500b9dca15c3380cba94f7d91b46318 Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 13:58:38 +0100 Subject: [PATCH 6/8] chore: regenerate action types --- .../MoneyAccountUpgradeController-method-action-types.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts index 53a75996643..c0222989087 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts @@ -19,9 +19,12 @@ import type { MoneyAccountUpgradeController } from './MoneyAccountUpgradeControl * active config no longer matches the recorded fingerprint, the sequence * re-runs. * - * A call that arrives while the bootstrap is still in flight waits for it - * rather than failing; it only throws when no bootstrap has armed a config - * (feature disabled, wallet locked, or the last bootstrap failed). + * A call that arrives while the bootstrap chain is still in flight — + * including runs scheduled while waiting — waits for it to settle rather + * than failing, so the upgrade always runs against the latest armed + * config. Scheduling a bootstrap for a changed vault config disarms the + * previous one, so it only throws when no bootstrap has armed a config: + * feature disabled, wallet locked, or the last bootstrap failed. * * @param address - The Money Account address to upgrade. */ From 6ec257f43b34572c9078b23da7d4888028a052ba Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 14:38:41 +0100 Subject: [PATCH 7/8] fix: harden bootstrap and upgrade against mid-flight state changes - Make `underlyingToken` optional in the shared vault-config parser so flags that predate the field still parse, matching the balance service - Re-check the armed config before every upgrade step and abort if it is disarmed or superseded mid-sequence - Refuse `upgradeAccount` while the wallet is locked - Contain a throwing `onBootstrapError` hook so the failed bootstrap is still retried and nothing escapes `init()` or `sync()` Co-Authored-By: Claude Fable 5.1 --- .../CHANGELOG.md | 4 +- ...ntUpgradeController-method-action-types.ts | 8 +- .../src/MoneyAccountUpgradeController.test.ts | 145 +++++++++++++++++- .../src/MoneyAccountUpgradeController.ts | 38 ++++- packages/money-account-utils/CHANGELOG.md | 2 +- .../src/vault-config.test.ts | 49 +++++- .../money-account-utils/src/vault-config.ts | 25 +-- 7 files changed, 247 insertions(+), 24 deletions(-) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index da9eb1aa685..bfeb007c927 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -18,7 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `init()` is now the no-argument lifecycle entry point (subscribe and first sync), to be called once after all controllers and services the messenger reaches are constructed. The former `init({ chainId, boringVaultAddress })` config-arming routine is internal and reads the chain id and boring vault from the flag; clients that called it must remove that orchestration. - The constructor requires a `hooks` option carrying the client-specific parts of the bootstrap: `isEnabled(remoteFeatureFlags)` (required — version-gated flag evaluation depends on the client version, and clients may add gates such as a basic-functionality toggle), plus optional `isEligible()` (an async pre-bootstrap gate, e.g. fail-closed geolocation), `ensureChainConfigured(vaultConfig)` (client-specific network adding), and `onBootstrapError(error)`. - The messenger must now allow the `RemoteFeatureFlagController:getState` and `KeyringController:getState` actions and the `RemoteFeatureFlagController:stateChanged` and `KeyringController:stateChanged` events. -- **BREAKING:** `upgradeAccount()` now waits for the in-flight bootstrap chain to settle (including runs scheduled while waiting) instead of throwing, and throws a new not-bootstrapped error message when no bootstrap has armed a config. Scheduling a bootstrap for a changed vault config — or `isEnabled` flipping off — disarms the previous config, so an upgrade can never sign against a superseded vault, including after a failed re-bootstrap ([#10072](https://github.com/MetaMask/core/pull/10072)) +- **BREAKING:** `upgradeAccount()` now waits for the in-flight bootstrap chain to settle (including runs scheduled while waiting) instead of throwing, and throws a new not-bootstrapped error message when no bootstrap has armed a config or the wallet is locked. Scheduling a bootstrap for a changed vault config — or `isEnabled` flipping off — disarms the previous config, so an upgrade can never sign against a superseded vault, including after a failed re-bootstrap ([#10072](https://github.com/MetaMask/core/pull/10072)) + - The armed config is re-checked before every step; if it is disarmed or superseded while the sequence is running, `upgradeAccount()` throws an upgrade-aborted error before the next step signs anything and records nothing + - An `onBootstrapError` hook that throws is contained: the failed bootstrap is still forgotten and retried on the next trigger, and the throw does not escape `init()` or `sync()` - Add `@metamask/money-account-utils` and `@metamask/remote-feature-flag-controller` as dependencies ([#10072](https://github.com/MetaMask/core/pull/10072)) - Bump `@metamask/authenticated-user-storage` from `^3.0.1` to `^3.0.2` ([#9972](https://github.com/MetaMask/core/pull/9972)) - Bump `@metamask/chomp-api-service` from `^4.0.0` to `^4.0.1` ([#9972](https://github.com/MetaMask/core/pull/9972)) diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts index c0222989087..c302b2fe05e 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts @@ -23,8 +23,12 @@ import type { MoneyAccountUpgradeController } from './MoneyAccountUpgradeControl * including runs scheduled while waiting — waits for it to settle rather * than failing, so the upgrade always runs against the latest armed * config. Scheduling a bootstrap for a changed vault config disarms the - * previous one, so it only throws when no bootstrap has armed a config: - * feature disabled, wallet locked, or the last bootstrap failed. + * previous one, so it throws when no bootstrap has armed a config (feature + * disabled or the last bootstrap failed) or when the wallet is locked. + * + * The armed config is re-checked before every step: if a sync disarms or + * supersedes it while the sequence is running, the sequence aborts before + * the next step signs anything, and nothing is recorded. * * @param address - The Money Account address to upgrade. */ diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index b0aff38cfe3..2ff79762030 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -456,11 +456,14 @@ describe('MoneyAccountUpgradeController', () => { it('is idempotent: a second init() does not re-subscribe or re-bootstrap', async () => { const { controller, mocks, bootstrap, triggerFlagChange } = setup(); await bootstrap(); + const syncsAfterFirstInit = mocks.isEnabled.mock.calls.length; controller.init(); await flushPromises(); - await triggerFlagChange(); + expect(mocks.isEnabled).toHaveBeenCalledTimes(syncsAfterFirstInit); + await triggerFlagChange(); + expect(mocks.isEnabled).toHaveBeenCalledTimes(syncsAfterFirstInit + 1); expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); }); @@ -609,6 +612,45 @@ describe('MoneyAccountUpgradeController', () => { expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); }); + it('skips a chained run entirely when the wallet locks before it starts', async () => { + let resolveServiceDetails: (value?: unknown) => void = () => undefined; + const { + config, + mocks, + bootstrap, + triggerFlagChange, + triggerKeyringChange, + } = setup(); + mocks.getServiceDetails + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveServiceDetails = resolve; + }), + ) + .mockResolvedValue(MOCK_SERVICE_DETAILS_RESPONSE); + await bootstrap(); + + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + config.isUnlocked = false; + await triggerKeyringChange(); + resolveServiceDetails(MOCK_SERVICE_DETAILS_RESPONSE); + await flushPromises(); + + expect(mocks.isEligible).toHaveBeenCalledTimes(1); + expect(mocks.ensureChainConfigured).toHaveBeenCalledTimes(1); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + + config.isUnlocked = true; + await triggerKeyringChange(); + + expect(mocks.ensureChainConfigured).toHaveBeenLastCalledWith( + CHANGED_VAULT_CONFIG, + ); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + it('reports a missing vault config through onBootstrapError only once', async () => { const { mocks, bootstrap, triggerFlagChange } = setup({ vaultConfig: null, @@ -777,6 +819,34 @@ describe('MoneyAccountUpgradeController', () => { expect(() => controller.sync()).not.toThrow(); expect(mocks.onBootstrapError).toHaveBeenCalledWith(failure); }); + + it('still retries a failed bootstrap when the onBootstrapError hook throws', async () => { + const { mocks, bootstrap, triggerKeyringChange } = setup(); + mocks.getServiceDetails + .mockRejectedValueOnce(new Error('CHOMP outage')) + .mockResolvedValue(MOCK_SERVICE_DETAILS_RESPONSE); + mocks.onBootstrapError.mockImplementationOnce(() => { + throw new Error('reporter is broken'); + }); + + await bootstrap(); + await triggerKeyringChange(); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + }); + + it('does not let a throwing onBootstrapError hook escape sync', async () => { + const { controller, mocks, bootstrap } = setup(); + await bootstrap(); + mocks.isEnabled.mockImplementationOnce(() => { + throw new Error('handler not registered'); + }); + mocks.onBootstrapError.mockImplementationOnce(() => { + throw new Error('reporter is broken'); + }); + + expect(() => controller.sync()).not.toThrow(); + }); }); describe('bootstrap failures', () => { @@ -1024,6 +1094,79 @@ describe('MoneyAccountUpgradeController', () => { expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); }); + it('throws without running any step while the wallet is locked, and resumes on unlock without re-bootstrapping', async () => { + const { controller, config, mocks, bootstrap, triggerKeyringChange } = + setup(); + await bootstrap(); + + config.isUnlocked = false; + await triggerKeyringChange(); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow( + 'MoneyAccountUpgradeController is not bootstrapped: upgradeAccount() requires the feature flag on, the wallet unlocked, and a successful bootstrap', + ); + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + + config.isUnlocked = true; + await triggerKeyringChange(); + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(1); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toBeDefined(); + }); + + it('aborts the sequence without signing further when disarmed mid-run', async () => { + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + await bootstrap(); + mocks.signPersonalMessage.mockImplementationOnce(async () => { + config.isEnabled = false; + await triggerFlagChange(); + return '0xdeadbeef'; + }); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow( + 'MoneyAccountUpgradeController upgrade aborted: the upgrade config was disarmed or superseded while the sequence was running', + ); + + expect(mocks.signEip7702Authorization).not.toHaveBeenCalled(); + expect(mocks.signDelegation).not.toHaveBeenCalled(); + expect(controller.state.upgradedAccounts).toStrictEqual({}); + }); + + it('aborts the sequence when the vault config changes mid-run, then upgrades against the new config', async () => { + const { controller, config, mocks, bootstrap, triggerFlagChange } = + setup(); + await bootstrap(); + mocks.signEip7702Authorization.mockImplementationOnce(async () => { + config.vaultConfig = CHANGED_VAULT_CONFIG; + await triggerFlagChange(); + return `0x${'1'.repeat(64)}${'2'.repeat(64)}1c`; + }); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('MoneyAccountUpgradeController upgrade aborted'); + + expect(mocks.signDelegation).not.toHaveBeenCalled(); + expect(controller.state.upgradedAccounts).toStrictEqual({}); + expect(mocks.getServiceDetails).toHaveBeenCalledTimes(2); + + clearMockCalls(mocks); + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect(mocks.signDelegation).toHaveBeenCalled(); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toBeDefined(); + }); + it('runs each step against the deployment-derived contract addresses', async () => { const { controller, mocks, bootstrap } = setup(); await bootstrap(); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index 6c22664c4fc..5042b51d195 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -370,7 +370,24 @@ export class MoneyAccountUpgradeController extends BaseController< this.#scheduleBootstrap(vaultConfig); } catch (error) { + this.#reportBootstrapError(error); + } + } + + /** + * Hand a bootstrap failure to the client's `onBootstrapError` hook. A hook + * that throws must not break the controller: from a subscription it would + * surface as an unhandled rejection, and from `init()` it would abort the + * caller's startup. + * + * @param error - The failure to report. + */ + #reportBootstrapError(error: unknown): void { + try { this.#onBootstrapError(error); + } catch { + // The hook is the client's error sink; there is nowhere else to send its + // own failure without recursing into it. } } @@ -441,8 +458,8 @@ export class MoneyAccountUpgradeController extends BaseController< this.#bootstrap = bootstrap; bootstrap.catch((error) => { - this.#onBootstrapError(error); this.#forget(vaultConfig); + this.#reportBootstrapError(error); }); } @@ -468,7 +485,7 @@ export class MoneyAccountUpgradeController extends BaseController< #reportMissingConfig(): void { if (!this.#missingConfigReported) { this.#missingConfigReported = true; - this.#onBootstrapError(new MissingMoneyAccountVaultConfigError()); + this.#reportBootstrapError(new MissingMoneyAccountVaultConfigError()); } } @@ -554,8 +571,12 @@ export class MoneyAccountUpgradeController extends BaseController< * including runs scheduled while waiting — waits for it to settle rather * than failing, so the upgrade always runs against the latest armed * config. Scheduling a bootstrap for a changed vault config disarms the - * previous one, so it only throws when no bootstrap has armed a config: - * feature disabled, wallet locked, or the last bootstrap failed. + * previous one, so it throws when no bootstrap has armed a config (feature + * disabled or the last bootstrap failed) or when the wallet is locked. + * + * The armed config is re-checked before every step: if a sync disarms or + * supersedes it while the sequence is running, the sequence aborts before + * the next step signs anything, and nothing is recorded. * * @param address - The Money Account address to upgrade. */ @@ -567,7 +588,9 @@ export class MoneyAccountUpgradeController extends BaseController< // one too, until the chain settles. bootstrap = this.#bootstrap === bootstrap ? undefined : this.#bootstrap; } - if (!this.#config) { + // A lock does not disarm the config (unlocking must not cost a CHOMP + // re-fetch), so the wallet has to be checked here as well. + if (!this.#config || !this.#areGatesOpen()) { throw new Error( 'MoneyAccountUpgradeController is not bootstrapped: upgradeAccount() requires the feature flag on, the wallet unlocked, and a successful bootstrap', ); @@ -584,6 +607,11 @@ export class MoneyAccountUpgradeController extends BaseController< } for (const step of this.#steps) { + if (this.#config !== config) { + throw new Error( + 'MoneyAccountUpgradeController upgrade aborted: the upgrade config was disarmed or superseded while the sequence was running', + ); + } try { await step.run({ messenger: this.messenger, diff --git a/packages/money-account-utils/CHANGELOG.md b/packages/money-account-utils/CHANGELOG.md index a093f3e9024..ec684bab192 100644 --- a/packages/money-account-utils/CHANGELOG.md +++ b/packages/money-account-utils/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add Money Account vault-config helpers, shared by the upgrade controller and clients ([#10072](https://github.com/MetaMask/core/pull/10072)) - - `getMoneyAccountVaultConfig` and `parseMoneyAccountVaultConfig` read and validate the `moneyAccountVaultConfig` remote feature flag into a `MoneyAccountVaultConfig` whose chain id and addresses are known-good `Hex` + - `getMoneyAccountVaultConfig` and `parseMoneyAccountVaultConfig` read and validate the `moneyAccountVaultConfig` remote feature flag into a `MoneyAccountVaultConfig` whose chain id and addresses are known-good `Hex`; `underlyingToken` is optional so flags deployed before that field existed still parse - `areMoneyAccountVaultConfigsEqual` compares configs field by field so consumers keyed on the config (bootstraps, caches) can detect changes - `MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME` names the flag diff --git a/packages/money-account-utils/src/vault-config.test.ts b/packages/money-account-utils/src/vault-config.test.ts index f3769a9189d..32bc6a63659 100644 --- a/packages/money-account-utils/src/vault-config.test.ts +++ b/packages/money-account-utils/src/vault-config.test.ts @@ -14,14 +14,15 @@ const VALID_CONFIG = { underlyingToken: '0xacA92E438df0B2401fF60dA7E4337B687a2435DA', } as const; -const ADDRESS_KEYS = [ +const REQUIRED_ADDRESS_KEYS = [ 'boringVault', 'tellerAddress', 'accountantAddress', 'lensAddress', - 'underlyingToken', ] as const; +const ADDRESS_KEYS = [...REQUIRED_ADDRESS_KEYS, 'underlyingToken'] as const; + const INVALID_CHAIN_IDS: [string, unknown][] = [ ['is not prefixed', '8f'], ['has non-hex digits', '0xmonad'], @@ -92,7 +93,7 @@ describe('parseMoneyAccountVaultConfig', () => { }); } - for (const key of ADDRESS_KEYS) { + for (const key of REQUIRED_ADDRESS_KEYS) { for (const [description, value] of INVALID_ADDRESSES) { it(`rejects a config whose ${key} ${description}`, () => { expect( @@ -102,6 +103,30 @@ describe('parseMoneyAccountVaultConfig', () => { } } + // Flags deployed before `underlyingToken` existed must still parse, matching + // the balance service's schema for the same flag. + it('accepts a config without an underlyingToken', () => { + const { underlyingToken: _omitted, ...withoutUnderlyingToken } = + VALID_CONFIG; + + expect(parseMoneyAccountVaultConfig(withoutUnderlyingToken)).toStrictEqual( + withoutUnderlyingToken, + ); + }); + + for (const [description, value] of INVALID_ADDRESSES.filter( + ([, invalid]) => invalid !== undefined, + )) { + it(`rejects a config whose underlyingToken ${description}`, () => { + expect( + parseMoneyAccountVaultConfig({ + ...VALID_CONFIG, + underlyingToken: value, + }), + ).toBeUndefined(); + }); + } + for (const [description, raw] of NON_OBJECT_FLAGS) { it(`rejects a flag that is ${description}`, () => { expect(parseMoneyAccountVaultConfig(raw)).toBeUndefined(); @@ -152,4 +177,22 @@ describe('areMoneyAccountVaultConfigsEqual', () => { ).toBe(false); }); } + + it('treats configs that both omit the underlyingToken as equal', () => { + const { underlyingToken: _omitted, ...withoutUnderlyingToken } = config; + + expect( + areMoneyAccountVaultConfigsEqual(withoutUnderlyingToken, { + ...withoutUnderlyingToken, + }), + ).toBe(true); + }); + + it('treats a config that gains an underlyingToken as changed', () => { + const { underlyingToken: _omitted, ...withoutUnderlyingToken } = config; + + expect( + areMoneyAccountVaultConfigsEqual(withoutUnderlyingToken, config), + ).toBe(false); + }); }); diff --git a/packages/money-account-utils/src/vault-config.ts b/packages/money-account-utils/src/vault-config.ts index 6ae43b35d02..32f99865a64 100644 --- a/packages/money-account-utils/src/vault-config.ts +++ b/packages/money-account-utils/src/vault-config.ts @@ -15,6 +15,10 @@ export const MONEY_ACCOUNT_VAULT_CONFIG_FLAG_NAME = 'moneyAccountVaultConfig'; /** * The Money Account vault contracts served via remote feature flags, with the * chain id and every address validated as known-good `Hex`. + * + * `underlyingToken` is optional because flags deployed before the field + * existed must still validate, matching the balance service's schema for the + * same flag. */ export type MoneyAccountVaultConfig = { chainId: Hex; @@ -22,7 +26,7 @@ export type MoneyAccountVaultConfig = { tellerAddress: Hex; accountantAddress: Hex; lensAddress: Hex; - underlyingToken: Hex; + underlyingToken?: Hex; }; /** @@ -63,26 +67,25 @@ export const parseMoneyAccountVaultConfig = ( const tellerAddress = parseAddress(raw.tellerAddress); const accountantAddress = parseAddress(raw.accountantAddress); const lensAddress = parseAddress(raw.lensAddress); - const underlyingToken = parseAddress(raw.underlyingToken); - if ( - !boringVault || - !tellerAddress || - !accountantAddress || - !lensAddress || - !underlyingToken - ) { + if (!boringVault || !tellerAddress || !accountantAddress || !lensAddress) { return undefined; } - return { + const config: MoneyAccountVaultConfig = { chainId, boringVault, tellerAddress, accountantAddress, lensAddress, - underlyingToken, }; + + if (raw.underlyingToken === undefined) { + return config; + } + + const underlyingToken = parseAddress(raw.underlyingToken); + return underlyingToken ? { ...config, underlyingToken } : undefined; }; /** From ad38c6403fd54ef0609fcc7667dfea52497ef67a Mon Sep 17 00:00:00 2001 From: John Whiles Date: Wed, 2 Sep 2026 15:21:04 +0100 Subject: [PATCH 8/8] dedupe yarn.lock file --- yarn.lock | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/yarn.lock b/yarn.lock index 4d85d17a04a..16331b66344 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9304,26 +9304,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/utils@npm:^11.0.1, @metamask/utils@npm:^11.10.0, @metamask/utils@npm:^11.11.0, @metamask/utils@npm:^11.4.0, @metamask/utils@npm:^11.4.2, @metamask/utils@npm:^11.8.1, @metamask/utils@npm:^11.9.0": - version: 11.11.0 - resolution: "@metamask/utils@npm:11.11.0" - dependencies: - "@ethereumjs/tx": "npm:^4.2.0" - "@metamask/superstruct": "npm:^3.1.0" - "@noble/hashes": "npm:^1.3.1" - "@scure/base": "npm:^1.1.3" - "@types/debug": "npm:^4.1.7" - "@types/lodash": "npm:^4.17.20" - debug: "npm:^4.3.4" - lodash: "npm:^4.17.21" - pony-cause: "npm:^2.1.10" - semver: "npm:^7.5.4" - uuid: "npm:^9.0.1" - checksum: 10/c4381b9e451a9616bde84ac659bc0d1848ef06b6e605f877bfa065b78c8ed5015706683ea88a3387de5eaeb3a50d1af9af0994f04f9e06258d992598fe2be3bf - languageName: node - linkType: hard - -"@metamask/utils@npm:^11.12.0": +"@metamask/utils@npm:^11.0.1, @metamask/utils@npm:^11.10.0, @metamask/utils@npm:^11.11.0, @metamask/utils@npm:^11.12.0, @metamask/utils@npm:^11.4.0, @metamask/utils@npm:^11.4.2, @metamask/utils@npm:^11.8.1, @metamask/utils@npm:^11.9.0": version: 11.12.0 resolution: "@metamask/utils@npm:11.12.0" dependencies: