-
Notifications
You must be signed in to change notification settings - Fork 206
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
feat(import-bundle): Support Endo zip hex bundle format #1983
Changes from all commits
a3011e5
77ce676
219d6d1
522a77f
370395b
d6483c6
c35cb03
84f8310
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,6 @@ export function f6ReadGlobal() { | |
return globalThis.sneakyChannel; | ||
} | ||
|
||
export function f7WriteGlobal(a) { | ||
// this will throw TypeError | ||
globalThis.sneakyChannel = a; | ||
} | ||
|
||
export function f8ReadGlobalSubmodule() { | ||
export function f7ReadGlobalSubmodule() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, why remove this test? We should still check that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The crux of the issue here is that freezing I could leave this test in place if I copied and modified the test for the new bundle format, but that seems like an unnecessary maintenance overhead. I could parameterize the test fixture so the constraint could vary. I could alternately go back into compartment mapper and thread a directive to freeze globalThis from |
||
return bundle2ReadGlobal(); | ||
} |
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.
@michaelfig will exposing these enable a metering break? If they're wrapped like all the other globals, probably not, but it's a new ability being handed to all Compartments so I wanted to double-check.
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.
@kriskowal I'm guessing that
TextEncoder
andTextDecoder
will be available in the immediateCompartments
produced by import-bundle, but they won't automatically be in any child compartments created therein. Is that correct? I guess that won't prohibit nested import-bundles, like the kernel does to load vats.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.
encode
anddecode
are O(string.length), so we may need to meter them.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.
cc @michaelfig I think I need your reaction to the question “do we need metering to attenuate Text{En,De}coder?”, before I land this change.
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.
Ideally, the TextEncoder and TextDecoder sources would be evaluated in a metered compartment (not endowed as a vetted shim), so that the metering transform would be applied to them as they are to user code.
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.
TextEncoder and TextDecoder are platform code.
sigh… We could get metering for free by slurping a UTF-8 dependency, but I’d wanted to avoid that because it’ll be unnecessary bloat on the web. The Text* utilities are available in both Node.js and Web.
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.
Oh, in that case, they're covered by the blanket metering performed by
@agoric/install-metering-and-ses
. No need to special-case (I had thought they were shims written by you).