-
Notifications
You must be signed in to change notification settings - Fork 6
refactor: Begin using JSON-RPC for internal messages #451
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
Furthers the ongoing JSON-RPC refactor by making the "ping" method's params compliant therewith. Also fixes some test failures and minor issues in various config files.
| const testConfig: ClusterConfig = { | ||
| bootstrap: 'testVat', | ||
| forceReset: true, | ||
| bundles: null, |
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.
If there are no bundles indirectly referenced in any of the individual vat configs (which is actually a special case that doesn't happen that often), the bundles property should simply be absent, but I don't think there is any case where it is appropriate for it to have the value null. I noticed that you changed the schema from optional to nullable, but optional is really the correct thing. Since it's almost always absent, requiring it would be an ergonomic irritation for most use cases. What prompted this?
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 agree it's an ergonomic irritation, but this is another consequence of old TypeScript limitations around optional properties that we live with to this day via superstruct. Specifically, if you have a struct with property foo: optional(string), the resulting type will be foo?: string | undefined as opposed to foo?: string. Because JSON has no notion of undefined, our Json type forbids it, and optional properties on objects that will extend or be constrained to Json have to use nullable() instead of optional().
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's possible that this can be improved by adding a strictOptional struct to @metamask/superstruct. I'll try independently of this PR and undo the unergonomic-ness if so.
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 genuinely don't understand the constraint with optional properties. It shouldn't require you to JSON encode a value of undefined (which you correctly point out that JSON can't do) but simply omit the property entirely from the object.
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 used to be the case that TypeScript's notion of optional properties amounted to "the property can either be absent or set to undefined". superstruct has inherited this limitation, and does not in its current form support truly optional properties; whenever you use optional(), it permits the property to either be absent or set to undefined. Our own Json type is not so permissive; it does not permit the value undefined in any form. Because ClusterConfig is used in our messages, and our messages are as of this PR constrained to Json, ClusterConfig can no longer use optional(). However, I am trying to fix this upstream.
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.
JS's builtin JSON serialization is totally happy with undefined properties. If you JSON.stringify an object with a property whose value is undefined, it simply leaves the property out of the resulting JSON string. So it sounds to me like our Json type is just buggy. Is correcting that the upstream fix you're talking about?
packages/kernel/src/VatHandle.ts
Outdated
| // effectively performs the same function. Probably this ping should be | ||
| // removed. | ||
| await this.sendVatCommand({ method: VatCommandMethod.ping, params: null }); | ||
| await this.sendVatCommand({ method: VatCommandMethod.ping, params: [] }); |
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.
My vat restart PR #448 gets rid of this ping entirely.
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.
Word, I'll delete it here too then.
sirtimid
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
Ref: #281 #369
This is the first step toward simplifying our internal message types. We will accomplish this simplifications by making our transports dumber, in that their only expectation is that every message is a valid JSON-RPC message. All other validation will be relegated to the RPC call and method implementation sites.
MetaMask itself uses JSON-RPC for all of its internal (and most external) messages. To this end, it has a mature stack for handling JSON-RPC messages and requests/responses, centered around @metamask/json-rpc-engine. Here, we introduce
JsonRpcEngineand JSON-RPC for UI <-> kernel messages only. This will grow to include all internal messages.JsonRpcEngineis somewhat idiosyncratic (read: has some dumb stuff we'd like to remove), but is ultimately pretty straightforward to work with. See its documentation for details. One of the main implications is for our message types. Where they previously looked e.g. like this:They will now look like this:
Ultimately, callers and method implementers will never have to concern themselves with anything other than the
methodandparamsfor calls, andresult/errorfor responses. The biggest differences from our existing types are that:paramsmust be either an array or an object—think positional or named—and can therefore not benull.resultorerrorproperty, neverparams.This PR will be followed immediately by the next pipeline to receive the JSON-RPC treatment, and culminate in a rewrite of our RPC method implementations and their types.