refactor(extension): Extract kernel and vat logic into new package by sirtimid · Pull Request #79 · MetaMask/ocap-kernel

sirtimid · 2024-09-19T16:17:09Z

Closes#56

Extracts kernel and vat logic from the extension to a new package, @ocap/kernel.

sirtimid · 2024-09-19T20:23:35Z

+} from './types.js';
+export { KernelMessageTarget, Command } from './types.js';
 export {
- makeMessagePortStreamPair,


Here I removed from the exports many unused methods and types, we should check if we still need everything in this package and then we should clean up a bit

rekmarks

A pleasing refactor! See comments.

rekmarks · 2024-09-20T01:06:35Z

+export enum KernelMessageTarget {
+ Background = 'background',
+ Offscreen = 'offscreen',
+ WebWorker = 'webWorker',
+ Node = 'node',
+}


The original ExtensionMessageTarget enum was intended specifically for the chrome.runtime.sendMessage() API. If we want an enum like KernelMessageTarget, it should reference things like vat, kernel, and console, not web extension components.
This problem is to some extent preexisting, because we're mixing different abstraction layers. Ideally, our notion of kernel commands, for example, shouldn't have any knowledge of platform implementation details.
After further consideration, I think we should just commit to creating an issue here, and address this in a follow-up. I'm tempted to take a stab at establishing a cleaner abstraction boundary.

I ll leave that for the next iteration then

It's not just that the contents of the enum is specific to its use with the chrome.runtime.sendMessage API, but the enum's very existence is an artifact of how that API works. Its message targeting scheme is not only gratuitously different from the other web message APIs (e.g., MessagePort.postMessage) but also very anti-ocap'y. It or something like it is arguably necessary as an under-the-hood implementation artifact, but I don't think it should be generalized the way Eric proposes. It's better to have some kind of object that represents where a message should go, which can be a component of a cleaner message passing API that layers over all the web crap. Ocaps are not just an abstraction for security purposes, they're a pathway to sounder software architecture and safer code.

I'm not sure that any code that references the specific componentry of our system belongs in the streams package at all, which feels to me like it should be completely generic. However, if the plan is for this enum to evaporate soon I imagine we can live with it being here for now.

grypez

Overall I'm a big kernel fan. I have a suggestion for how to remove a few ?s.

grypez · 2024-09-20T12:12:31Z

+ /**
+ * Adds a vat to the kernel.
+ *
+ * @param vat - The vat record.
+ */
+ public addVat(vat: Vat): void {
+ if (this.#vats.has(vat.id)) {
+ throw new Error(`Vat with ID ${vat.id} already exists.`);
+ }
+ this.#vats.set(vat.id, vat);
+ }


Do we imagine the kernel receiving vats it didn't create itself?
We have LaunchVat in the design doc, which can save us some async construction defensiveness by accepting the VatWorker, awaiting it, and passing the resolved stream to the Vat.
Suggested change
/**
*Addsavattothekernel.
*
* @paramvat-Thevatrecord.
*/
publicaddVat(vat: Vat): void{
if(this.#vats.has(vat.id)){
thrownewError(`Vat with ID ${vat.id} already exists.`);
}
this.#vats.set(vat.id,vat);
}
/**
*Launchesanewvat.
* @paramvat-Thevatrecord.
*/
publiclaunchVat({ id, worker }: Omit<VatProps,'streams'>): Promise<Vat>{
if(this.#vats.has(vat.id)){
thrownewError(`Vat with ID ${vat.id} already exists.`);
}
const[streams,]=awaitworker.init(id);
constvat=newVat({ id, worker, streams });
this.#vats.set(vat.id,vat);
awaitvat.init();
returnvat;
}
Possibly it is reasonable to withhold the worker from the vat entirely, and instead invoke the worker's delete method from the Kernel.

I like this approach, I m going to implement it, but I will pass the delete method in the vat I don't want to store it in the kernel

I think I like launchVat() (startVat()?), but curious to hear what the author has to say.
I do think it's a good idea to withhold the worker from the vat, and manage its lifecycle / bookkeeping outside of the vat. The vat doesn't need to know where or how it came into being, and the kernel has to directly manage its lifecycle (and implicitly the lifecycle of its worker) anyway.
Food for future thought: Ultimately, I think the kernel should be responsible for launching / starting a vat, and the vat object should be constructed in that method. However, I don't think that a worker should be passed to the kernel method responsible for this. Rather, we should establish a notion of a "worker service" that is passed to the kernel on construction time, from which it can request workers. The reason being: only the kernel can determine when it needs a new vat, at which point it needs to ask for a worker. We have an abstraction boundary similar to this in Snaps, and it's served us well there.

Ok let's do this one step at a time, first I will implement @grypez suggestion passing the worker on launchVat. The worker service can be done on another PR

@rekmarks @grypez I made the change

It can make total sense for a vat to be able to launch another vat, as it can be very useful in some applications, but it must do so via a capability (which the typical vat would not be given) that lets it request the kernel to start the second vat on its behalf, rather than by somehow creating the second vat directly. In other words, the kernel mediates all vat creation. The same logic applies to vat destruction.
Erik's remark:
The vat doesn't need to know where or how it came into being, and the kernel has to directly manage its lifecycle (and implicitly the lifecycle of its worker) anyway
Is spot on.

we should establish a notion of a "worker service" that is passed to the kernel on construction time, from which it can request workers
SwingSet has something called the vatLoader that abstracts away the low level mechanics of creating and launching a vat, with a sort of plug-in architecture for the actual implementation of each of the different ways this can happen. In SwingSet, there are three supported cases: an external NodeJS worker process, an external XSnap (XS) worker process, and running the vat locally within the kernel process itself. Production operations use the xs-worker and local vats (the latter for certain special inside-the-TCB vats such as comms), while the node-worker and local vats (used more generally) are very useful for debugging. The thing Erik describes sounds a lot like that. I suspect our thing will let us choose whether the vat is in a web worker or an iframe, and whether to run it natively or in xsnap wasm or something like that (I could imagine getting really fancy and having something like a special wasm stub that lets us launch an actual external OS process and run in that instead of running in the browser at all, but that seems highly speculative).

FUDCo

Big refactorings always tempt me towards infinitely subdividing fractal quibbles, but we'll get to those in due time. I call this a win.

Huge progress towards reshaping things into the way they ought to be. Bravo!

FUDCo · 2024-09-20T21:10:43Z

+export enum KernelMessageTarget {
+ Background = 'background',
+ Offscreen = 'offscreen',
+ WebWorker = 'webWorker',
+ Node = 'node',
+}


I'm not sure that any code that references the specific componentry of our system belongs in the streams package at all, which feels to me like it should be completely generic. However, if the plan is for this enum to evaporate soon I imagine we can live with it being here for now.

FUDCo · 2024-09-20T21:12:22Z

+type CommandMessage<TargetType extends KernelMessageTarget> =
 | CommandLike<Command.Ping, null | 'pong', TargetType>
 | CommandLike<Command.Evaluate, string, TargetType>
 | CommandLike<Command.CapTpInit, null, TargetType>
 | CommandLike<Command.CapTpCall, CapTpPayload, TargetType>;

-export type ExtensionMessage = CommandMessage<ExtensionMessageTarget>;
-export type IframeMessage = CommandMessage<never>;
+export type KernelMessage = CommandMessage<KernelMessageTarget>;
+export type VatMessage = CommandMessage<never>;

-export type WrappedIframeMessage = {
+export type WrappedVatMessage = {
 id: MessageId;
- message: IframeMessage;
+ message: VatMessage;


Similar to my comment above, I really don't think representations of specific message protocol elements belong in the streams package.

I've left all these types in place, either in anticipation of upcoming changes or with the expectation that @rekmarks will update them once this PR is merged, as he indicated.

grypez

A few more changes relevant to this issue's scope, but overall I like where the PR is at.

grypez

Only nits.

grypez · 2024-09-23T17:45:57Z

LGTM, pending others' reviews.

Changes addressed

rekmarks

Just a couple of nits.

rekmarks · 2024-09-23T19:47:35Z

+ await vat.terminate();
+ await worker.delete();
+ this.#vats.delete(id);


For future reference: we should consider what should happen when either of these promises reject, and whether to delete the vat from #vats anyway.

The command / message types have frustrated us on various occasions, e.g. [here](#79 (comment))). This is in part because they blur abstraction boundaries between the kernel and the platform it runs on. Here we attempt to delineate this abstraction boundary by extracting these types into a new "kitchen drawer" package (`@ocap/utils`), and refactoring them such that only a smaller set needs to be shared between different packages. In addition, the `Command`-family of types and enums receives an overhaul. Changes in detail: - Extracts use case-specific types and utilities from `@ocap/streams` into a new package, `@ocap/utils` - Extract single-package types into their respective packages from `@ocap/utils` - Rename command types, see `utils/src/types.ts` - The `Command` type itself blurs our ideal abstraction boundary by implementing low-level plumbing like `ping` alongside porcelain such as `callCapTp. Fixing this is deferred to future work. - Notably, the string enum of command names is now named `CommandMethod`, and the actual command payload `Command`. Various unnecessary generic parameters have been removed. --------- Co-authored-by: grypez <143971198+grypez@users.noreply.github.com>

sirtimid added 3 commits September 19, 2024 17:16

add kernel package
6139482

extract all code from extension and pass realm to vat
aedaab6

fix assertion
eed794e

sirtimid commented Sep 19, 2024
View reviewed changes

sirtimid marked this pull request as ready for review September 19, 2024 20:27

sirtimid requested a review from a team as a code owner September 19, 2024 20:27

sirtimid requested a review from rekmarks September 19, 2024 20:34

rekmarks requested changes Sep 20, 2024
View reviewed changes

fix tsconfigs, rename realm to worker and some sanity checks
9951d0b

grypez requested changes Sep 20, 2024
View reviewed changes

sirtimid added 3 commits September 20, 2024 15:18

fix stream tests
4dfef8d

add kernel tests
eae41e5

add index tests
be067ba

grypez reviewed Sep 20, 2024
View reviewed changes

Comment threadpackages/streams/src/type-guards.test.ts Outdated

sirtimid added 5 commits September 20, 2024 19:28

CHanged addVat to launchVat
8a6eda6

cleanup
4d962a1

add some space on test
f1356f9

remove unsused
84e3c7f

make type guard tests parameterized
b0c0c9a

FUDCo reviewed Sep 20, 2024
View reviewed changes

sirtimid added 3 commits September 20, 2024 23:36

add Vat tests
85ca274

Manage worker from kernel
00e9849

resolve endoify for tests
1edbfa0

sirtimid requested review from grypez and rekmarks September 20, 2024 23:46

remove types
430b2f4

grypez requested changes Sep 22, 2024
View reviewed changes

Comment threadpackages/kernel/src/Kernel.ts Outdated
Comment threadpackages/kernel/src/Kernel.ts Outdated
Comment threadpackages/kernel/src/Kernel.test.ts Outdated

grypez requested changes Sep 23, 2024
View reviewed changes

Comment threadpackages/kernel/src/Vat.ts Outdated
Comment threadpackages/kernel/src/Vat.ts Outdated

fixing vat tests
1eddbca

sirtimid requested a review from grypez September 23, 2024 16:21

grypez previously requested changes Sep 23, 2024
View reviewed changes

Comment threadpackages/kernel/src/Vat.test.ts Outdated
Comment threadpackages/kernel/src/Vat.test.ts Outdated
Comment threadpackages/kernel/src/Kernel.test.ts Outdated
Comment threadpackages/kernel/src/Kernel.test.ts Outdated

fix test typos
8df2e92

sirtimid requested a review from grypez September 23, 2024 17:16

sirtimid changed the title ~~feat(WIP): Add kernel package~~feat: Add kernel packageSep 23, 2024

rekmarks requested changes Sep 23, 2024
View reviewed changes

apply comments
6ec897c

sirtimid requested a review from rekmarks September 23, 2024 20:47

rekmarks changed the title ~~feat: Add kernel package~~refactor(extension): Extract kernel and vat logic into new packageSep 23, 2024

rekmarks enabled auto-merge (squash) September 23, 2024 21:23

rekmarks approved these changes Sep 23, 2024
View reviewed changes

rekmarks merged commit 0c52998 into mainSep 23, 2024

rekmarks deleted the sirtimid/kernel-vat-abstraction branch September 23, 2024 21:23

This was referenced Sep 23, 2024
refactor(kernel): Rename Vat to VatProxy#83
Closed
refactor: Extract message / command types into utils package #85
Merged

-/**
-*Addsavattothekernel.
-*
-* @paramvat-Thevatrecord.
-*/
-publicaddVat(vat: Vat): void{
-if(this.#vats.has(vat.id)){
-thrownewError(`Vat with ID ${vat.id} already exists.`);
-}
-this.#vats.set(vat.id,vat);
-}
+/**
+*Launchesanewvat.
+* @paramvat-Thevatrecord.
+*/
+publiclaunchVat({ id, worker }: Omit<VatProps,'streams'>): Promise<Vat>{
+if(this.#vats.has(vat.id)){
+thrownewError(`Vat with ID ${vat.id} already exists.`);
+}
+const[streams,]=awaitworker.init(id);
+constvat=newVat({ id, worker, streams });
+this.#vats.set(vat.id,vat);
+awaitvat.init();
+returnvat;
+}

Uh oh!

Conversation

sirtimid commented Sep 19, 2024• edited by rekmarks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sirtimidSep 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grypez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirtimidSep 20, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FUDCo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grypez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grypez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grypez commented Sep 23, 2024

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sirtimid commented Sep 19, 2024•
edited by rekmarks
Loading

sirtimidSep 19, 2024•
edited
Loading

sirtimidSep 20, 2024•
edited
Loading