From 07218a9b7f05f5c49eb8d1401f5b14df16558b7d Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 21:45:01 +0200 Subject: [PATCH 1/8] Assign stack on ocap errors --- packages/errors/src/BaseError.test.ts | 26 ++++- packages/errors/src/BaseError.ts | 23 +++- .../errors/src/errors/StreamReadError.test.ts | 9 +- packages/errors/src/errors/StreamReadError.ts | 18 +-- .../src/errors/VatAlreadyExistsError.test.ts | 1 + .../src/errors/VatAlreadyExistsError.ts | 13 ++- .../VatCapTpConnectionExistsError.test.ts | 1 + .../errors/VatCapTpConnectionExistsError.ts | 13 ++- .../VatCapTpConnectionNotFoundError.test.ts | 1 + .../errors/VatCapTpConnectionNotFoundError.ts | 15 ++- .../errors/src/errors/VatDeletedError.test.ts | 1 + packages/errors/src/errors/VatDeletedError.ts | 15 ++- .../src/errors/VatNotFoundError.test.ts | 1 + .../errors/src/errors/VatNotFoundError.ts | 15 ++- .../errors/src/marshal/unmarshalError.test.ts | 105 +++++++++++++++++- packages/errors/src/marshal/unmarshalError.ts | 38 +++++-- packages/errors/src/types.ts | 4 + 17 files changed, 253 insertions(+), 46 deletions(-) diff --git a/packages/errors/src/BaseError.test.ts b/packages/errors/src/BaseError.test.ts index d4a8719f3..29f1c75dc 100644 --- a/packages/errors/src/BaseError.test.ts +++ b/packages/errors/src/BaseError.test.ts @@ -9,6 +9,7 @@ describe('BaseError', () => { const mockMessage = 'VAT was not found.'; const mockData = { key: 'value' }; const mockCause = new Error('Root cause error'); + const mockStack = 'Error stack'; it('creates a BaseError with required properties', () => { const error = new BaseError(mockCode, mockMessage); @@ -22,12 +23,17 @@ describe('BaseError', () => { }); it('creates a BaseError with all properties', () => { - const error = new BaseError(mockCode, mockMessage, mockData, mockCause); + const error = new BaseError(mockCode, mockMessage, { + data: mockData, + cause: mockCause, + stack: mockStack, + }); expect(error.name).toBe('BaseError'); expect(error.message).toBe(mockMessage); expect(error.code).toBe(mockCode); expect(error.data).toStrictEqual(mockData); expect(error.cause).toBe(mockCause); + expect(error.stack).toBe(mockStack); }); it('inherits from the Error class and have the correct name', () => { @@ -37,13 +43,15 @@ describe('BaseError', () => { }); it('handles a missing data parameter', () => { - const error = new BaseError(mockCode, mockMessage, undefined, mockCause); + const error = new BaseError(mockCode, mockMessage, { + cause: mockCause, + }); expect(error.data).toBeUndefined(); expect(error.cause).toBe(mockCause); }); it('handles a missing cause parameter', () => { - const error = new BaseError(mockCode, mockMessage, mockData); + const error = new BaseError(mockCode, mockMessage, { data: mockData }); expect(error.data).toStrictEqual(mockData); expect(error.cause).toBeUndefined(); }); @@ -53,4 +61,16 @@ describe('BaseError', () => { 'Unmarshal method not implemented', ); }); + + it('initializes the stack property automatically if not provided', () => { + const error = new BaseError(mockCode, mockMessage, { cause: mockCause }); + expect(error.stack).toBeDefined(); + }); + + it('creates a BaseError with a custom stack', () => { + const error = new BaseError(mockCode, mockMessage, { stack: mockStack }); + expect(error.stack).toBe(mockStack); + expect(error.data).toBeUndefined(); + expect(error.cause).toBeUndefined(); + }); }); diff --git a/packages/errors/src/BaseError.ts b/packages/errors/src/BaseError.ts index 3817def11..adb32808e 100644 --- a/packages/errors/src/BaseError.ts +++ b/packages/errors/src/BaseError.ts @@ -1,20 +1,37 @@ import type { Json } from '@metamask/utils'; import type { ErrorCode } from './constants.js'; -import type { MarshaledOcapError, OcapError } from './types.js'; +import type { + MarshaledOcapError, + OcapError, + ErrorOptionsWithStack, +} from './types.js'; export class BaseError extends Error implements OcapError { public readonly code: ErrorCode; public readonly data: Json | undefined; - constructor(code: ErrorCode, message: string, data?: Json, cause?: unknown) { + constructor( + code: ErrorCode, + message: string, + options: ErrorOptionsWithStack & { + data?: Json; + } = {}, + ) { + const { data, cause, stack } = options; + super(message, { cause }); this.name = this.constructor.name; this.code = code; this.data = data; - this.cause = cause; + + // override the stack property if provided + if (stack) { + this.stack = stack; + } + harden(this); } diff --git a/packages/errors/src/errors/StreamReadError.test.ts b/packages/errors/src/errors/StreamReadError.test.ts index e78c062d5..1bebeaed2 100644 --- a/packages/errors/src/errors/StreamReadError.test.ts +++ b/packages/errors/src/errors/StreamReadError.test.ts @@ -12,7 +12,7 @@ describe('StreamReadError', () => { it('creates a StreamReadError for Supervisor with the correct properties', () => { const error = new StreamReadError( { supervisorId: mockSupervisorId }, - mockOriginalError, + { cause: mockOriginalError }, ); expect(error).toBeInstanceOf(StreamReadError); expect(error.code).toBe(ErrorCode.StreamReadError); @@ -22,7 +22,10 @@ describe('StreamReadError', () => { }); it('creates a StreamReadError for Vat with the correct properties', () => { - const error = new StreamReadError({ vatId: mockVatId }, mockOriginalError); + const error = new StreamReadError( + { vatId: mockVatId }, + { cause: mockOriginalError }, + ); expect(error).toBeInstanceOf(StreamReadError); expect(error.code).toBe(ErrorCode.StreamReadError); expect(error.message).toBe('Unexpected stream read error.'); @@ -49,6 +52,7 @@ describe('StreamReadError', () => { expect(unmarshaledError).toBeInstanceOf(StreamReadError); expect(unmarshaledError.code).toBe(ErrorCode.StreamReadError); expect(unmarshaledError.message).toBe('Unexpected stream read error.'); + expect(unmarshaledError.stack).toBe('customStack'); expect(unmarshaledError.data).toStrictEqual({ vatId: mockVatId, }); @@ -75,6 +79,7 @@ describe('StreamReadError', () => { expect(unmarshaledError).toBeInstanceOf(StreamReadError); expect(unmarshaledError.code).toBe(ErrorCode.StreamReadError); expect(unmarshaledError.message).toBe('Unexpected stream read error.'); + expect(unmarshaledError.stack).toBe('customStack'); expect(unmarshaledError.data).toStrictEqual({ supervisorId: mockSupervisorId, }); diff --git a/packages/errors/src/errors/StreamReadError.ts b/packages/errors/src/errors/StreamReadError.ts index 73abe67f0..012452fb4 100644 --- a/packages/errors/src/errors/StreamReadError.ts +++ b/packages/errors/src/errors/StreamReadError.ts @@ -14,18 +14,19 @@ import { ErrorCode, MarshaledErrorStruct, } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; type StreamReadErrorData = { vatId: string } | { supervisorId: string }; +type StreamReadErrorOptions = Required & + Pick; export class StreamReadError extends BaseError { - constructor(data: StreamReadErrorData, originalError: Error) { - super( - ErrorCode.StreamReadError, - 'Unexpected stream read error.', + constructor(data: StreamReadErrorData, options: StreamReadErrorOptions) { + super(ErrorCode.StreamReadError, 'Unexpected stream read error.', { + ...options, data, - originalError, - ); + }); harden(this); } @@ -52,8 +53,7 @@ export class StreamReadError extends BaseError { assert(marshaledError, this.struct); return new StreamReadError( marshaledError.data as StreamReadErrorData, - // The cause will be properly unmarshaled during the parent call. - new Error(marshaledError.cause?.message), + unmarshalErrorOptions(marshaledError) as StreamReadErrorOptions, ); } } diff --git a/packages/errors/src/errors/VatAlreadyExistsError.test.ts b/packages/errors/src/errors/VatAlreadyExistsError.test.ts index 649c6e4a0..dcd9f8b6a 100644 --- a/packages/errors/src/errors/VatAlreadyExistsError.test.ts +++ b/packages/errors/src/errors/VatAlreadyExistsError.test.ts @@ -29,6 +29,7 @@ describe('VatAlreadyExistsError', () => { expect(unmarshaledError).toBeInstanceOf(VatAlreadyExistsError); expect(unmarshaledError.code).toBe(ErrorCode.VatAlreadyExists); expect(unmarshaledError.message).toBe('Vat already exists.'); + expect(unmarshaledError.stack).toBe('stack trace'); expect(unmarshaledError.data).toStrictEqual({ vatId: mockVatId, }); diff --git a/packages/errors/src/errors/VatAlreadyExistsError.ts b/packages/errors/src/errors/VatAlreadyExistsError.ts index c8a9e9291..5653de8ae 100644 --- a/packages/errors/src/errors/VatAlreadyExistsError.ts +++ b/packages/errors/src/errors/VatAlreadyExistsError.ts @@ -2,12 +2,14 @@ import { assert, literal, object, string } from '@metamask/superstruct'; import { BaseError } from '../BaseError.js'; import { marshaledErrorSchema, ErrorCode } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; export class VatAlreadyExistsError extends BaseError { - constructor(vatId: string) { + constructor(vatId: string, options?: ErrorOptionsWithStack) { super(ErrorCode.VatAlreadyExists, 'Vat already exists.', { - vatId, + ...options, + data: { vatId }, }); harden(this); } @@ -33,7 +35,10 @@ export class VatAlreadyExistsError extends BaseError { marshaledError: MarshaledOcapError, ): VatAlreadyExistsError { assert(marshaledError, this.struct); - return new VatAlreadyExistsError(marshaledError.data.vatId); + return new VatAlreadyExistsError( + marshaledError.data.vatId, + unmarshalErrorOptions(marshaledError), + ); } } harden(VatAlreadyExistsError); diff --git a/packages/errors/src/errors/VatCapTpConnectionExistsError.test.ts b/packages/errors/src/errors/VatCapTpConnectionExistsError.test.ts index efd6d8f2b..17410152f 100644 --- a/packages/errors/src/errors/VatCapTpConnectionExistsError.test.ts +++ b/packages/errors/src/errors/VatCapTpConnectionExistsError.test.ts @@ -29,6 +29,7 @@ describe('VatCapTpConnectionExistsError', () => { VatCapTpConnectionExistsError.unmarshal(marshaledError); expect(unmarshaledError).toBeInstanceOf(VatCapTpConnectionExistsError); expect(unmarshaledError.code).toBe(ErrorCode.VatCapTpConnectionExists); + expect(unmarshaledError.stack).toBe('stack trace'); expect(unmarshaledError.message).toBe( 'Vat already has a CapTP connection.', ); diff --git a/packages/errors/src/errors/VatCapTpConnectionExistsError.ts b/packages/errors/src/errors/VatCapTpConnectionExistsError.ts index 275bc35e7..e0ef9b73b 100644 --- a/packages/errors/src/errors/VatCapTpConnectionExistsError.ts +++ b/packages/errors/src/errors/VatCapTpConnectionExistsError.ts @@ -2,15 +2,17 @@ import { assert, literal, object, string } from '@metamask/superstruct'; import { BaseError } from '../BaseError.js'; import { marshaledErrorSchema, ErrorCode } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; export class VatCapTpConnectionExistsError extends BaseError { - constructor(vatId: string) { + constructor(vatId: string, options?: ErrorOptionsWithStack) { super( ErrorCode.VatCapTpConnectionExists, 'Vat already has a CapTP connection.', { - vatId, + ...options, + data: { vatId }, }, ); harden(this); @@ -37,7 +39,10 @@ export class VatCapTpConnectionExistsError extends BaseError { marshaledError: MarshaledOcapError, ): VatCapTpConnectionExistsError { assert(marshaledError, this.struct); - return new VatCapTpConnectionExistsError(marshaledError.data.vatId); + return new VatCapTpConnectionExistsError( + marshaledError.data.vatId, + unmarshalErrorOptions(marshaledError), + ); } } harden(VatCapTpConnectionExistsError); diff --git a/packages/errors/src/errors/VatCapTpConnectionNotFoundError.test.ts b/packages/errors/src/errors/VatCapTpConnectionNotFoundError.test.ts index a76dcf74d..89ae04c7a 100644 --- a/packages/errors/src/errors/VatCapTpConnectionNotFoundError.test.ts +++ b/packages/errors/src/errors/VatCapTpConnectionNotFoundError.test.ts @@ -29,6 +29,7 @@ describe('VatCapTpConnectionNotFoundError', () => { VatCapTpConnectionNotFoundError.unmarshal(marshaledError); expect(unmarshaledError).toBeInstanceOf(VatCapTpConnectionNotFoundError); expect(unmarshaledError.code).toBe(ErrorCode.VatCapTpConnectionNotFound); + expect(unmarshaledError.stack).toBe('stack trace'); expect(unmarshaledError.message).toBe( 'Vat does not have a CapTP connection.', ); diff --git a/packages/errors/src/errors/VatCapTpConnectionNotFoundError.ts b/packages/errors/src/errors/VatCapTpConnectionNotFoundError.ts index 6500332f4..662859733 100644 --- a/packages/errors/src/errors/VatCapTpConnectionNotFoundError.ts +++ b/packages/errors/src/errors/VatCapTpConnectionNotFoundError.ts @@ -2,14 +2,18 @@ import { assert, literal, object, string } from '@metamask/superstruct'; import { BaseError } from '../BaseError.js'; import { marshaledErrorSchema, ErrorCode } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; export class VatCapTpConnectionNotFoundError extends BaseError { - constructor(vatId: string) { + constructor(vatId: string, options?: ErrorOptionsWithStack) { super( ErrorCode.VatCapTpConnectionNotFound, 'Vat does not have a CapTP connection.', - { vatId }, + { + ...options, + data: { vatId }, + }, ); harden(this); } @@ -35,7 +39,10 @@ export class VatCapTpConnectionNotFoundError extends BaseError { marshaledError: MarshaledOcapError, ): VatCapTpConnectionNotFoundError { assert(marshaledError, this.struct); - return new VatCapTpConnectionNotFoundError(marshaledError.data.vatId); + return new VatCapTpConnectionNotFoundError( + marshaledError.data.vatId, + unmarshalErrorOptions(marshaledError), + ); } } harden(VatCapTpConnectionNotFoundError); diff --git a/packages/errors/src/errors/VatDeletedError.test.ts b/packages/errors/src/errors/VatDeletedError.test.ts index 28e478a57..1fa7016a8 100644 --- a/packages/errors/src/errors/VatDeletedError.test.ts +++ b/packages/errors/src/errors/VatDeletedError.test.ts @@ -29,6 +29,7 @@ describe('VatDeletedError', () => { expect(unmarshaledError).toBeInstanceOf(VatDeletedError); expect(unmarshaledError.code).toBe(ErrorCode.VatDeleted); expect(unmarshaledError.message).toBe('Vat was deleted.'); + expect(unmarshaledError.stack).toBe('stack trace'); expect(unmarshaledError.data).toStrictEqual({ vatId: mockVatId, }); diff --git a/packages/errors/src/errors/VatDeletedError.ts b/packages/errors/src/errors/VatDeletedError.ts index b6fff0cf2..122a7114e 100644 --- a/packages/errors/src/errors/VatDeletedError.ts +++ b/packages/errors/src/errors/VatDeletedError.ts @@ -2,11 +2,15 @@ import { assert, literal, object, string } from '@metamask/superstruct'; import { BaseError } from '../BaseError.js'; import { marshaledErrorSchema, ErrorCode } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; export class VatDeletedError extends BaseError { - constructor(vatId: string) { - super(ErrorCode.VatDeleted, 'Vat was deleted.', { vatId }); + constructor(vatId: string, options?: ErrorOptionsWithStack) { + super(ErrorCode.VatDeleted, 'Vat was deleted.', { + ...options, + data: { vatId }, + }); harden(this); } @@ -29,7 +33,10 @@ export class VatDeletedError extends BaseError { */ public static unmarshal(marshaledError: MarshaledOcapError): VatDeletedError { assert(marshaledError, this.struct); - return new VatDeletedError(marshaledError.data.vatId); + return new VatDeletedError( + marshaledError.data.vatId, + unmarshalErrorOptions(marshaledError), + ); } } harden(VatDeletedError); diff --git a/packages/errors/src/errors/VatNotFoundError.test.ts b/packages/errors/src/errors/VatNotFoundError.test.ts index d61f74b89..e24f872ba 100644 --- a/packages/errors/src/errors/VatNotFoundError.test.ts +++ b/packages/errors/src/errors/VatNotFoundError.test.ts @@ -29,6 +29,7 @@ describe('VatNotFoundError', () => { expect(unmarshaledError).toBeInstanceOf(VatNotFoundError); expect(unmarshaledError.code).toBe(ErrorCode.VatNotFound); expect(unmarshaledError.message).toBe('Vat does not exist.'); + expect(unmarshaledError.stack).toBe('stack trace'); expect(unmarshaledError.data).toStrictEqual({ vatId: mockVatId, }); diff --git a/packages/errors/src/errors/VatNotFoundError.ts b/packages/errors/src/errors/VatNotFoundError.ts index 10d699b48..8254cf866 100644 --- a/packages/errors/src/errors/VatNotFoundError.ts +++ b/packages/errors/src/errors/VatNotFoundError.ts @@ -2,11 +2,15 @@ import { assert, literal, object, string } from '@metamask/superstruct'; import { BaseError } from '../BaseError.js'; import { marshaledErrorSchema, ErrorCode } from '../constants.js'; -import type { MarshaledOcapError } from '../types.js'; +import { unmarshalErrorOptions } from '../marshal/unmarshalError.js'; +import type { ErrorOptionsWithStack, MarshaledOcapError } from '../types.js'; export class VatNotFoundError extends BaseError { - constructor(vatId: string) { - super(ErrorCode.VatNotFound, 'Vat does not exist.', { vatId }); + constructor(vatId: string, options?: ErrorOptionsWithStack) { + super(ErrorCode.VatNotFound, 'Vat does not exist.', { + ...options, + data: { vatId }, + }); harden(this); } @@ -31,7 +35,10 @@ export class VatNotFoundError extends BaseError { marshaledError: MarshaledOcapError, ): VatNotFoundError { assert(marshaledError, this.struct); - return new VatNotFoundError(marshaledError.data.vatId); + return new VatNotFoundError( + marshaledError.data.vatId, + unmarshalErrorOptions(marshaledError), + ); } } harden(VatNotFoundError); diff --git a/packages/errors/src/marshal/unmarshalError.test.ts b/packages/errors/src/marshal/unmarshalError.test.ts index d510b2d95..a2ee23ff3 100644 --- a/packages/errors/src/marshal/unmarshalError.test.ts +++ b/packages/errors/src/marshal/unmarshalError.test.ts @@ -1,7 +1,7 @@ import { makeErrorMatcherFactory } from '@ocap/test-utils'; import { describe, it, expect } from 'vitest'; -import { unmarshalError } from './unmarshalError.js'; +import { unmarshalError, unmarshalErrorOptions } from './unmarshalError.js'; import { ErrorCode, ErrorSentinel } from '../constants.js'; import { StreamReadError } from '../errors/StreamReadError.js'; import { VatAlreadyExistsError } from '../errors/VatAlreadyExistsError.js'; @@ -89,7 +89,9 @@ describe('unmarshalError', () => { const expectedCauseError = new Error('foo'); expectedCauseError.stack = 'bar'; - const expectedError = new StreamReadError(data, expectedCauseError); + const expectedError = new StreamReadError(data, { + cause: expectedCauseError, + }); expectedError.stack = 'customStack'; const unmarshaledError = unmarshalError(marshaledError) as OcapError; @@ -113,4 +115,103 @@ describe('unmarshalError', () => { 'At path: data -- Expected an object, but received: "invalid data"', ); }); + + it('should unmarshal a marshaled error without a stack trace', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'foo', + } as const; + + expect(unmarshalError(marshaledError)).toStrictEqual( + makeErrorMatcher(new Error('foo')), + ); + }); + + it('should unmarshal a marshaled error without a cause', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'foo', + stack: 'bar', + } as const; + + expect(unmarshalError(marshaledError)).toStrictEqual( + makeErrorMatcher(new Error('foo')), + ); + }); +}); + +describe('unmarshalErrorOptions', () => { + it('should unmarshal error options without cause', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'An error occurred.', + stack: 'Error stack trace', + } as const; + + const expectedOptions = { + stack: 'Error stack trace', + }; + + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( + expectedOptions, + ); + }); + + it('should unmarshal error options with string cause', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'An error occurred.', + stack: 'Error stack trace', + cause: 'A string cause', + } as const; + + const expectedOptions = { + stack: 'Error stack trace', + cause: new Error('A string cause'), + }; + + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( + expectedOptions, + ); + }); + + it('should unmarshal error options with nested marshaled error as cause', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'An error occurred.', + stack: 'Error stack trace', + cause: { + [ErrorSentinel]: true, + message: 'Cause message', + stack: 'Cause stack trace', + }, + } as const; + + const expectedCauseError = new Error('Cause message'); + expectedCauseError.stack = 'Cause stack trace'; + + const expectedOptions = { + stack: 'Error stack trace', + cause: expectedCauseError, + }; + + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( + expectedOptions, + ); + }); + + it('should return default stack when stack is undefined', () => { + const marshaledError = { + [ErrorSentinel]: true, + message: 'An error occurred.', + } as const; + + const expectedOptions = { + stack: '', + }; + + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( + expectedOptions, + ); + }); }); diff --git a/packages/errors/src/marshal/unmarshalError.ts b/packages/errors/src/marshal/unmarshalError.ts index f2f6851b9..7edc69630 100644 --- a/packages/errors/src/marshal/unmarshalError.ts +++ b/packages/errors/src/marshal/unmarshalError.ts @@ -1,6 +1,10 @@ import { isMarshaledOcapError } from './isMarshaledOcapError.js'; import { errorClasses } from '../errors/index.js'; -import type { MarshaledError, OcapError } from '../types.js'; +import type { + ErrorOptionsWithStack, + MarshaledError, + OcapError, +} from '../types.js'; /** * Unmarshals a {@link MarshaledError} into an {@link Error}. @@ -11,24 +15,44 @@ import type { MarshaledError, OcapError } from '../types.js'; export function unmarshalError( marshaledError: MarshaledError, ): Error | OcapError { - let error: Error | OcapError; - if (isMarshaledOcapError(marshaledError)) { - error = errorClasses[marshaledError.code].unmarshal(marshaledError); - } else { - error = new Error(marshaledError.message); + return errorClasses[marshaledError.code].unmarshal(marshaledError); } + let cause; if (marshaledError.cause) { - error.cause = + cause = typeof marshaledError.cause === 'string' ? marshaledError.cause : unmarshalError(marshaledError.cause); } + const error = new Error(marshaledError.message, { cause }); + if (marshaledError.stack) { error.stack = marshaledError.stack; } return error; } + +/** + * Gets the error options from a marshaled error. + * + * @param marshaledError - The marshaled error to get the options from. + * @returns The error options. + */ +export function unmarshalErrorOptions( + marshaledError: MarshaledError, +): ErrorOptionsWithStack { + const output: ErrorOptionsWithStack = { stack: marshaledError.stack ?? '' }; + + if (marshaledError.cause) { + output.cause = + typeof marshaledError.cause === 'string' + ? new Error(marshaledError.cause) + : unmarshalError(marshaledError.cause); + } + + return output; +} diff --git a/packages/errors/src/types.ts b/packages/errors/src/types.ts index 13c106b33..a933cbc89 100644 --- a/packages/errors/src/types.ts +++ b/packages/errors/src/types.ts @@ -2,6 +2,10 @@ import type { Json } from '@metamask/utils'; import type { ErrorCode, ErrorSentinel } from './constants.js'; +export type ErrorOptionsWithStack = ErrorOptions & { + stack?: string; +}; + export type OcapError = { code: ErrorCode; data: Json | undefined; From fd4aa743df63130e1bf4b91783b02e0f9dfd71a2 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 22:16:47 +0200 Subject: [PATCH 2/8] Dont mock endo in tests more tests --- packages/errors/package.json | 1 + packages/errors/src/marshal/endoify.test.ts | 16 ++++++++++++++++ .../errors/src/marshal/unmarshalError.test.ts | 10 ++++++++-- packages/errors/vitest.config.ts | 10 +++++++++- 4 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 packages/errors/src/marshal/endoify.test.ts diff --git a/packages/errors/package.json b/packages/errors/package.json index bb358bcf2..e9c065311 100644 --- a/packages/errors/package.json +++ b/packages/errors/package.json @@ -51,6 +51,7 @@ "@metamask/eslint-config": "^14.0.0", "@metamask/eslint-config-nodejs": "^14.0.0", "@metamask/eslint-config-typescript": "^14.0.0", + "@ocap/shims": "workspace:^", "@ocap/test-utils": "workspace:^", "@ts-bridge/cli": "^0.5.1", "@ts-bridge/shims": "^0.1.1", diff --git a/packages/errors/src/marshal/endoify.test.ts b/packages/errors/src/marshal/endoify.test.ts new file mode 100644 index 000000000..d3b61ad02 --- /dev/null +++ b/packages/errors/src/marshal/endoify.test.ts @@ -0,0 +1,16 @@ +import '@ocap/shims/endoify'; + +import { VatAlreadyExistsError } from 'src/errors/VatAlreadyExistsError.js'; +import { describe, it, expect } from 'vitest'; + +import { marshalError } from './marshalError.js'; +import { unmarshalError } from './unmarshalError.js'; + +describe('marshal', () => { + it('should round trip a thrown error', async () => { + const thrown = new VatAlreadyExistsError('v123'); + const marshaled = marshalError(thrown); + const received = unmarshalError(marshaled); + expect(received).toStrictEqual(thrown); + }); +}); diff --git a/packages/errors/src/marshal/unmarshalError.test.ts b/packages/errors/src/marshal/unmarshalError.test.ts index a2ee23ff3..e1e4bd4a1 100644 --- a/packages/errors/src/marshal/unmarshalError.test.ts +++ b/packages/errors/src/marshal/unmarshalError.test.ts @@ -61,7 +61,9 @@ describe('unmarshalError', () => { } as const; const expectedError = new VatAlreadyExistsError(data.vatId); - expectedError.stack = 'customStack'; + expect(() => (expectedError.stack = 'customStack')).toThrow( + 'Cannot assign to read only property', + ); const unmarshaledError = unmarshalError(marshaledError) as OcapError; @@ -90,9 +92,13 @@ describe('unmarshalError', () => { expectedCauseError.stack = 'bar'; const expectedError = new StreamReadError(data, { + stack: 'customStack', cause: expectedCauseError, }); - expectedError.stack = 'customStack'; + // since StreamReadError is hardened we cannot modify the stack + expect(() => (expectedError.stack = 'customStack')).toThrow( + 'Cannot assign to read only property', + ); const unmarshaledError = unmarshalError(marshaledError) as OcapError; diff --git a/packages/errors/vitest.config.ts b/packages/errors/vitest.config.ts index a158b2c0e..b43a3b78a 100644 --- a/packages/errors/vitest.config.ts +++ b/packages/errors/vitest.config.ts @@ -1,6 +1,7 @@ // eslint-disable-next-line spaced-comment /// +import path from 'path'; import { defineConfig, mergeConfig } from 'vite'; import { getDefaultConfig } from '../../vitest.config.packages.js'; @@ -12,7 +13,14 @@ const config = mergeConfig( defineConfig({ test: { pool: 'vmThreads', - setupFiles: '../test-utils/src/env/mock-endo.ts', + setupFiles: path.resolve('../shims/src/endoify.js'), + alias: [ + { + find: '@ocap/shims/endoify', + replacement: path.resolve('../shims/src/endoify.js'), + customResolver: (id) => ({ external: true, id }), + }, + ], }, }), ); From 6f4ba3b4d0af49fd1e70a4af693fca5ba8dde989 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 22:27:30 +0200 Subject: [PATCH 3/8] fix lint --- packages/errors/src/marshal/endoify.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/errors/src/marshal/endoify.test.ts b/packages/errors/src/marshal/endoify.test.ts index d3b61ad02..f8f0d843b 100644 --- a/packages/errors/src/marshal/endoify.test.ts +++ b/packages/errors/src/marshal/endoify.test.ts @@ -1,10 +1,10 @@ import '@ocap/shims/endoify'; -import { VatAlreadyExistsError } from 'src/errors/VatAlreadyExistsError.js'; import { describe, it, expect } from 'vitest'; import { marshalError } from './marshalError.js'; import { unmarshalError } from './unmarshalError.js'; +import { VatAlreadyExistsError } from '../errors/VatAlreadyExistsError.js'; describe('marshal', () => { it('should round trip a thrown error', async () => { From 09596a3279cebeaec9f7a8537eda2e7cadeb9398 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 22:29:01 +0200 Subject: [PATCH 4/8] run yarn --- yarn.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/yarn.lock b/yarn.lock index 8312a704f..dc42959d8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1471,6 +1471,7 @@ __metadata: "@metamask/eslint-config-typescript": "npm:^14.0.0" "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^9.3.0" + "@ocap/shims": "workspace:^" "@ocap/test-utils": "workspace:^" "@ts-bridge/cli": "npm:^0.5.1" "@ts-bridge/shims": "npm:^0.1.1" From 25c4e50712d7a738032334d82fbdec307160ab62 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 22:33:49 +0200 Subject: [PATCH 5/8] refine --- .../errors/src/marshal/unmarshalError.test.ts | 32 +++++-------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/packages/errors/src/marshal/unmarshalError.test.ts b/packages/errors/src/marshal/unmarshalError.test.ts index e1e4bd4a1..30d30a062 100644 --- a/packages/errors/src/marshal/unmarshalError.test.ts +++ b/packages/errors/src/marshal/unmarshalError.test.ts @@ -154,13 +154,9 @@ describe('unmarshalErrorOptions', () => { stack: 'Error stack trace', } as const; - const expectedOptions = { + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ stack: 'Error stack trace', - }; - - expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( - expectedOptions, - ); + }); }); it('should unmarshal error options with string cause', () => { @@ -171,14 +167,10 @@ describe('unmarshalErrorOptions', () => { cause: 'A string cause', } as const; - const expectedOptions = { + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ stack: 'Error stack trace', cause: new Error('A string cause'), - }; - - expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( - expectedOptions, - ); + }); }); it('should unmarshal error options with nested marshaled error as cause', () => { @@ -196,14 +188,10 @@ describe('unmarshalErrorOptions', () => { const expectedCauseError = new Error('Cause message'); expectedCauseError.stack = 'Cause stack trace'; - const expectedOptions = { + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ stack: 'Error stack trace', cause: expectedCauseError, - }; - - expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( - expectedOptions, - ); + }); }); it('should return default stack when stack is undefined', () => { @@ -212,12 +200,8 @@ describe('unmarshalErrorOptions', () => { message: 'An error occurred.', } as const; - const expectedOptions = { + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ stack: '', - }; - - expect(unmarshalErrorOptions(marshaledError)).toStrictEqual( - expectedOptions, - ); + }); }); }); From e8931bc8fcc4731df8a93589835c388c72a6f49b Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 21 Oct 2024 22:39:39 +0200 Subject: [PATCH 6/8] use unmarshalErrorOptions at unmarshalError --- packages/errors/src/marshal/unmarshalError.test.ts | 2 +- packages/errors/src/marshal/unmarshalError.ts | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/errors/src/marshal/unmarshalError.test.ts b/packages/errors/src/marshal/unmarshalError.test.ts index 30d30a062..541f3895a 100644 --- a/packages/errors/src/marshal/unmarshalError.test.ts +++ b/packages/errors/src/marshal/unmarshalError.test.ts @@ -169,7 +169,7 @@ describe('unmarshalErrorOptions', () => { expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ stack: 'Error stack trace', - cause: new Error('A string cause'), + cause: 'A string cause', }); }); diff --git a/packages/errors/src/marshal/unmarshalError.ts b/packages/errors/src/marshal/unmarshalError.ts index 7edc69630..95ad5a8fa 100644 --- a/packages/errors/src/marshal/unmarshalError.ts +++ b/packages/errors/src/marshal/unmarshalError.ts @@ -19,18 +19,12 @@ export function unmarshalError( return errorClasses[marshaledError.code].unmarshal(marshaledError); } - let cause; - if (marshaledError.cause) { - cause = - typeof marshaledError.cause === 'string' - ? marshaledError.cause - : unmarshalError(marshaledError.cause); - } + const { cause, stack } = unmarshalErrorOptions(marshaledError); const error = new Error(marshaledError.message, { cause }); - if (marshaledError.stack) { - error.stack = marshaledError.stack; + if (stack) { + error.stack = stack; } return error; @@ -50,7 +44,7 @@ export function unmarshalErrorOptions( if (marshaledError.cause) { output.cause = typeof marshaledError.cause === 'string' - ? new Error(marshaledError.cause) + ? marshaledError.cause : unmarshalError(marshaledError.cause); } From 5b1d4dc63884fff0823788d5cd966946cec2ce8e Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 22 Oct 2024 18:21:16 +0200 Subject: [PATCH 7/8] rename test and remove default stack --- .../errors/src/marshal/{endoify.test.ts => marshal.test.ts} | 2 -- packages/errors/src/marshal/unmarshalError.test.ts | 6 ++---- packages/errors/src/marshal/unmarshalError.ts | 6 +++++- 3 files changed, 7 insertions(+), 7 deletions(-) rename packages/errors/src/marshal/{endoify.test.ts => marshal.test.ts} (94%) diff --git a/packages/errors/src/marshal/endoify.test.ts b/packages/errors/src/marshal/marshal.test.ts similarity index 94% rename from packages/errors/src/marshal/endoify.test.ts rename to packages/errors/src/marshal/marshal.test.ts index f8f0d843b..1ffaa8cea 100644 --- a/packages/errors/src/marshal/endoify.test.ts +++ b/packages/errors/src/marshal/marshal.test.ts @@ -1,5 +1,3 @@ -import '@ocap/shims/endoify'; - import { describe, it, expect } from 'vitest'; import { marshalError } from './marshalError.js'; diff --git a/packages/errors/src/marshal/unmarshalError.test.ts b/packages/errors/src/marshal/unmarshalError.test.ts index 541f3895a..db1dce15c 100644 --- a/packages/errors/src/marshal/unmarshalError.test.ts +++ b/packages/errors/src/marshal/unmarshalError.test.ts @@ -194,14 +194,12 @@ describe('unmarshalErrorOptions', () => { }); }); - it('should return default stack when stack is undefined', () => { + it('should not return stack when stack is undefined', () => { const marshaledError = { [ErrorSentinel]: true, message: 'An error occurred.', } as const; - expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({ - stack: '', - }); + expect(unmarshalErrorOptions(marshaledError)).toStrictEqual({}); }); }); diff --git a/packages/errors/src/marshal/unmarshalError.ts b/packages/errors/src/marshal/unmarshalError.ts index 95ad5a8fa..d0cd12c0d 100644 --- a/packages/errors/src/marshal/unmarshalError.ts +++ b/packages/errors/src/marshal/unmarshalError.ts @@ -39,7 +39,11 @@ export function unmarshalError( export function unmarshalErrorOptions( marshaledError: MarshaledError, ): ErrorOptionsWithStack { - const output: ErrorOptionsWithStack = { stack: marshaledError.stack ?? '' }; + const output: ErrorOptionsWithStack = {}; + + if (marshaledError.stack) { + output.stack = marshaledError.stack; + } if (marshaledError.cause) { output.cause = From f5deb5ad44195588c521a67db66dab0cad3f71bd Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 22 Oct 2024 18:23:08 +0200 Subject: [PATCH 8/8] clean --- packages/errors/package.json | 1 - packages/errors/vitest.config.ts | 7 ------- yarn.lock | 1 - 3 files changed, 9 deletions(-) diff --git a/packages/errors/package.json b/packages/errors/package.json index e9c065311..bb358bcf2 100644 --- a/packages/errors/package.json +++ b/packages/errors/package.json @@ -51,7 +51,6 @@ "@metamask/eslint-config": "^14.0.0", "@metamask/eslint-config-nodejs": "^14.0.0", "@metamask/eslint-config-typescript": "^14.0.0", - "@ocap/shims": "workspace:^", "@ocap/test-utils": "workspace:^", "@ts-bridge/cli": "^0.5.1", "@ts-bridge/shims": "^0.1.1", diff --git a/packages/errors/vitest.config.ts b/packages/errors/vitest.config.ts index b43a3b78a..de9febbf4 100644 --- a/packages/errors/vitest.config.ts +++ b/packages/errors/vitest.config.ts @@ -14,13 +14,6 @@ const config = mergeConfig( test: { pool: 'vmThreads', setupFiles: path.resolve('../shims/src/endoify.js'), - alias: [ - { - find: '@ocap/shims/endoify', - replacement: path.resolve('../shims/src/endoify.js'), - customResolver: (id) => ({ external: true, id }), - }, - ], }, }), ); diff --git a/yarn.lock b/yarn.lock index dc42959d8..8312a704f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1471,7 +1471,6 @@ __metadata: "@metamask/eslint-config-typescript": "npm:^14.0.0" "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^9.3.0" - "@ocap/shims": "workspace:^" "@ocap/test-utils": "workspace:^" "@ts-bridge/cli": "npm:^0.5.1" "@ts-bridge/shims": "npm:^0.1.1"