-
Notifications
You must be signed in to change notification settings - Fork 6
Give each vat its own personality #229
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
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@18.19.55, npm/acorn@8.12.1, npm/undici-types@5.26.5 |
9bcc6d0 to
85ca41c
Compare
54732e2 to
b07ee5a
Compare
sirtimid
left a comment
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
Dynamically loads vat user code from a fetched bundle under control of a configuration object. Closes issue #203
eslint.config.mjs
Outdated
| allowExpressions: true, | ||
| }, | ||
| ], | ||
| 'no-useless-return': 'off', |
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.
Linting succeeds without this. In general we should also prefer // eslint-disable directives over disabling rules wholesale.
| 'no-useless-return': 'off', |
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 had a couple of returns that were deeply nested inside switch statements that, if you investigated deeply enough, did indeed turn out to be the last statements that would have been executed on their particular code paths, but it was far from obvious. Putting in an explicit return made things unambiguously clear. I submit that rather than being something to be selectively overridden on a case-by-case basis, the no-useless-return lint rule is simply a mistake and should not exist at all.
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'm sympathetic to that, although the rule also catches a number of other instances where the redundant returns only add noise. Either way, the change appears to be orthogonal to this PR in its current state, and I'd prefer if we don't change it here. If it comes up again, we can disable the rule in a separate PR.
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.
OK, I'll take it out, but I think it'll make the weird return whinge return, which I'll override (of course, now we'll find that the evolution of the code that has taken place in the meantime will have made that nugatory, because Murphy)
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.
Addendum to above: yup!
b07ee5a to
55d57af
Compare
rekmarks
left a comment
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!
Dynamically loads vat user code from a fetched bundle under control of a subcluster configuration spec object.
This still lacks CapTP message plumbing into and out of vats (that will be #29) and a way to dynamically specify or inject the configurations (possibly that's #204); right now a placeholder subcluster of 3 vats is statically configured for test and demo purposes.
For this to work the Supervisor needs to be able to load bundle files dynamically. This is currently done via HTTP using the JS
fetchcall, so to try this out you'll need have a simple web server running somewhere that can vend bundle files. In principle it should be possible to usefile://URLs, but for the moment this proved a bridge too far due to cargo cult browser security machinery that will need some kind of configuration voodoo to overcome, and figuring that out didn't seem like it belonged on the critical path.Also, of course, you'll need one or more code bundles for your vat(s). The current placeholder configuration expects to load a bundle of
sample-vat.jsfound inextension/src/kernel/vats. This PR includes a simple bundling tool in the repo rootscriptsdirectory that you can run usingyarn bundle <pathToEntryPointSource>, e.g.,yarn bundle extension/src/vats/sample-vat.js. Be aware that bundling is still a little sketchy; you might note that the aforementioned sample vat code sits in a directory with its own stubpackage.jsonfile. This is a workaround for a glitch in the Endo bundler, which apparently gets a bit confused by peer dependencies in the enclosing package dependency graph, which one of the@metamaskpackages we depend on has (Kris Kowal knows about this and is working on a fix).This also needs quite a bit more test love. Right now you can only test the user code loading stuff manually, by reloading the extension and looking at the console output. Presumably the appropriate test automation can be effectuated via some kind of browser mocking, but that's something that I think I need some help on from somebody who understands browser frontend test stuff much better than I do, and I wanted to get this code out for review sooner rather than later.
Closes #203