Skip to content

Commit d562139

Browse files
authored
feat: expose more info about chomp upgrade step failures (MetaMask#9204)
## Explanation When the upgrade process throws an error, include the step where the error occurred in what is thrown. This will allow client applications to report errors more effectively. <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Only changes error propagation on the failure path; upgrade sequencing and success behavior are unchanged. > > **Overview** > **`upgradeAccount`** now catches failures from any upgrade step and re-throws them as **`MoneyAccountUpgradeStepError`**, with **`step`** set to that step’s `name` and the original value kept on **`cause`**. The public message still includes the underlying text (from `Error.message` or `String(cause)`). > > The package adds **`MoneyAccountUpgradeStepError`**, **`isMoneyAccountUpgradeStepError`** (structural guard for bundled apps), unit/integration tests, changelog notes, and updated JSDoc for the messenger action type. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d66b594. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 88638be commit d562139

7 files changed

Lines changed: 207 additions & 9 deletions

File tree

packages/money-account-upgrade-controller/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Add `MoneyAccountUpgradeStepError` (and the `isMoneyAccountUpgradeStepError` type guard). `upgradeAccount` now wraps any error thrown by a step in this error, exposing the failing step's `name` as `step` and preserving the original error as `cause`, so consumers can attribute failures to a specific step when reporting to Sentry). ([#9204](https://github.com/MetaMask/core/pull/9204))
13+
1014
### Changed
1115

1216
- Bump `@metamask/keyring-controller` from `^27.0.0` to `^27.1.0` ([#9129](https://github.com/MetaMask/core/pull/9129))

packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController-method-action-types.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ import type { MoneyAccountUpgradeController } from './MoneyAccountUpgradeControl
99
* Runs each step in the upgrade sequence in order. A step that reports
1010
* `'already-done'` is skipped without performing any action; a step that
1111
* reports `'completed'` has performed its action. An error thrown by any
12-
* step propagates and halts the sequence.
12+
* step halts the sequence and is re-thrown wrapped in a
13+
* {@link MoneyAccountUpgradeStepError} that records which step failed (the
14+
* original error is preserved as `cause`).
1315
*
1416
* @param address - The Money Account address to upgrade.
1517
*/

packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.test.ts

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,14 @@ import type {
88
import { hexToNumber } from '@metamask/utils';
99
import type { Hex } from '@metamask/utils';
1010

11-
import type { MoneyAccountUpgradeControllerMessenger } from '.';
12-
import { MoneyAccountUpgradeController } from '.';
11+
import type {
12+
MoneyAccountUpgradeControllerMessenger,
13+
MoneyAccountUpgradeStepError,
14+
} from '.';
15+
import {
16+
MoneyAccountUpgradeController,
17+
isMoneyAccountUpgradeStepError,
18+
} from '.';
1319

1420
const MOCK_CHAIN_ID = '0x1' as Hex; // mainnet, supported in delegation-deployments@1.3.0
1521
const UNSUPPORTED_CHAIN_ID = '0x539' as Hex; // 1337 — local dev, not in registry
@@ -438,5 +444,70 @@ describe('MoneyAccountUpgradeController', () => {
438444
controller.upgradeAccount(MOCK_ACCOUNT_ADDRESS),
439445
).rejects.toThrow('signing failed');
440446
});
447+
448+
it('wraps a step failure in a MoneyAccountUpgradeStepError that records the step and cause', async () => {
449+
const { controller, mocks } = setup();
450+
await controller.init({
451+
chainId: MOCK_CHAIN_ID,
452+
boringVaultAddress: MOCK_BORING_VAULT_ADDRESS,
453+
});
454+
const cause = new Error('signing failed');
455+
// The associate-address step (first in the sequence) signs a personal
456+
// message before calling CHOMP, so failing this surfaces that step.
457+
mocks.signPersonalMessage.mockRejectedValue(cause);
458+
459+
const error = await controller
460+
.upgradeAccount(MOCK_ACCOUNT_ADDRESS)
461+
.catch((thrown: unknown) => thrown);
462+
463+
expect(isMoneyAccountUpgradeStepError(error)).toBe(true);
464+
expect(error).toMatchObject({
465+
step: 'associate-address',
466+
cause,
467+
});
468+
expect((error as MoneyAccountUpgradeStepError).message).toBe(
469+
'Money Account upgrade failed at step "associate-address": signing failed',
470+
);
471+
});
472+
473+
it('records the name of the specific step that failed', async () => {
474+
const { controller, mocks } = setup();
475+
await controller.init({
476+
chainId: MOCK_CHAIN_ID,
477+
boringVaultAddress: MOCK_BORING_VAULT_ADDRESS,
478+
});
479+
// The first step (associate-address) passes; fail at the second step
480+
// (eip-7702-authorization), which signs the authorization.
481+
mocks.signEip7702Authorization.mockRejectedValue(
482+
new Error('authorization rejected'),
483+
);
484+
485+
const error = await controller
486+
.upgradeAccount(MOCK_ACCOUNT_ADDRESS)
487+
.catch((thrown: unknown) => thrown);
488+
489+
expect(error).toMatchObject({ step: 'eip-7702-authorization' });
490+
});
491+
492+
it('wraps a non-Error thrown by a step, stringifying it as the cause message', async () => {
493+
const { controller, mocks } = setup();
494+
await controller.init({
495+
chainId: MOCK_CHAIN_ID,
496+
boringVaultAddress: MOCK_BORING_VAULT_ADDRESS,
497+
});
498+
mocks.signPersonalMessage.mockRejectedValue('plain string failure');
499+
500+
const error = await controller
501+
.upgradeAccount(MOCK_ACCOUNT_ADDRESS)
502+
.catch((thrown: unknown) => thrown);
503+
504+
expect(error).toMatchObject({
505+
step: 'associate-address',
506+
cause: 'plain string failure',
507+
});
508+
expect((error as MoneyAccountUpgradeStepError).message).toBe(
509+
'Money Account upgrade failed at step "associate-address": plain string failure',
510+
);
511+
});
441512
});
442513
});

packages/money-account-upgrade-controller/src/MoneyAccountUpgradeController.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import type {
3030
import { hexToNumber } from '@metamask/utils';
3131
import type { Hex } from '@metamask/utils';
3232

33+
import { MoneyAccountUpgradeStepError } from './errors';
3334
import type { MoneyAccountUpgradeControllerMethodActions } from './MoneyAccountUpgradeController-method-action-types';
3435
import { associateAddressStep } from './steps/associate-address';
3536
import { buildDelegationStep } from './steps/build-delegations';
@@ -203,7 +204,9 @@ export class MoneyAccountUpgradeController extends BaseController<
203204
* Runs each step in the upgrade sequence in order. A step that reports
204205
* `'already-done'` is skipped without performing any action; a step that
205206
* reports `'completed'` has performed its action. An error thrown by any
206-
* step propagates and halts the sequence.
207+
* step halts the sequence and is re-thrown wrapped in a
208+
* {@link MoneyAccountUpgradeStepError} that records which step failed (the
209+
* original error is preserved as `cause`).
207210
*
208211
* @param address - The Money Account address to upgrade.
209212
*/
@@ -215,11 +218,15 @@ export class MoneyAccountUpgradeController extends BaseController<
215218
}
216219

217220
for (const step of this.#steps) {
218-
await step.run({
219-
messenger: this.messenger,
220-
address,
221-
...this.#config,
222-
});
221+
try {
222+
await step.run({
223+
messenger: this.messenger,
224+
address,
225+
...this.#config,
226+
});
227+
} catch (error) {
228+
throw new MoneyAccountUpgradeStepError(step.name, error);
229+
}
223230
}
224231
}
225232
}
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
import {
2+
MoneyAccountUpgradeStepError,
3+
isMoneyAccountUpgradeStepError,
4+
} from './errors';
5+
6+
describe('MoneyAccountUpgradeStepError', () => {
7+
it('records the step name and preserves an Error cause', () => {
8+
const cause = new Error('boom');
9+
10+
const error = new MoneyAccountUpgradeStepError('build-delegation', cause);
11+
12+
expect(error).toBeInstanceOf(Error);
13+
expect(error.name).toBe('MoneyAccountUpgradeStepError');
14+
expect(error.step).toBe('build-delegation');
15+
expect(error.cause).toBe(cause);
16+
expect(error.message).toBe(
17+
'Money Account upgrade failed at step "build-delegation": boom',
18+
);
19+
});
20+
21+
it('stringifies a non-Error cause in the message', () => {
22+
const error = new MoneyAccountUpgradeStepError('register-intents', 42);
23+
24+
expect(error.cause).toBe(42);
25+
expect(error.message).toBe(
26+
'Money Account upgrade failed at step "register-intents": 42',
27+
);
28+
});
29+
});
30+
31+
describe('isMoneyAccountUpgradeStepError', () => {
32+
it('returns true for a MoneyAccountUpgradeStepError', () => {
33+
expect(
34+
isMoneyAccountUpgradeStepError(
35+
new MoneyAccountUpgradeStepError('associate-address', new Error('x')),
36+
),
37+
).toBe(true);
38+
});
39+
40+
it('returns true for a structurally-equivalent error from another realm', () => {
41+
const lookalike = new Error('whatever');
42+
lookalike.name = 'MoneyAccountUpgradeStepError';
43+
(lookalike as unknown as { step: string }).step = 'associate-address';
44+
45+
expect(isMoneyAccountUpgradeStepError(lookalike)).toBe(true);
46+
});
47+
48+
it('returns false for a plain Error', () => {
49+
expect(isMoneyAccountUpgradeStepError(new Error('nope'))).toBe(false);
50+
});
51+
52+
it('returns false for an error with the right name but no step', () => {
53+
const error = new Error('nope');
54+
error.name = 'MoneyAccountUpgradeStepError';
55+
56+
expect(isMoneyAccountUpgradeStepError(error)).toBe(false);
57+
});
58+
59+
it('returns false for non-error values', () => {
60+
expect(isMoneyAccountUpgradeStepError(undefined)).toBe(false);
61+
expect(isMoneyAccountUpgradeStepError(null)).toBe(false);
62+
expect(isMoneyAccountUpgradeStepError('error')).toBe(false);
63+
expect(isMoneyAccountUpgradeStepError({ step: 'x' })).toBe(false);
64+
});
65+
});
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/**
2+
* Error thrown by `MoneyAccountUpgradeController.upgradeAccount` when one of
3+
* the upgrade steps fails.
4+
*
5+
* Wraps the underlying error (preserved as `cause`) and records the name of
6+
* the step that was running, so consumers can attribute the failure to a
7+
* specific step — e.g. when tagging an error report — without parsing the
8+
* message.
9+
*/
10+
export class MoneyAccountUpgradeStepError extends Error {
11+
/** The name of the step that threw. */
12+
readonly step: string;
13+
14+
/** The underlying error thrown by the step. */
15+
readonly cause: unknown;
16+
17+
constructor(step: string, cause: unknown) {
18+
const causeMessage = cause instanceof Error ? cause.message : String(cause);
19+
super(`Money Account upgrade failed at step "${step}": ${causeMessage}`);
20+
21+
this.name = 'MoneyAccountUpgradeStepError';
22+
this.step = step;
23+
this.cause = cause;
24+
}
25+
}
26+
27+
/**
28+
* Type guard for {@link MoneyAccountUpgradeStepError}.
29+
*
30+
* Uses a structural check rather than `instanceof` so it holds across module
31+
* realm boundaries — e.g. when the controller is consumed from a bundled host
32+
* app where a duplicate copy of this class may exist.
33+
*
34+
* @param error - The value to test.
35+
* @returns Whether `error` is a `MoneyAccountUpgradeStepError`.
36+
*/
37+
export function isMoneyAccountUpgradeStepError(
38+
error: unknown,
39+
): error is MoneyAccountUpgradeStepError {
40+
return (
41+
error instanceof Error &&
42+
error.name === 'MoneyAccountUpgradeStepError' &&
43+
typeof (error as { step?: unknown }).step === 'string'
44+
);
45+
}

packages/money-account-upgrade-controller/src/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
export type { UpgradeConfig } from './types';
2+
export {
3+
MoneyAccountUpgradeStepError,
4+
isMoneyAccountUpgradeStepError,
5+
} from './errors';
26
export { MoneyAccountUpgradeController } from './MoneyAccountUpgradeController';
37
export type {
48
MoneyAccountUpgradeControllerState,

0 commit comments

Comments
 (0)