-
Notifications
You must be signed in to change notification settings - Fork 5
feat(ocap-kernel): use kernel platforms #615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e464774
to
0c2d4a8
Compare
return message; | ||
}, | ||
async fetch(url) { | ||
const response = await fetch(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constrained fetch is endowed i.e. available in the global namespace of the vat.
9366a07
to
1bd527a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...platformEndowments, | ||
// Important, liveslots endowments should be last so they override | ||
...lsEndowments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just throw an error in case of collisions here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. That would be less silent. Since the comment only makes explicit the strategy we already employed, I think this can be its own (small) issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying platform endowments that collide with inescapable global properties should be forbidden. I would prefer if we make this validity check here, or at least create a high-priority issue to it immediately after this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a start here, and welcome further discussion and finer requirements.
1bd527a
to
9e05cef
Compare
id: vatId, | ||
kernelStream, | ||
logger: logger.subLogger(vatId), | ||
makePlatform, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we can do a dynamic import and use it directly inside VatSupervisor
like we do here (you can make a utility isNodeEnv()
)? It's not ideal that we are making users do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are making users do this.
There is certainly a tradeoff between explicitness and simplicity. IMO we should prefer statically analyzable dependencies for the TCB, which in our model includes the worker files. The vat usercode is outside the TCB, and remains simple, i.e. agnostic to these choices.
That said, we should package together choices which must be made jointly but are kept separate for maintainability. See makeNodeJsVatSupervisor
for an ergonomic alternative to new VatSupervisor({ ... })
.
We can ultimately offer a handful of out-of-the-box workers that provide most users with an ocap kernel that 'just works'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this is partially my fault. On second thought, I think we shouldn't force users to specify platform capabilities (i.e. it should be an optional feature). It's not ideal that you can specify a platform config to a vat that cannot satisfy it, but that's okay for now. We should probably store some kind of vat metadata on the kernel side to know which platform capabilities a vat has, if any.
Here's a diff to make the makePlatform
argument optional again, but more succinctly:
diff --git a/packages/ocap-kernel/src/VatSupervisor.ts b/packages/ocap-kernel/src/VatSupervisor.ts
index 43e3a194..7e40255f 100644
--- a/packages/ocap-kernel/src/VatSupervisor.ts
+++ b/packages/ocap-kernel/src/VatSupervisor.ts
@@ -41,7 +41,7 @@ type SupervisorConstructorProps = {
id: VatId;
kernelStream: DuplexStream<JsonRpcMessage, JsonRpcMessage>;
logger: Logger;
- makePlatform: PlatformFactory;
+ makePlatform?: PlatformFactory;
platformOptions?: Record<string, unknown>;
vatPowers?: Record<string, unknown> | undefined;
fetchBlob?: FetchBlob;
@@ -105,7 +105,9 @@ export class VatSupervisor {
kernelStream,
logger,
vatPowers,
- makePlatform,
+ makePlatform = () => {
+ throw new Error('No platform capabilities provided');
+ },
platformOptions,
fetchBlob,
}: SupervisorConstructorProps) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably store some kind of vat metadata on the kernel side to know which platform capabilities a vat has, if any.
We store the vatConfig for each vat, and the platformConfig is a part of the vatConfig.
This could use a test for the error thrown since 1099f48. Once that and the merge conflicts are fixed I'll approve! |
I think it is due to an architectural constraint that we don't unit test |
13162c4
to
88f8f25
Compare
88f8f25
to
5017064
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
#633) Closes: #589 Refs: #610 #615 This PR introduces a new `@ocap/nodejs-test-workers` package and provides an integration test of the `@ocap/kernel-platforms` endowment functionality. ## Changes ### `@ocap/nodejs-test-workers` - Exports the `getWorkerFile` function, which maps a name to a built worker - Defines the `mock-fetch` worker, which mocks fetch using the platformOptions parameter ### `@ocap/kernel-test` - Adds a test that the kernel endows a vat with a caveated fetch capability
This PR integrates the
@ocap/kernel-platforms
package across the various monorepo packages which should use it, enabling platform-specific capabilities and endowments for vats. TheplatformOptions
parameter is supplied by the vat worker constructing the vat's supervisor, while theplatformConfig
parameter is delivered from the kernel as part of the vat initialization command.Key Changes
Note to Reviewers
This diff shows how to endow a host-constrained fetch capability to a vat.