feat: Custom Error Classes with Error Codes #149
Conversation
There was a problem hiding this comment.
Very clean 💯. I wonder if it would be preferable to declare the kernel specific errors in or next to the code that throws them. The constructors could then be more strongly typed (i.e. vatId: VatId instead of vatId: string), and whatever code imports the throwing routine can also import the error from the same place.
What do you think?
@grypez We already went this path the first time and decided to move them on their own package so we have all the codes exported centrally in one place. But your point is valid and I am thinking that in order to avoid circular dependencies we will do more acrobatics like this, instead of having a |
packages/errors/src/constants.ts
Outdated
| CaptpConnectionExists = 'CAPTP_CONNECTION_EXISTS', | ||
| CaptpConnectionNotFound = 'CAPTP_CONNECTION_NOT_FOUND', |
There was a problem hiding this comment.
These are only thrown by the vat. I think it would be virtuous if each error is associated with the component that throws it. Perhaps that's not feasible in practice (because the same error can be thrown in multiple places). What do you think?
There was a problem hiding this comment.
I named this thinking that it can be used elsewhere, but it also makes sense to have distinct errors for each component, even if they refer to errors of the same service. I renamed it.
There was a problem hiding this comment.
Oh, one minor thing we should settle on actually:
This PR capitalizes "CapTP" differently in different places. We should determine our canonical capitalization, and apply that across the entire repo with a find-and-replace.
I think the least bad / most conventional option for MM is capTp / CapTp, and I recommend applying that. We can live with imports from endo being CapTP.
If we go for CapTP as our capitalization, we should avoid names that start with "CapTP" because capTP looks stupid. 😛
sirtimid
force-pushed
the
sirtimid/custom-error-classes
branch
from
5c92d43 to
4e01ba1
Compare
packages/errors/src/errors.ts
Outdated
| export class SupervisorReadError extends BaseError { | ||
| constructor(supervisorId: string, originalError: Error) { | ||
| super( | ||
| ErrorCode.SupervisorReadError, | ||
| 'Unexpected read error from Supervisor.', | ||
| { | ||
| supervisorId, | ||
| }, | ||
| originalError, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export class VatReadError extends BaseError { | ||
| constructor(vatId: string, originalError: Error) { | ||
| super( | ||
| ErrorCode.VatReadError, | ||
| 'Unexpected read error from Vat.', | ||
| { | ||
| vatId, | ||
| }, | ||
| originalError, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm wondering if it's worthwhile to differentiate between stream read errors based on where they occur. Perhaps we should create a StreamReadError and let the stack trace / data property identify the location?
For instance, we could have data as:
type Location = { vat: VatId } | { supervisor: SupervisorId };
type Data = { location: Location };There was a problem hiding this comment.
That sounds reasonable 👍
|
See previous inline comment, then I otherwise only have this nit: #156 |
closes #95
This PR adds custom error classes with unique error codes. It establishes the base error class, and use that to createerrors in kernel package classes Vat, Kernel and Supervisor.