chore: Safe JSON RPC server and client

[spalladino]

Current situation

What was wrong with our way of exposing our APIs over JSON RPC? First of all, the JsonRpcServer defaulted to exporting every method on the underlying handler, including those flagged as private in ts (though js-private like #method were safe). While we had a blacklist, it's safer to be explicit on what to expose.

Then, argument deserialization was handled based on client inputs exclusively, allowing to target any class registered in the RPC interface. This means a method expecting an instance of class A as an argument, could just be fed an instance of B by its caller.

Deserialization itself was also unchecked. Most fromJSON methods did not validate any of their inputs, just assuming the any input object matched the expected shape. Primitive arguments to functions were also unchecked, so a caller could eg pass a number where a string was expected.

Other unrelated issues included often forgetting to register a class for (de)serialization in the rpc server, or forgetting to type all API return types as async (if they are over an RPC interface, calling any method should be async!).

These issues all affected the client as well. Though security is not so much of an issue on the client, lack of validation could indeed cause bugs by querying a server with a mismatching version.

Proposed fix

We introduce a pair of "safe" JSON rpc client and server abstractions, that consume a schema along with a handler. The schema leverages zod for validating all inputs (in the case of the server) and outputs (in the case of the client) and defining the exact set of methods that are exposed. Schemas are type-checked against the interface, so a change in the interface will trigger tsc to alert us to change the schemas as well.

As a side effect of this change, since each method now knows what it should be deserializing, we can kill much of the custom logic for (de)serialization, such as the string and class converters, and just rely on vanilla json serialization. Each class that we intend to pass through the wire should expose a static zod schema used for both validation and hydration, and a toJSON method that is used for serialization:

export class TestNote {
  constructor(private data: string) {}

  static get schema() {
    return z.object({ data: z.string() }).transform(({ data }) => new TestNote(data));
  }

  toJSON() {
    return { data: this.data };
  }
}

Then the API is defined as a plain interface:

export interface TestStateApi {
  getNote: (index: number) => Promise<TestNote>;
  getNotes: () => Promise<TestNote[]>;
  clear: () => Promise<void>;
  addNotes: (notes: TestNote[]) => Promise<TestNote[]>;
  fail: () => Promise<void>;
  count: () => Promise<number>;
  getStatus: () => Promise<{ status: string; count: bigint }>;
}

With its corresponding schema:

export const TestStateSchema: ApiSchemaFor<TestStateApi> = {
  getNote: z.function().args(z.number()).returns(TestNote.schema),
  getNotes: z.function().returns(z.array(TestNote.schema)),
  clear: z.function().returns(z.void()),
  addNotes: z.function().args(z.array(TestNote.schema)).returns(z.array(TestNote.schema)),
  fail: z.function().returns(z.void()),
  count: z.function().returns(z.number()),
  getStatus: z.function().returns(z.object({ status: z.string(), count: schemas.BigInt })),
};

Scope of this PR

This PR introduces the new safe json rpc client and server abstractions, but does not yet enable them. All start methods still use the old flavors. Upcoming PRs will add schemas for all our public interfaces and make the switch.

[ludamad]

Why check for string null and undefined?

I don't see any reason to

[ludamad]

tests look nice

[ludamad]

One wishlist item for me would be a request ID that is used to log this error so that it can be easily correlated from the tx bot e.g. side

[spalladino]

Not just for you: https://hackmd.io/@aztec-network/rkebFFEhA?type=view#Tracing-requests

[ludamad]

LGTM, thanks for doing this, effort definitely appreciated - don't take me nodding along as not appreciating every point you made :)

[benesjan]

Lovely PR writeup! Thanks