Compartment transforms can't seem to add endowments

[warner]

The failure I'm seeing in import-bundle (in https://github.com/Agoric/agoric-sdk/pull/762) seems to stem from Compartment` transforms not working the way I think they're supposed to. The minimal test case is:

function transform(rewriterState) {
  const { src, endowments } = rewriterState;
  return { src: src.replace('replaceme', 'substitution'),
           endowments: { added: 'by transform', ...endowments },
         };
}

test.only('transforms can add endowments', t => {
  const src = '(function f4(a) {  return `replaceme ${added} ${a}`; })';
  const transforms = [transform];
  const c = new Compartment({}, {}, { transforms });
  const f4 = c.evaluate(src);
  const out = f4('ok');
  t.equal(out, 'substitution by transform ok');
  t.end();
});

I'll add a branch with this appended to test/compartment.test.js for convenience.

What I'm seeing is that the c.evaluate fails with ReferenceError: added is not defined. When I read through compartment-shim.js, I thought each transform accepted a { src, endowments } object and returned a new one of the same shape, so my transform() ought to leave an added endowment that should show up by the time f4 is evaluated.

I also tried providing the transforms as an option to c.evaluate(src, { transforms }), but that didn't work either.

[warner]

@michaelfig see if you can think of a reason why transforms are failing here.. nothing's jumped out at me.

BTW when I run yarn test to exercise this new test, I see the failed assertion go flying by, and the summary at the end of the test run is not so helpful in finding out the nature of the error:

  985 passing (3s)
  1 failing

  1) test/compartment.test.js test count !== plan:

      test count !== plan
      + expected - actual

      -11
      +1



error Command failed with exit code 1.

I'm grateful that tap told me the file where the failure occurred, but gosh it'd be nice if it told me the test case, or the exception. Is there a best-practice I'm missing on how to run or write the tests to get more information when one of them fails?

In case it helps anybody, I used yarn exec -- tap --no-esm test/compartment.test.js to run fewer tests (and give me a better chance of finding the error in the output). Additionally, I was able to change the test to a test.only and get even less noise (but note that, unlike tape, marking a tap test as test.only in one file doesn't inhibit the execution of all tests in other files, so you have to combine the two techniques). Also you might have to add --only; I'm not sure.

[warner]

I'm adding this to the zoe-alpha project because it's a blocker for the Agoric/agoric-sdk#477 new-ses work.

[warner]

@erights any ideas?

[warner]

Actually, this only blocks kernel-on-new-ses if swingset actually uses transforms to add endowments. @michaelfig I was assuming the metering/injection code used that feature (to add an endowment with the actual meter object to be decremented). If we're not using that (or if it's a SES1 thing only), then we can reduce the priority of this.

[michaelfig]

For (more-)secure metering, we actually need to put something in the global lexical scope (like SES1 endowments did), not on the globalThis (like SES2 endowments do). Otherwise, the user can bypass the transform's restrictions by concocting a string just with an indexed property get on globalThis to access the meter directly, which would be bad.

I know JF was talking about supporting the global lexical scope, but even if that exists, I'm certain that the transforms mechanism does not allow using it yet.

[erights]

Since I'm familiar with the SES1 per-evaluation endowments mechanism, I could add such a thing to the SES-shim. However, it would be peculiar to the shim. We've already covered with Moddable that the XS SES machine will not support such a mechanism. Therefore we are also unlikely to propose such a mechanism.

Are the per-compartments endowments problematic because they are per-compartment, or because they're on the global object? If the only problem is the latter, then we could get per-compartment global lexical endowments using the "optimizer" mechanism in the 8 magic lines. This also fits better with what Moddable can smoothly implement (attn @phoddie @patrick-soquet ). The shim would only support const bindings, but that's all that what we need here. Yes?

I suspect tc39 would also be more tolerant to see a per-compartment global lexical environment added to the proposal, as a construction option, than to see one added as a per-evaluation option.

[michaelfig]

Are the per-compartments endowments problematic because they are per-compartment, or because they're on the global object?

The latter. As long as the meter endowment is at least a const binding, it can only be referenced directly by name in an evaluation. That makes it possible for a transform to ensure that the evaluated pre-transformed code doesn't reference it.