Dynamically create new vats. #410

Chris-Hibbert · 2020-01-13T02:02:22Z

Doesn't handle termination.

Includes updates to tests.

The base change for the vatAdmin changes is here. See this discussion for context.

warner · 2020-01-14T21:50:00Z

It looks like you disabled a whole bunch of tests.. was that intentional?

Chris-Hibbert · 2020-01-14T22:19:24Z

Many non-SES tests failed, and I remembered @warner saying something indicating he that was likely ahead of time. As discussed off-line, Brian wasn't saying that was acceptable, so I'll go back and figure out what's necessary to re-enable them.

warner

ok, I think there are some non-critical changes we can make to clean things up, but overall it looks sound and correct. I'm most interested in --without-ses working, and then my next priority is code cleanup so we can read this better in the future, followed by handling syntax errors in the new vat's code somehow.

packages/SwingSet/src/kernel/vatAdmin/vatAdmin.js

packages/SwingSet/src/kernel/vatAdmin/vatAdminWrapper.js

packages/SwingSet/src/kernel/vatAdmin/vatAdmin-src.js

packages/SwingSet/src/kernel/kernel.js

Chris-Hibbert

Since you looked at the code before, my recommendation is not to look at the new individual commits, but instead to review the entire PR as a diff against master.

packages/SwingSet/src/kernel/kernel.js

packages/SwingSet/src/kernel/vatAdmin/vatAdmin-src.js

packages/SwingSet/src/kernel/vatAdmin/vatAdmin.js

packages/SwingSet/src/kernel/vatAdmin/vatAdminWrapper.js

packages/SwingSet/src/kernel/kernel.js

packages/SwingSet/src/controller.js

warner · 2020-01-22T01:24:51Z

hm, could you make two changes to the patch series:?

combine the "add test.skip to all --no-ses tests" and the "remove test.skip from all --no-ses tests" patches, so they cancel out and disapear. I tried to rebase this to trunk and got conflicts in e.g. Zoe, which isn't touched in the overall diff, because of fleeting changes that then got undone. Like virtual particles caroming off an event horizon (or not). Should be a job for git rebase --interactive
rebase this to current master

Doesn't handle termination. Includes updates to tests. No longer skips non-SES tests to simplify merging. This means non-SES tests are broken.

…s dropped in the merge.

It has lots of internal state that doesn't need to leak.

and update tests.

Test changes have been split into a separate commit, since they invert most of an earlier commit, and can be merged more cleanly separately.

Various other responses to review comments included.

Handle errors during vat creation (and add a test case). Simplify registration of vat admin functions with vatAdmin Device More refactoring for readability

Chris-Hibbert · 2020-01-23T01:55:35Z

There was a request to handle syntax errors in the new vat's code. See the brokenVatTest to decide whether you like the approach.

I merged out the skipping of non-SES tests, since it touched so many files. That meant that many intermediate commits wouldn't pass tests, but that's probably okay at this point.

This has now been rebased to current master.

warner

I like where this is going!

packages/SwingSet/src/kernel/kernel.js

packages/SwingSet/src/kernel/vatAdmin/vatAdmin-src.js

packages/SwingSet/src/kernel/vatAdmin/vatAdminWrapper.js

Create serialized string directly when notifying adminVat. Various renamings and comment clarifications. Provide vat control functions directly, rather than providing an accessor. Remove adminStats framework, since it's not complete here. Coming soon.

Chris-Hibbert · 2020-01-23T19:26:55Z

I'll make separate PRs to 1) switch to imports rather than require() and 2) renaming kernelKeeper's
provide... functions.

I didn't add a distinct notification for the error case in vat creation. The adminVat finds out
immediately, and doesn't create a promise that would need to be rejected.

warner

ok, let's land this now, and fix those two remaining issues (#444 and #445) in a second pass. Thanks!

dckc · 2020-01-23T23:25:19Z

packages/SwingSet/src/controller.js

@@ -19,6 +19,9 @@ import { insistCapData } from './capdata';
 import { parseVatSlot } from './parseVatSlots';
 import { buildStorageInMemory } from './hostStorage';

+const ADMIN_DEVICE_PATH = require.resolve('./kernel/vatAdmin/vatAdmin-src');


darn. More use of require.resolve. (ambient authority grumble... and doesn't work on xs)

I wish I had taken a look sooner.

Chris-Hibbert · 2020-01-23T23:29:57Z

@dckc what would work better here? I wasn't happy with this, but I didn't find anything else that allowed the kernel to get access to source code to use for a built-in device and vat.

dckc · 2020-01-23T23:36:15Z

I hope import() works out better. I see discussion of further work in that direction above.

In particluar, import() skips access to the source code and just gives the exports back.

I suppose it doesn't help that much with the relative path resolution fuctionality of require.resolve.

If we have to expose reading source code from files in our APIs, we should use filesystem access objects, a la java's File or the Emily file API. Not ambient fs.readFileSync() access. See #66 (comment) and following.

warner · 2020-01-24T01:49:26Z

I'd rather pass source code in to the SES realm/kernel (as a string) than pass in file-reading ability. But yeah, I think proper modules and import statements (note: not dynamic import() expressions) will help here.

Chris-Hibbert mentioned this pull request Jan 13, 2020

marker commit for vatAdmin branch #407

Closed

7 tasks

Chris-Hibbert force-pushed the provideVatId branch 2 times, most recently from 7fd2259 to e8cda0a Compare January 13, 2020 02:49

Chris-Hibbert force-pushed the createDynamicVats branch 2 times, most recently from 32b77a5 to f7c09eb Compare January 13, 2020 05:21

Chris-Hibbert requested a review from warner January 13, 2020 05:45

warner requested changes Jan 15, 2020

View reviewed changes

Chris-Hibbert commented Jan 18, 2020

View reviewed changes

Chris-Hibbert requested a review from warner January 20, 2020 20:29

warner reviewed Jan 20, 2020

View reviewed changes

packages/SwingSet/src/controller.js Show resolved Hide resolved

warner added the SwingSet package: SwingSet label Jan 21, 2020

warner mentioned this pull request Jan 22, 2020

Mostly "lint-fix"es. Remove some jsx and react dependencies #366

Merged

Chris-Hibbert force-pushed the createDynamicVats branch from 01d663f to ac545fc Compare January 23, 2020 01:32

Chris-Hibbert added 14 commits January 22, 2020 17:35

Dynamically create new vats.

66f9579

Doesn't handle termination. Includes updates to tests. No longer skips non-SES tests to simplify merging. This means non-SES tests are broken.

eslint fixes

8ae976b

drop unnecessary import of harden. cleanup a test.only

273ba96

drop extraneous TODO

2272d43

device has no need for adminId. I removed this once before, but it wa…

b82399f

…s dropped in the merge.

extract serialize() from createVatAndNotify()

0e40ce1

It has lots of internal state that doesn't need to leak.

Wrapper vat doesn't need adminId either. Remove it.

3813d85

Remove some unnecessary lines indicated in code review.

7e8f79e

and update tests.

Make non-SES path work.

7340546

Test changes have been split into a separate commit, since they invert most of an earlier commit, and can be merged more cleanly separately.

Tests for non-SES path.

493394b

More tests fixes for making the non-SES path work.

1bad36e

Mostly making non-SES vat creation work, and re-enabling tests.

8fc56c5

Various other responses to review comments included.

Simplify vat admin function registration; handle vat creation errors

fbb2447

Handle errors during vat creation (and add a test case). Simplify registration of vat admin functions with vatAdmin Device More refactoring for readability

Minor clean-ups.

243ea80

Fix a problem eslint was unhappy about.

77820e3

Chris-Hibbert force-pushed the createDynamicVats branch from ac545fc to 77820e3 Compare January 23, 2020 01:36

Chris-Hibbert changed the base branch from provideVatId to master January 23, 2020 01:37

Chris-Hibbert requested a review from warner January 23, 2020 01:55

warner requested changes Jan 23, 2020

View reviewed changes

Changes for review comments.

a9b4213

Create serialized string directly when notifying adminVat. Various renamings and comment clarifications. Provide vat control functions directly, rather than providing an accessor. Remove adminStats framework, since it's not complete here. Coming soon.

Chris-Hibbert requested a review from warner January 23, 2020 19:27

This was referenced Jan 23, 2020

swingset kernel: avoid use of require #444

Closed

improve swingset dynamic-vat API: return errors with rejected promise, not string #445

Closed

warner approved these changes Jan 23, 2020

View reviewed changes

Chris-Hibbert force-pushed the createDynamicVats branch from b669f53 to a9b4213 Compare January 23, 2020 22:46

Chris-Hibbert merged commit 4bde699 into master Jan 23, 2020

Chris-Hibbert deleted the createDynamicVats branch January 23, 2020 23:16

dckc reviewed Jan 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically create new vats. #410

Dynamically create new vats. #410

Chris-Hibbert commented Jan 13, 2020 •

edited

Loading

warner commented Jan 14, 2020

Chris-Hibbert commented Jan 14, 2020

warner left a comment

Chris-Hibbert left a comment

warner commented Jan 22, 2020

Chris-Hibbert commented Jan 23, 2020

warner left a comment

Chris-Hibbert commented Jan 23, 2020

warner left a comment

dckc Jan 23, 2020

Chris-Hibbert commented Jan 23, 2020

dckc commented Jan 23, 2020

warner commented Jan 24, 2020

Dynamically create new vats. #410

Dynamically create new vats. #410

Conversation

Chris-Hibbert commented Jan 13, 2020 • edited Loading

warner commented Jan 14, 2020

Chris-Hibbert commented Jan 14, 2020

warner left a comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

warner commented Jan 22, 2020

Chris-Hibbert commented Jan 23, 2020

warner left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Jan 23, 2020

warner left a comment

Choose a reason for hiding this comment

dckc Jan 23, 2020

Choose a reason for hiding this comment

Chris-Hibbert commented Jan 23, 2020

dckc commented Jan 23, 2020

warner commented Jan 24, 2020

Chris-Hibbert commented Jan 13, 2020 •

edited

Loading