Skip to content
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

fix(swingset): startVat(vatParameters) are now capdata #4839

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

warner
Copy link
Member

@warner warner commented Mar 14, 2022

The startVat() message accepts "vat parameters", but previously these were
arbitrary (inert) data. This changes the argument to contain capdata.
Liveslots will unserialize this data into the vatParameters argument
given to buildRootObject().

The kernel was updated to populate this field with capdata in all pathways
that create create/start/upgrade-vat events. Parameters passed in through
config (which must be inert data) are serialized into capdata.

Parameters passed through E(vatAdminService).createVat(bundle, { vatParameters }) are also treated as inert data and serialized into capdata,
however when we move to "device hooks", this will change, and dynamic vat
creators will be able to pass object references in those parameters.

refs #4381

@warner warner added the SwingSet package: SwingSet label Mar 14, 2022
@warner warner requested a review from FUDCo March 14, 2022 20:38
@warner warner self-assigned this Mar 14, 2022
@warner warner force-pushed the 4381-slots-in-vatparameters branch from 7d7e030 to 219d851 Compare March 14, 2022 23:09
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly ok, modulo a couple of comments about how you're using capdata that might warrant some refactoring after giving it a thought.

@@ -87,8 +87,9 @@ export function initializeKernel(config, hostStorage, verbose = false) {
const vatKeeper = kernelKeeper.provideVatKeeper(vatID);
vatKeeper.setSourceAndOptions({ bundleID }, creationOptions);
vatKeeper.initializeReapCountdown(creationOptions.reapInterval);
const vpCapData = { body: stringify(harden(vatParameters)), slots: [] };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This recurring idiom of using stringify inside a hand-constructed capData object feels ugly. Can you not get the same result using serialize with marshal having been given no-op convertSlotToVal and convertValToSlot functions. (Even better: instead of no-op, have them throw; this will ensure that you don't accidentally try to pass remotables before the machinery is implemented to do that).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's precisely what import { stringify } from '@endo/marshal' does: https://github.com/endojs/endo/blob/master/packages/marshal/src/marshal-stringify.js : it uses makeMarshal(doNotConvertValToSlot, doNotConvertSlotToVal) to produce a stringify/parse pair that take the body and not slots.

It's confusing (but understandable) that @endo/marshal exports the same stringify and parse names that live on the built-in JSON object. In one sense, the Marshal forms are like a super-powered JSON object (they can handle undefined and BigInts). In another, they are weaker (they bail on anything shaped like a Remotable or a Promise, whereas plain JSON would at least emit something).

But I completely agree that this feels ugly, specifically that I'm switching back and forth between three forms of representation: the userspace objects (i.e. something that holds a Presence), the intermediate qclass form (something that holds an object { "@qclass": "slot" } and references a slot index), and the stringified form (something that holds a string '{"@qclass":"slot"}') we call "CapData".

Both the userspace objects and the intermediate qclass form have the same shape: serializing an array of Presences will give you an array of {"@qclass":"slot"} objects.

In this case (initializeKernel), the config object holds the userspace form, except that it's so early (and not inside a vat) that there are no Presences anyways. So we use Marshal's stringify to convert all the way down into CapData. This accomodates config objects that hold BigInts or -Infinity or other nonsense.

Later, when I file the PR to make the vat-admin kernel hook extract fields like vatParameters out of options, I'll have to convert from CapData to the intermediate qclass form, then slice up arrays and records, then convert back into CapData. I'll be using JSON.parse and JSON.stringify for those, because I can't use Marshal's tools (the data does contain Remotables, I just want to leave them in their intermediate form, and the kernel never ever holds a Presence).

I'll try to find a way to make this clearer in the subsequent PRs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. I see stringify being used to generate the body property of a hand-assembled capdata with an empty slots array. Seems like you should be able to generate the whole capdata object with a serialize(harden(vatParameters)) call, with the (suitably configured) marshaler ensuring that vatParameters doesn't contain anything that would end up in the slots array.

function doStartVat(vatParametersCapData) {
insistCapData(vatParametersCapData);
assert(vatParametersCapData.slots.length === 0, 'comms got slots');
const vatParameters = parse(vatParametersCapData.body) || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto prior comment, only this time with unserialize

@warner warner force-pushed the 4381-slots-in-vatparameters branch from 219d851 to 7c95b97 Compare March 18, 2022 00:17
The `startVat()` message accepts "vat parameters", but previously these were
arbitrary (inert) data. This changes the argument to contain capdata.
Liveslots will `unserialize` this data into the `vatParameters` argument
given to `buildRootObject()`.

The kernel was updated to populate this field with capdata in all pathways
that create create/start/upgrade-vat events. Parameters passed in through
config (which must be inert data) are serialized into capdata.

Parameters passed through `E(vatAdminService).createVat(bundle, {
vatParameters })` are also treated as inert data and serialized into capdata,
however when we move to "device hooks", this will change, and dynamic vat
creators will be able to pass object references in those parameters.

refs #4381
@warner warner force-pushed the 4381-slots-in-vatparameters branch from 7c95b97 to 0d273c9 Compare March 18, 2022 17:17
@warner warner merged commit 4d6d266 into master Mar 18, 2022
@warner warner deleted the 4381-slots-in-vatparameters branch March 18, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants