From a6f4683e1521dd85ff07ff21ed385a7a5e4af0fb Mon Sep 17 00:00:00 2001 From: John Whiles Date: Tue, 14 Jul 2026 11:25:17 +0100 Subject: [PATCH 1/3] feat(money-account-upgrade-controller): persist upgrade status and add retry Track fully upgraded accounts in persisted controller state, keyed by lowercased address and fingerprinted against the active upgrade config, so upgradeAccount skips the step sequence once an account is recorded as upgraded and re-runs it when the config changes. Add upgradeAccountWithRetry, retrying failed attempts with capped exponential backoff (10s/20s/40s/60s, 5 attempts by default) and AbortSignal cancellation. Failures that cannot resolve by retrying (account delegated to a third-party EIP-7702 implementation, or unexpected on-chain code) are marked terminal via TerminalUpgradeError and are not retried. Co-Authored-By: Claude Fable 5 --- .../CHANGELOG.md | 10 + ...ntUpgradeController-method-action-types.ts | 30 +- .../src/MoneyAccountUpgradeController.test.ts | 418 +++++++++++++++++- .../src/MoneyAccountUpgradeController.ts | 208 ++++++++- .../src/errors.test.ts | 83 ++++ .../src/errors.ts | 48 ++ .../src/index.ts | 13 +- .../src/steps/eip-7702-authorization.test.ts | 14 + .../src/steps/eip-7702-authorization.ts | 5 +- 9 files changed, 816 insertions(+), 13 deletions(-) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index 57dc368b3ce..10494cc62bb 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -7,12 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add persisted state tracking fully upgraded accounts ([#9500](https://github.com/MetaMask/core/pull/9500)) + - `MoneyAccountUpgradeControllerState` now contains `upgradedAccounts`, keyed by lowercased account address. Each entry records when the upgrade sequence completed and a fingerprint of the config it completed under (see new `MoneyAccountUpgradeStatus` type). + - The constructor now accepts an optional `state` option, merged with the defaults; add `getDefaultMoneyAccountUpgradeControllerState` to construct those defaults. +- Add `upgradeAccountWithRetry` method and matching `MoneyAccountUpgradeController:upgradeAccountWithRetry` messenger action ([#9500](https://github.com/MetaMask/core/pull/9500)) + - Retries failed `upgradeAccount` attempts with capped exponential backoff (10s, 20s, 40s, then 60s between attempts; 5 attempts by default). Terminal failures and non-step errors are rethrown without retrying. Accepts an `AbortSignal` to cancel waiting between attempts. +- Add `TerminalUpgradeError` and `isTerminalMoneyAccountUpgradeError`, and a `terminal` property on `MoneyAccountUpgradeStepError`, marking failures that cannot resolve by retrying — currently an account delegated to a third-party EIP-7702 implementation, or an account with unexpected on-chain code ([#9500](https://github.com/MetaMask/core/pull/9500)) + ### Changed - **BREAKING:** The `associate-address` upgrade step now checks the profile's existing address associations via `ChompApiService:getAssociatedAddresses` before signing, and reports `already-done` without signing or submitting anything when the address is already associated ([#9387](https://github.com/MetaMask/core/pull/9387)) - `MoneyAccountUpgradeControllerMessenger` consumers must grant the `ChompApiService:getAssociatedAddresses` action alongside the previously required actions, and must provide a `@metamask/chomp-api-service` version that registers it (`>=4.0.0`). - The lookup is an optimization: if it fails, the step falls through to the previous sign-and-submit behavior. - A 409 conflict from the association request is disambiguated by re-fetching the associations, so a same-profile create race reports `already-done` instead of failing the upgrade; a genuine conflict (address associated with a different profile) still fails the step. +- `upgradeAccount` now skips the step sequence entirely when the account is recorded in state as upgraded under the active config fingerprint, and records the account after a successful run. If the chain, CHOMP contract addresses, or Delegation Framework version change, the fingerprint no longer matches and the sequence re-runs ([#9500](https://github.com/MetaMask/core/pull/9500)) - Bump `@metamask/messenger` from `^1.2.0` to `^2.0.0` ([#9392](https://github.com/MetaMask/core/pull/9392)) - Bump `@metamask/authenticated-user-storage` from `^3.0.0` to `^3.0.1` ([#9458](https://github.com/MetaMask/core/pull/9458)) 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 23d5c723629..b40cbd0f9d8 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 @@ -13,6 +13,12 @@ import type { MoneyAccountUpgradeController } from './MoneyAccountUpgradeControl * {@link MoneyAccountUpgradeStepError} that records which step failed (the * original error is preserved as `cause`). * + * A run that completes is recorded in state (keyed by lowercased address, + * fingerprinted against the active config); subsequent calls for a + * recorded account return immediately without running any steps. If the + * active config no longer matches the recorded fingerprint, the sequence + * re-runs. + * * @param address - The Money Account address to upgrade. */ export type MoneyAccountUpgradeControllerUpgradeAccountAction = { @@ -20,8 +26,30 @@ export type MoneyAccountUpgradeControllerUpgradeAccountAction = { handler: MoneyAccountUpgradeController['upgradeAccount']; }; +/** + * Runs the upgrade sequence via + * {@link MoneyAccountUpgradeController.upgradeAccount}, retrying failed + * attempts with capped exponential backoff (10s, 20s, 40s, then 60s + * between attempts). Rethrows the last error without further attempts when + * the failure is terminal (see `isTerminalMoneyAccountUpgradeError`), when + * it is not a step failure at all, or when `maxAttempts` is exhausted. + * + * @param address - The Money Account address to upgrade. + * @param options - Retry options. + * @param options.signal - Aborts waiting between attempts and prevents + * further attempts. An aborted run rejects with an error stating the retry + * was aborted. + * @param options.maxAttempts - Maximum number of attempts, including the + * first. Defaults to 5. + */ +export type MoneyAccountUpgradeControllerUpgradeAccountWithRetryAction = { + type: `MoneyAccountUpgradeController:upgradeAccountWithRetry`; + handler: MoneyAccountUpgradeController['upgradeAccountWithRetry']; +}; + /** * Union of all MoneyAccountUpgradeController action types. */ export type MoneyAccountUpgradeControllerMethodActions = - MoneyAccountUpgradeControllerUpgradeAccountAction; + | MoneyAccountUpgradeControllerUpgradeAccountAction + | MoneyAccountUpgradeControllerUpgradeAccountWithRetryAction; diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 7a08f3a60fa..125ca5c56c7 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -10,11 +10,14 @@ import type { Hex } from '@metamask/utils'; import type { MoneyAccountUpgradeControllerMessenger, + MoneyAccountUpgradeControllerState, MoneyAccountUpgradeStepError, } from '.'; import { MoneyAccountUpgradeController, + getDefaultMoneyAccountUpgradeControllerState, isMoneyAccountUpgradeStepError, + isTerminalMoneyAccountUpgradeError, } from '.'; const MOCK_CHAIN_ID = '0x1' as Hex; // mainnet, supported in delegation-deployments@1.3.0 @@ -84,7 +87,11 @@ type Mocks = { createIntents: jest.Mock; }; -function setup(): { +function setup({ + state, +}: { + state?: Partial; +} = {}): { controller: MoneyAccountUpgradeController; rootMessenger: RootMessenger; messenger: MoneyAccountUpgradeControllerMessenger; @@ -230,11 +237,25 @@ function setup(): { const controller = new MoneyAccountUpgradeController({ messenger, + state, }); return { controller, rootMessenger, messenger, mocks }; } +/** + * Resets the call history of every mock in the bag, preserving their + * configured implementations. Useful for asserting that a later + * `upgradeAccount` call performs no work. + * + * @param mocks - The mocks bag from `setup`. + */ +function clearMockCalls(mocks: Mocks): void { + for (const mock of Object.values(mocks)) { + mock.mockClear(); + } +} + describe('MoneyAccountUpgradeController', () => { describe('constructor', () => { it('does not make async init calls when constructed', () => { @@ -242,6 +263,27 @@ describe('MoneyAccountUpgradeController', () => { expect(mocks.getServiceDetails).not.toHaveBeenCalled(); }); + + it('starts with the default empty state', () => { + const { controller } = setup(); + + expect(controller.state).toStrictEqual( + getDefaultMoneyAccountUpgradeControllerState(), + ); + expect(controller.state.upgradedAccounts).toStrictEqual({}); + }); + + it('merges provided partial state with the defaults', () => { + const status = { configFingerprint: 'fingerprint', completedAt: 123 }; + + const { controller } = setup({ + state: { upgradedAccounts: { [MOCK_ACCOUNT_ADDRESS]: status } }, + }); + + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toStrictEqual(status); + }); }); describe('init', () => { @@ -516,5 +558,379 @@ describe('MoneyAccountUpgradeController', () => { 'Money Account upgrade failed at step "associate-address": plain string failure', ); }); + + 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, + }); + // EIP-7702 delegation code pointing at a third-party impl. + mocks.providerRequest.mockImplementation( + async ({ method }: { method: string }) => { + if (method === 'eth_getCode') { + return `0xef0100${'9'.repeat(40)}`; + } + return '0x0'; + }, + ); + + const error = await controller + .upgradeAccount(MOCK_ACCOUNT_ADDRESS) + .catch((thrown: unknown) => thrown); + + expect(isTerminalMoneyAccountUpgradeError(error)).toBe(true); + }); + + 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, + }); + mocks.signPersonalMessage.mockRejectedValue(new Error('network down')); + + const error = await controller + .upgradeAccount(MOCK_ACCOUNT_ADDRESS) + .catch((thrown: unknown) => thrown); + + expect(isMoneyAccountUpgradeStepError(error)).toBe(true); + expect(isTerminalMoneyAccountUpgradeError(error)).toBe(false); + }); + }); + + 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 mixedCaseAddress = MOCK_ACCOUNT_ADDRESS.replace( + '0xabc', + '0xABC', + ) as Hex; + + await controller.upgradeAccount(mixedCaseAddress); + + expect(mocks.signPersonalMessage).toHaveBeenCalled(); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toStrictEqual({ + configFingerprint: expect.any(String), + completedAt: expect.any(Number), + }); + }); + + 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, + }); + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + clearMockCalls(mocks); + + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + expect(mocks.providerRequest).not.toHaveBeenCalled(); + expect(mocks.listDelegations).not.toHaveBeenCalled(); + expect(mocks.getIntentsByAddress).not.toHaveBeenCalled(); + }); + + it('treats recorded upgrades case-insensitively', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + clearMockCalls(mocks); + + await controller.upgradeAccount( + MOCK_ACCOUNT_ADDRESS.replace('0xabc', '0xABC') as Hex, + ); + + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + }); + + 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.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.controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect(second.mocks.signPersonalMessage).not.toHaveBeenCalled(); + expect(second.mocks.providerRequest).not.toHaveBeenCalled(); + }); + + 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, + }); + mocks.signPersonalMessage.mockRejectedValueOnce( + new Error('signing failed'), + ); + + await expect( + controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('signing failed'); + + expect(controller.state.upgradedAccounts).toStrictEqual({}); + + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toBeDefined(); + }); + + 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, + }); + 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. + mocks.getServiceDetails.mockResolvedValue({ + ...MOCK_SERVICE_DETAILS_RESPONSE, + chains: { + [MOCK_CHAIN_ID]: { + ...MOCK_SERVICE_DETAILS_RESPONSE.chains[MOCK_CHAIN_ID], + autoDepositDelegate: + '0x2222222222222222222222222222222222222222' as Hex, + }, + }, + }); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + clearMockCalls(mocks); + + await controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS); + + expect(mocks.signPersonalMessage).toHaveBeenCalled(); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS] + .configFingerprint, + ).not.toBe(originalFingerprint); + }); + }); + + describe('upgradeAccountWithRetry', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + it('resolves after a single attempt when the upgrade succeeds', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + + await controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS); + + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toBeDefined(); + }); + + it('retries a failed attempt after 10 seconds', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + mocks.signPersonalMessage.mockRejectedValueOnce( + new Error('network down'), + ); + jest.useFakeTimers(); + + const promise = controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS); + await jest.advanceTimersByTimeAsync(0); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(9_999); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(1); + await promise; + + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(2); + expect( + controller.state.upgradedAccounts[MOCK_ACCOUNT_ADDRESS], + ).toBeDefined(); + }); + + it('backs off exponentially between attempts, capped at 60 seconds', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + mocks.signPersonalMessage.mockRejectedValue(new Error('network down')); + jest.useFakeTimers(); + + const promise = controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS, { + maxAttempts: 6, + }); + // Swallow the eventual rejection so advancing timers doesn't surface an + // unhandled rejection; the real assertion happens below. + promise.catch(() => undefined); + + await jest.advanceTimersByTimeAsync(0); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(10_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(2); + await jest.advanceTimersByTimeAsync(20_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(3); + await jest.advanceTimersByTimeAsync(40_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(4); + await jest.advanceTimersByTimeAsync(60_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(5); + // The cap repeats once the schedule is exhausted. + await jest.advanceTimersByTimeAsync(60_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(6); + + await expect(promise).rejects.toThrow('network down'); + }); + + it('gives up after maxAttempts and rethrows the last step error', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + mocks.signPersonalMessage.mockRejectedValue(new Error('network down')); + jest.useFakeTimers(); + + const promise = controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS, { + maxAttempts: 2, + }); + promise.catch(() => undefined); + await jest.advanceTimersByTimeAsync(10_000); + + await expect(promise).rejects.toMatchObject({ + step: 'associate-address', + }); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(2); + expect(controller.state.upgradedAccounts).toStrictEqual({}); + }); + + it('does not retry terminal failures', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + // Account delegated to a third-party impl — retrying cannot help. + mocks.providerRequest.mockImplementation( + async ({ method }: { method: string }) => { + if (method === 'eth_getCode') { + return `0xef0100${'9'.repeat(40)}`; + } + return '0x0'; + }, + ); + + await expect( + controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow('already upgraded to another smart account'); + + expect(mocks.signEip7702Authorization).not.toHaveBeenCalled(); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + }); + + it('does not retry non-step errors such as calling before init', async () => { + const { controller, mocks } = setup(); + + await expect( + controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS), + ).rejects.toThrow( + 'MoneyAccountUpgradeController must be initialized via init() before upgradeAccount() can be called', + ); + + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + }); + + it('throws without attempting when the signal is already aborted', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + const abortController = new AbortController(); + abortController.abort(); + + await expect( + controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS, { + signal: abortController.signal, + }), + ).rejects.toThrow('Money Account upgrade retry aborted'); + + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + }); + + it('stops retrying when the signal aborts during the backoff wait', async () => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + mocks.signPersonalMessage.mockRejectedValue(new Error('network down')); + jest.useFakeTimers(); + const abortController = new AbortController(); + + const promise = controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS, { + signal: abortController.signal, + }); + promise.catch(() => undefined); + await jest.advanceTimersByTimeAsync(0); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + + abortController.abort(); + await expect(promise).rejects.toThrow( + 'Money Account upgrade retry aborted', + ); + + // The pending wait was cancelled — advancing time runs no further attempts. + await jest.advanceTimersByTimeAsync(120_000); + expect(mocks.signPersonalMessage).toHaveBeenCalledTimes(1); + }); + + it('is callable via the messenger', async () => { + const { controller, rootMessenger } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + + expect( + await rootMessenger.call( + 'MoneyAccountUpgradeController:upgradeAccountWithRetry', + MOCK_ACCOUNT_ADDRESS, + ), + ).toBeUndefined(); + }); }); }); diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index b38e095b474..51e6aa5b8d3 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -31,7 +31,11 @@ import type { import { hexToNumber } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; -import { MoneyAccountUpgradeStepError } from './errors'; +import { + MoneyAccountUpgradeStepError, + isMoneyAccountUpgradeStepError, + isTerminalMoneyAccountUpgradeError, +} from './errors'; import type { MoneyAccountUpgradeControllerMethodActions } from './MoneyAccountUpgradeController-method-action-types'; import { associateAddressStep } from './steps/associate-address'; import { buildDelegationStep } from './steps/build-delegations'; @@ -48,12 +52,67 @@ const DELEGATION_FRAMEWORK_VERSION = '1.3.0'; export const controllerName = 'MoneyAccountUpgradeController'; -export type MoneyAccountUpgradeControllerState = Record; +/** + * Delays between retry attempts in + * {@link MoneyAccountUpgradeController.upgradeAccountWithRetry}. Once the + * schedule is exhausted the last delay repeats. + */ +const RETRY_DELAYS_MS = [10_000, 20_000, 40_000, 60_000]; + +const DEFAULT_MAX_RETRY_ATTEMPTS = 5; + +const RETRY_ABORTED_MESSAGE = 'Money Account upgrade retry aborted'; + +/** + * Record of a Money Account upgrade sequence that ran to completion. + */ +export type MoneyAccountUpgradeStatus = { + /** + * Fingerprint of the upgrade config the sequence completed under. The + * record is only trusted while the active config produces the same + * fingerprint — if the chain, CHOMP contracts, or Delegation Framework + * version change, the sequence re-runs. + */ + configFingerprint: string; + /** Unix timestamp (in milliseconds) when the sequence completed. */ + completedAt: number; +}; -const moneyAccountUpgradeControllerMetadata = - {} satisfies StateMetadata; +export type MoneyAccountUpgradeControllerState = { + /** + * Accounts whose upgrade sequence has fully completed, keyed by lowercased + * account address. + */ + upgradedAccounts: { [address: Hex]: MoneyAccountUpgradeStatus }; +}; -const MESSENGER_EXPOSED_METHODS = ['upgradeAccount'] as const; +const moneyAccountUpgradeControllerMetadata = { + upgradedAccounts: { + includeInDebugSnapshot: false, + includeInStateLogs: true, + persist: true, + usedInUi: false, + }, +} satisfies StateMetadata; + +/** + * Constructs the default {@link MoneyAccountUpgradeController} state. This + * allows consumers to provide a partial state object when initializing the + * controller and also helps in constructing complete state objects for this + * controller in tests. + * + * @returns The default {@link MoneyAccountUpgradeController} state. + */ +export function getDefaultMoneyAccountUpgradeControllerState(): MoneyAccountUpgradeControllerState { + return { + upgradedAccounts: {}, + }; +} + +const MESSENGER_EXPOSED_METHODS = [ + 'upgradeAccount', + 'upgradeAccountWithRetry', +] as const; export type MoneyAccountUpgradeControllerGetStateAction = ControllerGetStateAction< @@ -120,17 +179,23 @@ 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. */ constructor({ messenger, + state, }: { messenger: MoneyAccountUpgradeControllerMessenger; + state?: Partial; }) { super({ messenger, metadata: moneyAccountUpgradeControllerMetadata, name: controllerName, - state: {}, + state: { + ...getDefaultMoneyAccountUpgradeControllerState(), + ...state, + }, }); this.messenger.registerMethodActionHandlers( @@ -210,6 +275,12 @@ export class MoneyAccountUpgradeController extends BaseController< * {@link MoneyAccountUpgradeStepError} that records which step failed (the * original error is preserved as `cause`). * + * A run that completes is recorded in state (keyed by lowercased address, + * fingerprinted against the active config); subsequent calls for a + * recorded account return immediately without running any steps. If the + * active config no longer matches the recorded fingerprint, the sequence + * re-runs. + * * @param address - The Money Account address to upgrade. */ async upgradeAccount(address: Hex): Promise { @@ -218,17 +289,140 @@ export class MoneyAccountUpgradeController extends BaseController< 'MoneyAccountUpgradeController must be initialized via init() before upgradeAccount() can be called', ); } + const config = this.#config; + + const accountKey = address.toLowerCase() as Hex; + const configFingerprint = computeConfigFingerprint(config); + if ( + this.state.upgradedAccounts[accountKey]?.configFingerprint === + configFingerprint + ) { + return; + } for (const step of this.#steps) { try { await step.run({ messenger: this.messenger, address, - ...this.#config, + ...config, }); } catch (error) { throw new MoneyAccountUpgradeStepError(step.name, error); } } + + this.update((state) => { + state.upgradedAccounts[accountKey] = { + configFingerprint, + completedAt: Date.now(), + }; + }); + } + + /** + * Runs the upgrade sequence via + * {@link MoneyAccountUpgradeController.upgradeAccount}, retrying failed + * attempts with capped exponential backoff (10s, 20s, 40s, then 60s + * between attempts). Rethrows the last error without further attempts when + * the failure is terminal (see `isTerminalMoneyAccountUpgradeError`), when + * it is not a step failure at all, or when `maxAttempts` is exhausted. + * + * @param address - The Money Account address to upgrade. + * @param options - Retry options. + * @param options.signal - Aborts waiting between attempts and prevents + * further attempts. An aborted run rejects with an error stating the retry + * was aborted. + * @param options.maxAttempts - Maximum number of attempts, including the + * first. Defaults to 5. + */ + async upgradeAccountWithRetry( + address: Hex, + { + signal, + maxAttempts = DEFAULT_MAX_RETRY_ATTEMPTS, + }: { signal?: AbortSignal; maxAttempts?: number } = {}, + ): Promise { + for (let attempt = 1; ; attempt++) { + if (signal?.aborted) { + throw new Error(RETRY_ABORTED_MESSAGE); + } + try { + await this.upgradeAccount(address); + return; + } catch (error) { + if ( + attempt >= maxAttempts || + !isMoneyAccountUpgradeStepError(error) || + isTerminalMoneyAccountUpgradeError(error) + ) { + throw error; + } + await waitUnlessAborted(retryDelayMs(attempt), signal); + } + } } } + +/** + * Derives a stable fingerprint of the config fields that define what + * "upgraded" means for an account. A recorded upgrade is only trusted while + * the active config produces the same fingerprint. + * + * @param config - The active upgrade config. + * @returns A canonical string over the config's identifying fields. + */ +function computeConfigFingerprint( + config: UpgradeConfig & { chainId: Hex }, +): string { + return [ + DELEGATION_FRAMEWORK_VERSION, + config.chainId, + config.delegateAddress, + config.musdTokenAddress, + config.boringVaultAddress, + config.vedaVaultAdapterAddress, + config.delegatorImplAddress, + config.erc20TransferAmountEnforcer, + config.redeemerEnforcer, + config.valueLteEnforcer, + ] + .map((value) => value.toLowerCase()) + .join('|'); +} + +/** + * The backoff delay to wait after the given (1-indexed) failed attempt. Once + * the schedule is exhausted, the last delay repeats. + * + * @param attempt - The attempt that just failed. + * @returns The delay in milliseconds. + */ +function retryDelayMs(attempt: number): number { + return RETRY_DELAYS_MS[Math.min(attempt, RETRY_DELAYS_MS.length) - 1]; +} + +/** + * Waits for the given duration, rejecting early if `signal` aborts. + * + * @param durationMs - How long to wait. + * @param signal - Abort signal that cancels the wait. + * @returns A promise that resolves after the wait, or rejects on abort. + */ +async function waitUnlessAborted( + durationMs: number, + signal?: AbortSignal, +): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + signal?.removeEventListener('abort', onAbort); + resolve(); + }, durationMs); + + function onAbort(): void { + clearTimeout(timer); + reject(new Error(RETRY_ABORTED_MESSAGE)); + } + signal?.addEventListener('abort', onAbort, { once: true }); + }); +} diff --git a/packages/money-account-upgrade-controller/src/errors.test.ts b/packages/money-account-upgrade-controller/src/errors.test.ts index 53b3f1a885f..7bfa63f6ab4 100644 --- a/packages/money-account-upgrade-controller/src/errors.test.ts +++ b/packages/money-account-upgrade-controller/src/errors.test.ts @@ -1,6 +1,8 @@ import { MoneyAccountUpgradeStepError, + TerminalUpgradeError, isMoneyAccountUpgradeStepError, + isTerminalMoneyAccountUpgradeError, } from './errors'; describe('MoneyAccountUpgradeStepError', () => { @@ -26,6 +28,87 @@ describe('MoneyAccountUpgradeStepError', () => { 'Money Account upgrade failed at step "register-intents": 42', ); }); + + it('is non-terminal when the cause is a plain Error', () => { + const error = new MoneyAccountUpgradeStepError( + 'associate-address', + new Error('network down'), + ); + + expect(error.terminal).toBe(false); + }); + + it('is terminal when the cause is a TerminalUpgradeError', () => { + const error = new MoneyAccountUpgradeStepError( + 'eip-7702-authorization', + new TerminalUpgradeError('delegated elsewhere'), + ); + + expect(error.terminal).toBe(true); + }); + + it('is terminal when the cause is a structurally-terminal error from another realm', () => { + const cause = new Error('delegated elsewhere'); + (cause as unknown as { terminal: boolean }).terminal = true; + + const error = new MoneyAccountUpgradeStepError( + 'eip-7702-authorization', + cause, + ); + + expect(error.terminal).toBe(true); + }); + + it('is non-terminal when the cause is a non-Error carrying a terminal property', () => { + const error = new MoneyAccountUpgradeStepError('associate-address', { + terminal: true, + }); + + expect(error.terminal).toBe(false); + }); +}); + +describe('TerminalUpgradeError', () => { + it('is an Error marked as terminal', () => { + const error = new TerminalUpgradeError('cannot recover'); + + expect(error).toBeInstanceOf(Error); + expect(error.name).toBe('TerminalUpgradeError'); + expect(error.message).toBe('cannot recover'); + expect(error.terminal).toBe(true); + }); +}); + +describe('isTerminalMoneyAccountUpgradeError', () => { + it('returns true for a step error with a terminal cause', () => { + expect( + isTerminalMoneyAccountUpgradeError( + new MoneyAccountUpgradeStepError( + 'eip-7702-authorization', + new TerminalUpgradeError('delegated elsewhere'), + ), + ), + ).toBe(true); + }); + + it('returns false for a step error with a non-terminal cause', () => { + expect( + isTerminalMoneyAccountUpgradeError( + new MoneyAccountUpgradeStepError('associate-address', new Error('x')), + ), + ).toBe(false); + }); + + it('returns false for an unwrapped TerminalUpgradeError', () => { + expect( + isTerminalMoneyAccountUpgradeError(new TerminalUpgradeError('x')), + ).toBe(false); + }); + + it('returns false for non-step-error values', () => { + expect(isTerminalMoneyAccountUpgradeError(undefined)).toBe(false); + expect(isTerminalMoneyAccountUpgradeError(new Error('x'))).toBe(false); + }); }); describe('isMoneyAccountUpgradeStepError', () => { diff --git a/packages/money-account-upgrade-controller/src/errors.ts b/packages/money-account-upgrade-controller/src/errors.ts index 0fc386d60cc..1aef34ff3c8 100644 --- a/packages/money-account-upgrade-controller/src/errors.ts +++ b/packages/money-account-upgrade-controller/src/errors.ts @@ -14,6 +14,13 @@ export class MoneyAccountUpgradeStepError extends Error { /** The underlying error thrown by the step. */ readonly cause: unknown; + /** + * Whether the failure is terminal — the condition will not resolve on its + * own, so retrying the upgrade sequence cannot succeed. Derived from the + * cause (see {@link TerminalUpgradeError}). + */ + readonly terminal: boolean; + constructor(step: string, cause: unknown) { const causeMessage = cause instanceof Error ? cause.message : String(cause); super(`Money Account upgrade failed at step "${step}": ${causeMessage}`); @@ -21,6 +28,27 @@ export class MoneyAccountUpgradeStepError extends Error { this.name = 'MoneyAccountUpgradeStepError'; this.step = step; this.cause = cause; + this.terminal = + cause instanceof Error && + (cause as { terminal?: unknown }).terminal === true; + } +} + +/** + * Error a step throws to mark a failure as terminal: the condition will not + * resolve on its own, so retrying the upgrade sequence is pointless — e.g. + * the account is already delegated to a third-party implementation. + * + * Detected structurally via the `terminal` property (rather than + * `instanceof`) so the marking survives module-realm duplication. + */ +export class TerminalUpgradeError extends Error { + /** Marks the failure as not retryable. */ + readonly terminal = true; + + constructor(message: string) { + super(message); + this.name = 'TerminalUpgradeError'; } } @@ -43,3 +71,23 @@ export function isMoneyAccountUpgradeStepError( typeof (error as { step?: unknown }).step === 'string' ); } + +/** + * Whether `error` is a {@link MoneyAccountUpgradeStepError} marked as + * terminal — a failure that will not resolve on its own, so retrying the + * upgrade sequence cannot succeed. + * + * Uses the same structural checks as {@link isMoneyAccountUpgradeStepError} + * so it holds across module realm boundaries. + * + * @param error - The value to test. + * @returns Whether `error` is a terminal `MoneyAccountUpgradeStepError`. + */ +export function isTerminalMoneyAccountUpgradeError( + error: unknown, +): error is MoneyAccountUpgradeStepError { + return ( + isMoneyAccountUpgradeStepError(error) && + (error as { terminal?: unknown }).terminal === true + ); +} diff --git a/packages/money-account-upgrade-controller/src/index.ts b/packages/money-account-upgrade-controller/src/index.ts index 784e62cfd50..3b64f38a2bc 100644 --- a/packages/money-account-upgrade-controller/src/index.ts +++ b/packages/money-account-upgrade-controller/src/index.ts @@ -1,9 +1,14 @@ export type { UpgradeConfig } from './types'; export { MoneyAccountUpgradeStepError, + TerminalUpgradeError, isMoneyAccountUpgradeStepError, + isTerminalMoneyAccountUpgradeError, } from './errors'; -export { MoneyAccountUpgradeController } from './MoneyAccountUpgradeController'; +export { + MoneyAccountUpgradeController, + getDefaultMoneyAccountUpgradeControllerState, +} from './MoneyAccountUpgradeController'; export type { MoneyAccountUpgradeControllerState, MoneyAccountUpgradeControllerGetStateAction, @@ -11,5 +16,9 @@ export type { MoneyAccountUpgradeControllerStateChangedEvent, MoneyAccountUpgradeControllerEvents, MoneyAccountUpgradeControllerMessenger, + MoneyAccountUpgradeStatus, } from './MoneyAccountUpgradeController'; -export type { MoneyAccountUpgradeControllerUpgradeAccountAction } from './MoneyAccountUpgradeController-method-action-types'; +export type { + MoneyAccountUpgradeControllerUpgradeAccountAction, + MoneyAccountUpgradeControllerUpgradeAccountWithRetryAction, +} from './MoneyAccountUpgradeController-method-action-types'; diff --git a/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.test.ts b/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.test.ts index e1c179fc8fb..24de94126e2 100644 --- a/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.test.ts +++ b/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.test.ts @@ -202,6 +202,13 @@ describe('eip7702AuthorizationStep', () => { expect(mocks.signEip7702Authorization).not.toHaveBeenCalled(); expect(mocks.createUpgrade).not.toHaveBeenCalled(); }); + + it('marks the failure as terminal', async () => { + const { messenger, mocks } = setup(); + configureProvider(mocks, delegationCode(MOCK_THIRD_PARTY_IMPL)); + + await expect(run(messenger)).rejects.toMatchObject({ terminal: true }); + }); }); describe('when the account has unexpected non-delegation code', () => { @@ -217,6 +224,13 @@ describe('eip7702AuthorizationStep', () => { expect(mocks.createUpgrade).not.toHaveBeenCalled(); }); + it('marks the failure as terminal', async () => { + const { messenger, mocks } = setup(); + configureProvider(mocks, '0x6080604052' as Hex); + + await expect(run(messenger)).rejects.toMatchObject({ terminal: true }); + }); + it('throws when eth_getCode returns a non-hex value', async () => { const { messenger, mocks } = setup(); mocks.providerRequest.mockImplementation(async ({ method }) => { diff --git a/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.ts b/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.ts index f494a679452..1c217c17d6e 100644 --- a/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.ts +++ b/packages/money-account-upgrade-controller/src/steps/eip-7702-authorization.ts @@ -2,6 +2,7 @@ import type { Provider } from '@metamask/network-controller'; import { add0x, isStrictHexString } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; +import { TerminalUpgradeError } from '../errors'; import type { Step, StepContext } from './step'; const EIP_7702_DELEGATION_PREFIX = '0xef0100'; @@ -47,7 +48,7 @@ export const eip7702AuthorizationStep: Step = { if (existingDelegation === delegatorImplAddress.toLowerCase()) { return 'already-done'; } - throw new Error( + throw new TerminalUpgradeError( `Account ${address} is already upgraded to another smart account: ${existingDelegation}.`, ); } @@ -184,7 +185,7 @@ async function fetchDelegationAddress( return add0x(normalized.slice(EIP_7702_DELEGATION_PREFIX.length)); } - throw new Error( + throw new TerminalUpgradeError( `Account ${address} has unexpected on-chain code; expected either no code or an EIP-7702 delegation.`, ); } From 8b1e39b5667bdb67e771bea203bb968c9558a60d Mon Sep 17 00:00:00 2001 From: John Whiles Date: Thu, 16 Jul 2026 10:17:28 +0100 Subject: [PATCH 2/3] feat(money-account-upgrade-controller): treat confirmed cross-profile association conflicts as terminal A 409 from associateAddress whose disambiguating lookup confirms the address is not associated with the authenticated profile cannot resolve by retrying, so throw TerminalUpgradeError to stop the retry loop. If the lookup itself fails, the original retryable conflict still propagates. Co-Authored-By: Claude Fable 5 --- .../CHANGELOG.md | 2 +- .../src/steps/associate-address.test.ts | 17 ++++++++++++----- .../src/steps/associate-address.ts | 18 ++++++++++++++---- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index 10494cc62bb..0d46eed1f85 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The constructor now accepts an optional `state` option, merged with the defaults; add `getDefaultMoneyAccountUpgradeControllerState` to construct those defaults. - Add `upgradeAccountWithRetry` method and matching `MoneyAccountUpgradeController:upgradeAccountWithRetry` messenger action ([#9500](https://github.com/MetaMask/core/pull/9500)) - Retries failed `upgradeAccount` attempts with capped exponential backoff (10s, 20s, 40s, then 60s between attempts; 5 attempts by default). Terminal failures and non-step errors are rethrown without retrying. Accepts an `AbortSignal` to cancel waiting between attempts. -- Add `TerminalUpgradeError` and `isTerminalMoneyAccountUpgradeError`, and a `terminal` property on `MoneyAccountUpgradeStepError`, marking failures that cannot resolve by retrying — currently an account delegated to a third-party EIP-7702 implementation, or an account with unexpected on-chain code ([#9500](https://github.com/MetaMask/core/pull/9500)) +- Add `TerminalUpgradeError` and `isTerminalMoneyAccountUpgradeError`, and a `terminal` property on `MoneyAccountUpgradeStepError`, marking failures that cannot resolve by retrying — currently an account delegated to a third-party EIP-7702 implementation, an account with unexpected on-chain code, or an address confirmed to be associated with a different CHOMP profile ([#9500](https://github.com/MetaMask/core/pull/9500)) ### Changed diff --git a/packages/money-account-upgrade-controller/src/steps/associate-address.test.ts b/packages/money-account-upgrade-controller/src/steps/associate-address.test.ts index c1fafd7bd87..0cd00da6491 100644 --- a/packages/money-account-upgrade-controller/src/steps/associate-address.test.ts +++ b/packages/money-account-upgrade-controller/src/steps/associate-address.test.ts @@ -244,26 +244,33 @@ describe('associateAddressStep', () => { expect(result).toBe('already-done'); }); - it('rethrows a conflict when the address belongs to another profile', async () => { + it('throws a terminal error when the address belongs to another profile', async () => { const { messenger, mocks } = setup(); mocks.associateAddress.mockRejectedValue(conflictError()); mocks.getAssociatedAddresses.mockResolvedValue([]); - await expect(run(messenger)).rejects.toThrow( - "POST /v1/auth/address failed with status '409'", + const error = await run(messenger).catch((thrown: unknown) => thrown); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe( + `Address ${MOCK_ADDRESS} is associated with a different CHOMP profile.`, ); + expect(error).toMatchObject({ terminal: true }); }); - it('rethrows a conflict when the disambiguating lookup also fails', async () => { + it('rethrows the original, non-terminal conflict when the disambiguating lookup also fails', async () => { const { messenger, mocks } = setup(); mocks.associateAddress.mockRejectedValue(conflictError()); mocks.getAssociatedAddresses .mockResolvedValueOnce([]) .mockRejectedValueOnce(new Error('lookup failed')); - await expect(run(messenger)).rejects.toThrow( + const error = await run(messenger).catch((thrown: unknown) => thrown); + + expect((error as Error).message).toBe( "POST /v1/auth/address failed with status '409'", ); + expect(error).not.toMatchObject({ terminal: true }); }); it('propagates errors from signing and does not submit to the API', async () => { diff --git a/packages/money-account-upgrade-controller/src/steps/associate-address.ts b/packages/money-account-upgrade-controller/src/steps/associate-address.ts index 4bb9c3fec09..b66d4209a6b 100644 --- a/packages/money-account-upgrade-controller/src/steps/associate-address.ts +++ b/packages/money-account-upgrade-controller/src/steps/associate-address.ts @@ -3,6 +3,7 @@ import { hasProperty } from '@metamask/utils'; import { equalsIgnoreCase } from './delegation-matchers'; import type { Step } from './step'; +import { TerminalUpgradeError } from '../errors'; /** * Determines whether an error is a CHOMP conflict (HTTP 409) response. @@ -38,7 +39,10 @@ function isConflictError(error: unknown): boolean { * also returns it when two same-profile requests race on the initial create * (the loser's conditional write fails). The step disambiguates by re-fetching * the associations: if the address is now present the race was benign and the - * step reports `'already-done'`; otherwise the conflict propagates. + * step reports `'already-done'`. A confirmed cross-profile conflict is thrown + * as a {@link TerminalUpgradeError}, since no amount of retrying dissociates + * the address from the other profile; if the disambiguating lookup itself + * fails, the original (retryable) conflict propagates instead. */ export const associateAddressStep: Step = { name: 'associate-address', @@ -74,13 +78,19 @@ export const associateAddressStep: Step = { return response.status === 'active' ? 'already-done' : 'completed'; } catch (error) { if (isConflictError(error)) { + let associated; try { - if (await isAssociated()) { - return 'already-done'; - } + associated = await isAssociated(); } catch { // Could not disambiguate — surface the original conflict. + throw error; } + if (associated) { + return 'already-done'; + } + throw new TerminalUpgradeError( + `Address ${address} is associated with a different CHOMP profile.`, + ); } throw error; } From 5a47678b5596ff0bf87020de128ec79dbdd192f2 Mon Sep 17 00:00:00 2001 From: John Whiles Date: Thu, 16 Jul 2026 12:35:29 +0100 Subject: [PATCH 3/3] fix(money-account-upgrade-controller): address review findings - Mark the state-shape change as BREAKING in the changelog; the release is already major via #9387. - Exclude upgradedAccounts from state logs (address-keyed enrollment data, matching precedent in other address-keyed controllers). - Validate that maxAttempts is an integer >= 1 so NaN cannot cause unbounded retries. Co-Authored-By: Claude Fable 5 --- .../CHANGELOG.md | 6 ++--- ...ntUpgradeController-method-action-types.ts | 2 +- .../src/MoneyAccountUpgradeController.test.ts | 26 +++++++++++++++++++ .../src/MoneyAccountUpgradeController.ts | 7 +++-- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/money-account-upgrade-controller/CHANGELOG.md b/packages/money-account-upgrade-controller/CHANGELOG.md index 0d46eed1f85..20adc07e498 100644 --- a/packages/money-account-upgrade-controller/CHANGELOG.md +++ b/packages/money-account-upgrade-controller/CHANGELOG.md @@ -9,11 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add persisted state tracking fully upgraded accounts ([#9500](https://github.com/MetaMask/core/pull/9500)) - - `MoneyAccountUpgradeControllerState` now contains `upgradedAccounts`, keyed by lowercased account address. Each entry records when the upgrade sequence completed and a fingerprint of the config it completed under (see new `MoneyAccountUpgradeStatus` type). +- **BREAKING:** Add persisted state tracking fully upgraded accounts ([#9500](https://github.com/MetaMask/core/pull/9500)) + - `MoneyAccountUpgradeControllerState` changes from `Record` to `{ upgradedAccounts }`, keyed by lowercased account address. Each entry records when the upgrade sequence completed and a fingerprint of the config it completed under (see new `MoneyAccountUpgradeStatus` type). Code constructing the state type (e.g. `{}` in tests or default-state maps) must include `upgradedAccounts`. - The constructor now accepts an optional `state` option, merged with the defaults; add `getDefaultMoneyAccountUpgradeControllerState` to construct those defaults. - Add `upgradeAccountWithRetry` method and matching `MoneyAccountUpgradeController:upgradeAccountWithRetry` messenger action ([#9500](https://github.com/MetaMask/core/pull/9500)) - - Retries failed `upgradeAccount` attempts with capped exponential backoff (10s, 20s, 40s, then 60s between attempts; 5 attempts by default). Terminal failures and non-step errors are rethrown without retrying. Accepts an `AbortSignal` to cancel waiting between attempts. + - Retries failed `upgradeAccount` attempts with capped exponential backoff (10s, 20s, 40s, then 60s between attempts; 5 attempts by default). Terminal failures and non-step errors are rethrown without retrying. Accepts an `AbortSignal` to cancel waiting between attempts. Throws if `maxAttempts` is not an integer of at least 1. - Add `TerminalUpgradeError` and `isTerminalMoneyAccountUpgradeError`, and a `terminal` property on `MoneyAccountUpgradeStepError`, marking failures that cannot resolve by retrying — currently an account delegated to a third-party EIP-7702 implementation, an account with unexpected on-chain code, or an address confirmed to be associated with a different CHOMP profile ([#9500](https://github.com/MetaMask/core/pull/9500)) ### Changed 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 b40cbd0f9d8..2761f403d87 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 @@ -40,7 +40,7 @@ export type MoneyAccountUpgradeControllerUpgradeAccountAction = { * further attempts. An aborted run rejects with an error stating the retry * was aborted. * @param options.maxAttempts - Maximum number of attempts, including the - * first. Defaults to 5. + * first. Must be an integer of at least 1. Defaults to 5. */ export type MoneyAccountUpgradeControllerUpgradeAccountWithRetryAction = { type: `MoneyAccountUpgradeController:upgradeAccountWithRetry`; diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts index 125ca5c56c7..d1cdf94e8f0 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts @@ -873,6 +873,32 @@ describe('MoneyAccountUpgradeController', () => { expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); }); + it.each([ + ['zero', 0], + ['negative', -1], + ['a non-integer', 2.5], + ['NaN', Number.NaN], + ])( + 'rejects without attempting when maxAttempts is %s', + async (_label, maxAttempts) => { + const { controller, mocks } = setup(); + await controller.init({ + chainId: MOCK_CHAIN_ID, + boringVaultAddress: MOCK_BORING_VAULT_ADDRESS, + }); + + await expect( + controller.upgradeAccountWithRetry(MOCK_ACCOUNT_ADDRESS, { + maxAttempts, + }), + ).rejects.toThrow( + `maxAttempts must be an integer >= 1, got ${maxAttempts}`, + ); + + expect(mocks.signPersonalMessage).not.toHaveBeenCalled(); + }, + ); + it('throws without attempting when the signal is already aborted', async () => { const { controller, mocks } = setup(); await controller.init({ diff --git a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts index 51e6aa5b8d3..15e0602f4c9 100644 --- a/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts +++ b/packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts @@ -89,7 +89,7 @@ export type MoneyAccountUpgradeControllerState = { const moneyAccountUpgradeControllerMetadata = { upgradedAccounts: { includeInDebugSnapshot: false, - includeInStateLogs: true, + includeInStateLogs: false, persist: true, usedInUi: false, }, @@ -334,7 +334,7 @@ export class MoneyAccountUpgradeController extends BaseController< * further attempts. An aborted run rejects with an error stating the retry * was aborted. * @param options.maxAttempts - Maximum number of attempts, including the - * first. Defaults to 5. + * first. Must be an integer of at least 1. Defaults to 5. */ async upgradeAccountWithRetry( address: Hex, @@ -343,6 +343,9 @@ export class MoneyAccountUpgradeController extends BaseController< maxAttempts = DEFAULT_MAX_RETRY_ATTEMPTS, }: { signal?: AbortSignal; maxAttempts?: number } = {}, ): Promise { + if (!Number.isInteger(maxAttempts) || maxAttempts < 1) { + throw new Error(`maxAttempts must be an integer >= 1, got ${maxAttempts}`); + } for (let attempt = 1; ; attempt++) { if (signal?.aborted) { throw new Error(RETRY_ABORTED_MESSAGE);