refactor(extension,streams): Abstract envelope handling

[grypez]

Closes: #58

Adds makeStreamEnvelopeHandler, which makes a stream envelope handler for handling unknown values.

If the supplied value is a valid envelope with a defined stream envelope handler, the stream envelope handler will return whatever the defined handler returns.

If the stream envelope handler is passed a well-formed stream envelope without a defined handler, an explanation and the envelope will be passed to the supplied stream envelope handler.

If the stream envelope handler encounters an error while parsing the supplied value, it will pass the reason and value to the supplied stream envelope handler.

If no error handler is supplied, the default error handling behavior is to throw.

const streamEnvelopeHandler = makeStreamEnvelopeHandler(
  {
    command: handleMessage,
    capTp: async (content) => capTp?.dispatch(content),
  },
  (reason, value) => {
    throw new Error(`[vat IFRAME] ${reason} ${stringifyResult(value)}`);
  },
);

for await (const rawMessage of streams.reader) {
  console.debug('iframe received message', rawMessage);
  await streamEnvelopeHandler.handle(rawMessage);
}

Outdated but with previously requested changes either incorporated or borrowed on in the latest changes.

[SMotaal]

@grypez My review is limited to what was included in my previous review: LGTM

I'm not explicitly approving the PR as a whole as I'd like to give @rekmarks a chance to review and give his final approval.

[SMotaal]

I wonder if we want to upstream to @endo/captp, maybe bring it up in the next Endo Sync.

[FUDCo]

Why is URL being passed to the Compartment constructor?

[rekmarks]

That was my decision, but there's no greater intention behind it. I think I was of a mind to use it for demonstration purposes via kernel.evaluate(). Everything Compartment-related in the current vat implementation should be considered a placeholder.

[FUDCo]

In other words, this is a placeholder for "some universal endowments go here"?

BTW, IIUC, URL carries no authority, so it shouldn't be needed as an endowment, though it might be desirable to have it in the environment just to avoid having to reimplement it. In this case, it might be sufficient to harden URL, if that is actually possible -- I'm not sure how/if hardening works with exotic objects provided by the host, which I suspect this is, but if harden(URL) doesn't work, putting a hardened wrapper around it should be trivial. As far as I know Agoric hasn't done any work on taming the web APIs, so it's something we might well end up needing to do ourselves (not necessarily a comprehensive taming, but I certainly expect there will be specific things we want to make available to user code).

[rekmarks]

Not a final review, but a first pass.

[rekmarks]

This comment goes for all type tests: generally speaking, we haven't bothered with testing our types in this manner. I think the expense is especially difficult to justify for code that is internal to a monorepo, since any bugs or shortcomings of the types will become immediately apparent at development time. Furthermore, in my experience, exhaustive behavioral unit tests are generally sufficient to surface issues with the types.

The other issue with this way of writing type tests is that it's hard on the eyes, and disagreeable to the linter. There are libraries / frameworks that enable it to be done neatly, but adding those would be a bigger decision.

What do you think?

[grypez]

This issue was a challenging typescript exercise for me. The tests helped me validate my assumptions during development. Due to the expense you mentioned I don't advocate them as a required practice, but I do advocate them as an acceptable practice. Given the nuanced type interactions I am more confident in the maintainability of the envelope-kit solution with these type tests in place.

I think the patterns I used here are sufficient and would be helpful if a similar scenario arises, and could serve us well for two more PRs before we consider a dedicated library / framework. The behavioral unit tests cannot check the typing behavior without soothing typescript anyhow, so I collected all the typing assumptions into one test per tested artifact.

To reduce the number of eslint inline configuration comments, I could collect all the type tests into an envelope-kit-types.test.ts and wrap the whole file with the lint exceptions; alternatively we could adopt a convention like envelope-kit.test.types.ts and except these in .eslintrc.

[rekmarks]

Proposed different naming convention: envelope-kit.types.test.ts

[rekmarks]

TODO:

• Move type tests into envelope-kit.types.test.ts, configure exceptions in .eslintrc
• Move generics.ts to utils/generics.ts
• Simplify never type assertions

[rekmarks]

This file is currently being built to /dist due to its inclusion in /src. The file, however, is useful. I prefer to handle this in the following way:

• Create a directory /test in the package root directory
• Add the folder to the included files in tsconfig.json only
• Ensure that the folder is covered by the same ESLint rules as other test files in .eslintrc
• Import the fixtures from the new location

[rekmarks]

info: We call this practice "intentional destructive testing", and that is also an acceptable explanation string.

[rekmarks]

LGTM!