-
Notifications
You must be signed in to change notification settings - Fork 6
feat(extension): Database access PoC, with pseudo-kernel in a web worker #100
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
rekmarks
left a comment
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.
Just a preliminary review to make the linter happy. The inline suggestions plus a yarn lint:fix from the root should settle the matter.
| @@ -0,0 +1,260 @@ | |||
| import type { Database } from '@sqlite.org/sqlite-wasm'; | |||
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.
@sqlite.org/sqlite-wasm should be added as a production dependency of @ocap/extension.
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.
Q: does it need to be imported before endoify? It should be imported afterwards if possible.
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.
I think it needs to be before. I suspect SES will break OPFS access.
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.
It seems to work on my system: #105
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.
Ah, right. endoify installs SES but doesn't actually lock down the world (else none of the web APIs would work either).
| type VatId = string; | ||
| type RemoteId = string; | ||
| type EndpointId = VatId | RemoteId; |
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.
Our linter complains of a redundant union here, because this is essentially string | string, which is string. We could get around this problem by prefixing the ids and using string template types (this would require some extra type guards):
| type VatId = string; | |
| type RemoteId = string; | |
| type EndpointId = VatId | RemoteId; | |
| type VatId = `vid:${string}`; | |
| type RemoteId = `rid:${string}`; | |
| type EndpointId = VatId | RemoteId; |
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.
I think that's a lint rule that should be expunged with extreme prejudice. I can't imagine the mindset of someone who thinks that lint rule is a good idea -- part of the point of TypeScript's strictness is to let you work in the world of nominal types rather than locking you into JavaScript's rampant duck typing.
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.
In any case, it's a moot point for the moment, since these type defs are all speculative anyway, and I ended up commenting out the whole block of them to stop lint from complaining about unused types.
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.
I think this is actually pointing to a limitation of TypeScript, whose types are not inherently nominal, although nominal types can be simulated through various techniques.
For example this is completely valid TypeScript (playground link):
type VatId = string;
type RemoteId = string;
type EndpointId = VatId | RemoteId;
type Foo = string;
const foo: Foo = 'Nominal? Never heard of him';
const id: EndpointId = foo;The lint rule exists because the union is truly redundant; the type on both sides of the assignment operator are exactly the same.
Using template string literals is one way to get around this limitation (playground link):
type VatId = `vid:${string}`;
type RemoteId = `rid:${string}`;
type EndpointId = VatId | RemoteId;
type Foo = string;
const foo: Foo = 'Nominal? Never heard of him';
const id: EndpointId = foo;
// Type 'string' is not assignable to type 'EndpointId'.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.
Right. Hindley-Milner types, grrr. By rooting type inference in literals, they preclude certain kinds of nominal types without doing extra gymnastics such as the above. The presumption is that two things that are structurally the same are interchangeable, even when this is declaratively not so.
This nonsense:
type Point3d = [number, number, number]; // [x, y, z]
type Color = [number, number, number]; // [r, g, b]
export function moveTo(destination: Point3d): void {
// your ad here
}
const blue: Color = [0, 0, 255];
moveTo(blue);
Compiles without complaint.
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.
On the other hand, we can have the TS compiler do a lot of clever checking of literals using template string types:
type VatId = `v${number}`;
type RemoteId = `r${number}`;
type EndpointId = VatId | RemoteId;
type RefTypeTag = 'o' | 'p';
type RefModeTag = '+' | '-';
type InnerERef = `${RefTypeTag}${RefModeTag}${number}`;
type InnerKRef = `${RefTypeTag}${number}`;
type KRef = `k${InnerKRef}`;
type VRef = `v${InnerERef}`;
type RRef = `r${InnerERef}`;
type ERef = VRef | RRef;
const krgood: KRef = 'ko47'; // ok
const krbad: KRef = 'xo43'; // not ok
const vrgood: VRef = 'vp+43'; // ok
const vrbad: VRef = 'xx-19'; // not ok
const vgood: VatId = 'v29'; // ok
const vbad: VatId = 'vx13'; // not ok
let ref: ERef = 'vo-66'; // ok
ref = 'rp+3'; // ok
ref = 'xxx'; // nope
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.
(Though this is not as useful as it might seem, because these are things that rarely appear in literal form in the code anyway.)
This comment was marked as resolved.
This comment was marked as resolved.
|
@SocketSecurity ignore npm/@sqlite.org/sqlite-wasm@3.46.1-build3 |
| ], | ||
|
|
||
| // This rule complains about hygienically necessary `await null` | ||
| '@typescript-eslint/await-thenable': 'off', |
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.
Perhaps we should have an issue to add a lint rule requiring such hygiene.
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.
Yes! Agoric had just such a lint rule, so we can grab the requisite eslint voodoo from the agoric-sdk repo.
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.
Issues welcome 😄
rekmarks
left a comment
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.
Does what it says on the tin!
A couple of smaller things:
- See #105. I think we should merge or reimplement it
- Since this is explicitly a PoC, I won't block on this, but I would prefer it we deleted the commented-out code. I suggest:
- The placeholders that are already implemented elsewhere can just be deleted, then copied over when we want them.
- The novel types could be stored in a draft PR, issue, or I guess left in place.
Let me know what you think!
ad7bb11 to
9b64764
Compare
|
I deleted the commented out bits of code that implemented the unsupported kernel message cases (e.g., |
| // Handle messages from the console service worker | ||
| onmessage = async (event) => { | ||
| const message = event.data; | ||
| const { method, params } = message; |
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.
This change should cause the switch case blocks to infer the params type.
| const { method, params } = message; | |
| const { method, params } = message as Command; |
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.
We'd ideally throw in a type guard as well, but we can also leave it be for now since this will likely be addressed when we switch to streams in this file.
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.
Lovely!
rekmarks
left a comment
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.
LGTM!
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 (orKernel) that's in the current source tree, but rather lives alongside it. The source file undoubtedly should be moved to thekernelpackage, and then split into the parts that go into theKernelclass and the parts that go into the script that launches the kernel (feat(extension): Move kernel to dedicated worker #57).Messaging to and from the web worker is not yet integrated with our streams abstraction; for now I'm just using
postMessagein 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
CapTpCalland it is handled as a special case. Consequently I ended up having to add a case to theisCommandtype guard to accept an array as one of the valid parameter types in order to accommodate thekvSetandkvGetkernel 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. (refactor: Differentiate commands and command replies #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.tsyou'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.