Repository: endojs/endo (js)

Issue:
import-bundle: allow symbol-named properties in inescapableGlobalProperties
## What is the Problem Being Solved?

As discussed in the comments on #1676, we'd like to convey `passStyleOf` into vat environments via a symbol-named property on the global, rather than a string-named property on VatData.

To support this, we need to change `import-bundle` to pay attention to symbol-named properties of `inescapableGlobalProperties`, because currently it uses `Object.keys`, which ignores symbols and only copies the string-named properties into the child Compartment's global. We'll accomplish this by using `Reflect.ownKeys`

While we're at it, we'll change `import-bundle` to look for *all* own properties of `inescapableGlobalProperties`, not just the enumerable ones (which matches the change from `Object.keys` to `Reflect.ownKeys`).

## Description of the Design

Add tests. Change it to use `Reflect.ownKeys`.

## Security Considerations

None I can think of, whoever is calling `importBundle` is in control of what properties they put on `inescapableGlobalProperties`, so if they want to make those properties unenumerable or Symbols, that's their business. And that party is in complete control over the guest/Compartment code's world.

## Scaling Considerations

none

## Test Plan

unit tests

## Compatibility Considerations

In the unlikely event that someone was already using Symbol-named properties, or unenumerable ones, in their `inescapableGlobalProperties` call, their guest code will start getting access to new properties, perhaps suprisingly. But that seems quite unlikely.


## Upgrade Considerations

To benefit from this, the host code must start using a new version of `import-bundle`. For the Agoric chain, that means a new version of `swingset-xsnap-supervisor`, so the process is:

* do a chain-halting upgrade that includes the new supervisor
* create a new vat, or upgrade an existing vat, both of which will then use the new supervisor code
* (also use a new liveslots, to get the `passStyleOf` endowment, and a new/current `@endo/pass-style`, which pulls that endowment)


Pull request:
feat(import-bundle): accept symbol-named properties on inescapableGlobalProperties
Previously, the `inescapableGlobalProperties` option to `importBundle()` only accepted enumerable string-named properties. This commit changes that to accept all properties (even unenumerable ones), and to accept symbol-named properties (instead of only string-named ones).

We'll use this to provide `passStyleOf` to the vat environment built by liveslots, so it can use a real WeakMap, instead of the (slower) "virtual object -aware WeakMap".

closes #2376

### Documentation Considerations

This modifies the behavior of `options.inescapableGlobalProperties`, which is undocumented. We should probably write docs for it at some point.

### Upgrade Considerations

See #2376 for notes about compatibility considerations, probably nothing worth calling out in NEWS.md .


Patch:
diff --git a/packages/import-bundle/NEWS.md b/packages/import-bundle/NEWS.md
index 9eb6d279b..4d3cfa2de 100644
--- a/packages/import-bundle/NEWS.md
+++ b/packages/import-bundle/NEWS.md
@@ -1,3 +1,11 @@
+# User-visible changes to `@endo/import-bundle`
+
+## Next release
+
+The `inescapableGlobalProperties` option is changed from supporting only
+string-named enumerable own properties to supporting all own properties
+whether string-named or symbol-named, and whether enumerable or not. But
+see https://github.com/endojs/endo/blob/master/packages/import-bundle/src/compartment-wrapper.md for the longer term plan.
 
 ## 0.4.0 (2023-08-07)
 
diff --git a/packages/import-bundle/src/compartment-wrapper.js b/packages/import-bundle/src/compartment-wrapper.js
index b5bc57da7..bd420142f 100644
--- a/packages/import-bundle/src/compartment-wrapper.js
+++ b/packages/import-bundle/src/compartment-wrapper.js
@@ -42,10 +42,32 @@ export function wrapInescapableCompartment(
     // there are details to work out.
     c.globalThis.Compartment = NewCompartment;
 
-    for (const prop of Object.keys(inescapableGlobalProperties)) {
+    // Use Reflect.ownKeys, not Object.keys, because we want both
+    // string-named and symbol-named properties. Note that
+    // Reflect.ownKeys also includes non-enumerable keys.
+    // This differs from the longer term agreement discussed at
+    // https://www.youtube.com/watch?v=xlR21uDigGE in these ways:
+    // - The option in question should be named `inescapableGlobals` since
+    //   we want to reserve `*Properties` for descriptor copying rather
+    //   than value copying.
+    // - We don't plan to support such `*Properties` options at this time.
+    //   Rather, we should deprecate (and eventually remove) this one
+    //   once we introduce`inescapableGlobals`.
+    // - We plan to move these options to the `Compartment` constructor itself,
+    //   in which case their support in import-bundle will just forward
+    //   to `Compartment`.
+    // - Following the `assign`-like semantics agree on in that meeting,
+    //   this should only copy enumerable own properties, whereas the loop
+    //   below copies all own properties.
+    // The loop here does follow this agreement by differing from `assign` in
+    // making all the target properties non-enumerable. The agreement would
+    // further align the normal `Compartment` endowments argument to also
+    // make the target properties non-enumerable.
+    for (const prop of Reflect.ownKeys(inescapableGlobalProperties)) {
       Object.defineProperty(c.globalThis, prop, {
         value: inescapableGlobalProperties[prop],
         writable: true,
+        // properties of globalThis are generally non-enumerable
         enumerable: false,
         configurable: true,
       });
diff --git a/packages/import-bundle/src/compartment-wrapper.md b/packages/import-bundle/src/compartment-wrapper.md
index e43c6a971..d7fd4d3ce 100644
--- a/packages/import-bundle/src/compartment-wrapper.md
+++ b/packages/import-bundle/src/compartment-wrapper.md
@@ -44,3 +44,27 @@ c2.evaluate(`new WeakMap()`);
 
   [Compartments]: ../../ses/README.md#compartment
   [SES]: ../../ses/README.md
+
+## Note expected semantic changes
+
+This `inescapableGlobalProperties` currently copies the value of all
+own properties, whether string-named or symbol-named, and whether enumerable
+or not. This
+differs from the longer term agreement discussed at
+https://www.youtube.com/watch?v=xlR21uDigGE in these ways:
+- The option in question should be named `inescapableGlobals` since
+  we want to reserve `*Properties` for descriptor copying rather
+  than value copying.
+- We don't plan to support such `*Properties` options at this time.
+  Rather, we should deprecate (and eventually remove) this one
+  once we introduce`inescapableGlobals`.
+- We plan to move these options to the `Compartment` constructor itself,
+  in which case their support in import-bundle will just forward
+  to `Compartment`.
+- Following the `assign`-like semantics agree on in that meeting,
+  this should only copy enumerable own properties, whereas the loop
+  below copies all own properties.
+The loop here does follow this agreement by differing from `assign` in
+making all the target properties non-enumerable. The agreement would
+further align the normal `Compartment` endowments argument to also
+make the target properties non-enumerable.


Test patch:
diff --git a/packages/import-bundle/test/compartment-wrapper.test.js b/packages/import-bundle/test/compartment-wrapper.test.js
index beca26772..6494ee40e 100644
--- a/packages/import-bundle/test/compartment-wrapper.test.js
+++ b/packages/import-bundle/test/compartment-wrapper.test.js
@@ -5,6 +5,10 @@ import { wrapInescapableCompartment } from '../src/compartment-wrapper.js';
 
 const createChild = `() => new Compartment({console})`;
 
+// simulate what pass-style wants to give a liveslots environment
+const symbolEndowment = () => 0;
+const endowmentSymbol = Symbol.for('endowmentSymbol');
+
 function check(t, c, n) {
   t.is(c.evaluate('Compartment.name'), 'Compartment', `${n}.Compartment.name`);
 
@@ -21,11 +25,31 @@ function check(t, c, n) {
 
   t.is(c.evaluate('WeakMap'), 'replaced');
   t.is(c.evaluate('globalThis.WeakMap'), 'replaced');
+  t.is(c.evaluate('globalThis.unenumerable'), 'yep');
+  t.is(
+    c.evaluate(`globalThis[Symbol.for('endowmentSymbol')]`),
+    symbolEndowment,
+  );
 }
 
+// match what @endo/pass-style does, but note that import-bundle
+// does/should not depend upon on pass-style in any way
+//
+// alternative: have one of pass-style and import-bundle have a
+// devDependencies on the other (avoid a cycle, avoid strong/"normal"
+// `dependencies`), to allow a more thorough test, which is currently
+// only really exercised by the agoric-sdk/swingset-liveslots test
+
 test('wrap', t => {
   const inescapableTransforms = [];
-  const inescapableGlobalProperties = { WeakMap: 'replaced' };
+  const inescapableGlobalProperties = {
+    WeakMap: 'replaced',
+    [endowmentSymbol]: symbolEndowment,
+  };
+  Object.defineProperty(inescapableGlobalProperties, 'unenumerable', {
+    value: 'yep',
+    enumerable: false,
+  });
   const WrappedCompartment = wrapInescapableCompartment(
     Compartment,
     inescapableTransforms,