feat(kernel): Differentiate cluster, kernel and vat command types.

[grypez]

Changes

@ocap/extension

• Prefixes various method and type names with the actor to which they pertain.
• Imports the type definition VatId from @ocap/kernel instead of redefining it in shared.ts.

@ocap/kernel

• More strongly distinguishes between vat ids and vat message ids by enforcing a string pattern for each.¹
• The params of command and command reply types are now checked with respect to the method.
• Dissects command.ts into messages/<actor>.ts and messages/<actor>-test.ts files for cluster, kernel and vat actors.
• Implements a message-kit.ts file to denoise the command schema declarations in the above.
• CapTP types and type guards are moved into messages/captp.ts
• The command schemas and message types are collected into messages.ts to promote legibility of index.ts.

@ocap/utils

• The GuardType type is relocated from @ocap/extension and renamed ExtractGuardType.

Note to reviewers

The message-kit.ts file and the precise interface it introduces are likely to be replaced by @metamask/superstruct in the near future. Since the behavior is compile-time checked, reviewers are encouraged to consider the resulting declarations of messages/kernel-test.ts, messages/vat.ts, etc. as a preview of the intended effect and may be forgiven for reviewing the type transformations in message-kit.ts and utils.ts with reserved interest.

Footnotes

1. The exact pattern is not important to me. I found that when I made this change it revealed some discrepancies in the types and their usage that had snuck through due to these types being merely aliases of string. ↩

[grypez]

For now, the kernel commands are purely test commands. In a future PR under #123, I will add an InitKernel command here.

[rekmarks]

• Expunges @typescript-eslint/no-redundant-type-constituents with prejudice. This rule forbids $\cup$ operations on types unless they can be rewritten as $\sqcup$ operations, which is an overzealous design constraint.

As discussed here, I don't believe that this is overzealous. The resulting "unions" only end up erasing the more specific types, which can be misleading. Given:

type A = string;
type B = number;
// Elsewhere
type X = unknown;

// In a third location:
type MyUnion = A | B | X;

MyUnion is just unknown. This is bad, and I think we should keep the rule enabled. For further examples, see the rule's documentation.

Edit: Another reason to keep the rule: re-enabling it on this PR doesn't seem to cause any lint errors.

[rekmarks]

Although we could probably live with it if we had to due to tree-shaking, we should avoid entraining lodash if possible, and for the uses in this PR, it is definitely possible. We can discuss whether to upstream any desired utilities to @metamask/utils (or our own utils package), but IMO typeof x === 'string' and .toUpperCase() are good enough.

[rekmarks]

Rather than relying on capitalizing the first letter after the v, we could just add a "special" character to form e.g. v:, v_, v# etc. We shouldn't use -, because some UUID schemes we may use that character.

However, that being said, I just realized that we discussed only using incrementing IDs anyway, in which case it would be completely feasible to do something like:

Suggested change

	export type VatId = `v${Capitalize<string>}`;
	export type VatId = `v${number}`;

[grypez]

I like incremental.

type VatId = `v${number}`;
type VatMessageId = `${VatId}:${number}`;

[grypez]

• Expunges @typescript-eslint/no-redundant-type-constituents with prejudice. This rule forbids
  ∪
  operations on types unless they can be rewritten as
  ⊔
  operations, which is an overzealous design constraint.

As discussed here, I don't believe that this is overzealous. The resulting "unions" only end up erasing the more specific types, which can be misleading. Given:

type A = string;
type B = number;
// Elsewhere
type X = unknown;

// In a third location:
type MyUnion = A | B | X;

MyUnion is just unknown. This is bad, and I think we should keep the rule enabled. For further examples, see the rule's documentation.

Edit: Another reason to keep the rule: re-enabling it on this PR doesn't seem to cause any lint errors.

I understand your concern that in this case the union renders pointless the types for A and B which we defined with care. But if we are using the union operator correctly then in your example I think MyUnion should be unknown. The top of the type poset is unknown, and a join with the top is always the top. I point the finger in your example, if one must be pointed, at the definition of X being too wide. However, if the correct type of X is unknown, then the correct type of MyUnion is unknown, too.

But besides that, the rule incorrectly asserts that the resulting types are any (which is considerably less safe than unknown) when it finds a redundant type constituent, even if that redundant type constituent is a result of how typescript resolves the types, not how we declare them.

Observed Lint Error

Explanation (kernel-worker.ts)

The type of KernelCommand is not any.

type KernelCommand = {
    method: "evaluate";
    params: string;
} | {
    method: "ping";
    params: null;
} | {
    method: "capTpCall";
    params: CapTpPayload;
} | {
    method: "kVSet";
    params: {
        key: string;
        value: string;
    };
} | {
    method: "kVGet";
    params: string;
}

And in the offending lines,

type KernelWorkerCommand = KernelCommand & {
  method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet;
};

the type of KernelWorkerCommand is in fact narrower than KernelCommand, but typescript distributes the union of allowed methods as below.

type KernelWorkerCommand = ({
    method: "kVSet";
    params: {
        key: string;
        value: string;
    };
} & {
    method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet;
}) | ({
    method: "kVGet";
    params: string;
} & {
    method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet;
})

I believe the linter is picking up on this constituent appearing twice in typescript's resolution.

{
    method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet;
}

But as intended, the type of KernelWorkerCommand is equivalent to the following.

type KernelWorkerCommand = {
    method: "kVSet";
    params: {
        key: string;
        value: string;
    };
) | {
    method: "kVGet";
    params: string;
}

The resulting type is not any (or unknown), the KernelCommand type does not override all other types in the intersection, and I think in this slightly more but not unreasonably complex example, this shows the rule is overzealous.

If you still prefer @typescript-eslint/no-redundant-type-constituents for the safety it imparts to the simpler cases, I can just disable it here inline.

[grypez]

This change is made to avoid a cyclic dependency between types.ts and messages/vat-message.ts.

[rekmarks]

in your example I think MyUnion should be unknown.

@grypez I completely agree with this statement. My point is not about the behavior of TypeScript in this case, which I have no qualms with, but the legibility of our code. Specifically: when a union causes type specificity to be lost, we should just replace the union with the resulting type. This is especially true in cases where the result is unknown, number, string, etc.

For your specific example, we can get around the problem via the Extract utility type:

type KernelWorkerCommand = Extract<
  KernelCommand,
  { method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet }
>;

As you will see, Extract also results in a cleaner. I often resort to it and its inverse Exclude, their cousins Pick and Omit, and various others from the "official" utility types.

Does this make sense?

[grypez]

For your specific example, we can get around the problem via the Extract utility type:

type KernelWorkerCommand = Extract<
  KernelCommand,
  { method: typeof KernelCommandMethod.KVSet | typeof KernelCommandMethod.KVGet }
>;

As you will see, Extract also results in a cleaner. I often resort to it and its inverse Exclude, their cousins Pick and Omit, and various others from the "official" utility types.

Does this make sense?

That looks how I want it to look. I'll prefer these utilities going forward.

[sirtimid]

I'm approving but please wait for @rekmarks approval as well.

Also some types like in ./src/packages/kernel/src/messages/kernel.ts rely on a "test" temporary file (is it temporary?). Should/do we have an issue to fix this?

[grypez]

some types like in ./src/packages/kernel/src/messages/kernel.ts rely on a "test" temporary file (is it temporary?). Should/do we have an issue to fix this?

This may be handled differently under #120; these <actor>-test files pull out the commands that we are currently making available in the dev-console for testing purposes, but which we do not intend for production.

[rekmarks]

I'm investigating this locally, and it appears that KernelCommandReplyFunction<Promise<void>> evaluates to:

(alias) type KernelCommandReplyFunction<Return> = ((method: "evaluate", params: string) => Return) & ((method: "ping", params: "pong") => Return) & ((method: "capTpCall", params: string) => Return) & ((method: "kVSet", params: string) => Return) & ((method: "kVGet", params: string) => Return)

Note the intersection (&) joiners between the specific function types. It seems impossible for any value to simultaneously satisfy an intersection between distinct string literals. I'm worried that we have to throw in this @ts-expect-error here seems to signal a fundamental error with the message kit types.

Now, if we remove the usage of UnionToIntersection in order to replace the intersection type with a union (|) type, we are still in trouble because the method param of replyToBackground() becomes never instead. This is surprising to me and I can't immediately explain why this is happening.

[rekmarks]

I think it's the string literal type "pong" in the result for ping. If we refactor to an object union type, it all works without casts in this file:

  type KernelCommandReply =
  | { method: "evaluate"; params: string }
  | { method: "ping"; params: string }
  | { method: "capTpCall"; params: string }
  | { method: "kVSet"; params: string }
  | { method: "kVGet"; params: string };

const replyToBackground = async (reply: KernelCommandReply) => {
  await backgroundStreams.writer.next({
    method: reply.method,
    params: reply.params,
  });
};

[rekmarks]

I cannot get the original KernelCommandReplyFunction, ultimately derived from MessageFunction, to work, however. Not even if I remove UnionToIntersection in message-kit.ts.

[grypez]

Per our out of band chat, we should probably just use a typed message object as the argument to the reply function. It is the simplest implementation that definitely works.

[rekmarks]

I approve of this, with the caveat that I'm uncertain whether we are getting our money's worth from the message kit utility. For example:

• most of the *CommandFunction and *CommandReplyFunction types are not used
• it's possible that we could achieve the same degree of type safety with less complex types (less generics, less dependencies between types)

We should examine whether we can make any such simplifications before proceeding #131.