refactor: Differentiate commands and command replies #94

grypez · 2024-09-25T07:01:41Z

Toward #80, provides a typed distinction between issued commands and their replies.

Changes:

`@ocap/streams`

StreamPair now requires distinct Read and Write template parameters.

`@ocap/utils`

Introduces a new type CommandReply.
Introduces new interfaces CommandFunction and CommandReplyFunction to prevent mismatching method and params types in functions that accept these properties as independent arguments.
Splits VatMessage into VatCommand and VatCommandReply.
There are now two stream envelope kits: one for issued commands and one for replies. The existing streamEnvelopeKit is for issued commands, while the new streamEnvelopeReplyKit is for replies. Exports from the reply kit are named alike but have the suffix Reply, excepting the factory makeStreamEnvelopeReplyHandler and type StreamEnvelopeReplyHandler.

`@ocap/extension`

The background service worker, kernel worker and offscreen document now discriminate between issued commands and command replies at both typescript time and runtime.

`@ocap/kernel`

The Supervisor class's StreamEnvelopeHandler now expects issued commands, not command replies, at both typescript time and runtime.
The Vat class now uses a StreamEnvelopeReplyHandler when receiving messages. The handler has been renamed to streamEnvelopeReplyHandler to obviate this change.

rekmarks

Nice! Just some minor inline things.

packages/utils/src/types.ts

packages/utils/src/type-guards.ts

packages/extension/src/background.ts

packages/extension/src/offscreen.ts

packages/extension/src/shared.ts

@grypez

…ker (#100) This merges the work I've been doing on accessing SQLite from a web worker. This PR supercedes my earlier PR #74, since so much had changed in the main branch that it was easier to resync the changes in a new branch rather than to try to reconcile all the now irrelevant parts. Stuff that is still to do / stuff to note (a number of things are left undone in interest of moving things along, since they can be readily handled as separate issues/PRs): - This code creates a web worker that should eventually subsume the kernel, hence the file is called `kernel-worker.ts`. However, this does not replace the existing kernel (or `Kernel`) that's in the current source tree, but rather lives alongside it. The source file undoubtedly should be moved to the `kernel` package, and then split into the parts that go into the `Kernel` class and the parts that go into the script that launches the kernel (#57). - Messaging to and from the web worker is not yet integrated with our streams abstraction; for now I'm just using `postMessage` in the stupidest possible way. Integrating with streams proved cross-cutting enough with what I was already doing that I figured that was best handled in a separate pass. (#todo) - In the same vein, this would really like to have the rationalization of the messaging layer that @grypez is in the midst of, most especially more proper handling of reply messages. In particular, I found that the way we handle parameters is kind of sketchy, mostly because virtually the only operation we have that really uses parameters is `CapTpCall` and it is handled as a special case. Consequently I ended up having to add a case to the `isCommand` type guard to accept an array as one of the valid parameter types in order to accommodate the `kvSet` and `kvGet` kernel commands that I added. This is kind of a hack; while those two operations will go away, I expect there to be numerous commands to come that will really want proper parameter handling. (#94) - The database usage right now is just a simple key/value store that can be accessed via the console "API". This k/v thing is strictly for testing and validation purposes and should go away. However, inside `kernel-worker.ts` you'll find a bundle of preliminary TypeScript type defs for the state that the kernel should actually be storing. My plan is to generate corresponding database schemata as part of putting in the vat-to-vat message routing infrastructure (assuming nobody else gets to this first, which y'all have been amazingly good at doing). - In keeping with this being a PoC, there are no tests. Those will come when there's something worth testing.

Addressed

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

rekmarks · 2024-09-27T20:48:48Z

packages/utils/src/types.ts

  | CommandLike<CommandMethod.KVSet, { key: string; value: string }>;

-export type VatMessage = {
+export type CommandInterface<Return = void | Promise<void>> = {


"Interface" is a bit ambiguous for this type. Can we replace it with "Function"? Ditto for CommandReplyInterface.

Suggested change

export type CommandInterface<Return = void | Promise<void>> = {

export type CommandFunction<Return = void | Promise<void>> = {

rekmarks · 2024-09-27T20:50:16Z

packages/utils/src/type-guards.ts

+  isCommandLike(value) &&
  (typeof value.params === 'string' ||
    value.params === null ||
    isObject(value.params) || // XXX certainly wrong, needs better TypeScript magic


How does this line from Chip's PR interact with the command types?

It is not significantly looser than what we were already doing, which is checking that the method is an allowable type independently of the whether the params are an allowable type.

grypez requested a review from a team as a code owner September 25, 2024 07:01

grypez force-pushed the grypez/differentiate-commands-and-command-replies branch 2 times, most recently from 5c0d615 to a30d99a Compare September 25, 2024 21:39

rekmarks self-assigned this Sep 25, 2024

rekmarks self-requested a review September 25, 2024 21:43

rekmarks removed their assignment Sep 25, 2024

rekmarks previously requested changes Sep 25, 2024

View reviewed changes

grypez mentioned this pull request Sep 25, 2024

Investigate divergence between local and CI linter. #98

Closed

grypez requested a review from rekmarks September 26, 2024 15:50

rekmarks mentioned this pull request Sep 26, 2024

feat(extension): Database access PoC, with pseudo-kernel in a web worker #100

Merged

grypez force-pushed the grypez/differentiate-commands-and-command-replies branch from bec0434 to 1bd23aa Compare September 27, 2024 15:29

grypez force-pushed the grypez/differentiate-commands-and-command-replies branch 3 times, most recently from 9013a64 to 697c769 Compare September 27, 2024 17:24

grypez and others added 10 commits September 27, 2024 12:29

feat(utils): Add CommandReply.

411a20c

Differentiate commands and command replies

79062de

Pick a naming convention.

b21104c

finesse CI-specific lint error

fd53a39

fix(utils): Correct type definitions.

ff54b34

refactor(extension): Demorgan unexpected message checks.

1f40231

fix(extension): Remove generics.

3971fa7

docs(extension): Edit lint exception explanation.

7a9c75e

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

Add KV methods to CommandReply

b93deb8

feat(utils): Add Command and CommandReply interfaces.

d1b4aa0

grypez force-pushed the grypez/differentiate-commands-and-command-replies branch from 697c769 to d1b4aa0 Compare September 27, 2024 17:35

rekmarks requested changes Sep 27, 2024

View reviewed changes

rename Command*Interface -> Command*Function

74aa50c

grypez enabled auto-merge (squash) September 27, 2024 21:52

rekmarks approved these changes Sep 27, 2024

View reviewed changes

grypez merged commit 0ef1b9c into main Sep 27, 2024

grypez deleted the grypez/differentiate-commands-and-command-replies branch September 27, 2024 22:05

rekmarks mentioned this pull request Sep 30, 2024

feat(extension): Move kernel to dedicated worker #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Differentiate commands and command replies #94

refactor: Differentiate commands and command replies #94

Uh oh!

grypez commented Sep 25, 2024 •

edited

Loading

Uh oh!

rekmarks left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rekmarks Sep 27, 2024

Uh oh!

rekmarks Sep 27, 2024

Uh oh!

grypez Sep 27, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	export type CommandInterface<Return = void \| Promise<void>> = {
	export type CommandFunction<Return = void \| Promise<void>> = {

refactor: Differentiate commands and command replies #94

refactor: Differentiate commands and command replies #94

Uh oh!

Conversation

grypez commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

@ocap/streams

@ocap/utils

@ocap/extension

@ocap/kernel

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rekmarks Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

rekmarks Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

grypez Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grypez commented Sep 25, 2024 •

edited

Loading

`@ocap/streams`

`@ocap/utils`

`@ocap/extension`

`@ocap/kernel`

grypez Sep 27, 2024 •

edited

Loading