Skip to content
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

Separate lodestar-api package #2568

Merged
merged 24 commits into from
May 27, 2021
Merged

Separate lodestar-api package #2568

merged 24 commits into from
May 27, 2021

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented May 27, 2021

Motivation

It was overdue to move the REST api code into its own package so it can be consumed by the validator, lightclient and dependees on NPM.

I've taken the opportunity to review how we declare and consume the REST routes to be more typesafe and succint. We can use Typescript to our advantage to ensure the client and server are compatible at build time and all the logic matches the spec definition.

I've tried to come up with a setup that derives this code (or part of it) from the openapi definition but I was unsuccessful. The code that you get is good but not good enough and requires significant tweaking defeating its purpose. We also have our own SSZ types objects which the openapi is not aware of complicating the logic in connecting both worlds.

See inline comments for rationale

// Reasoning of the API definitions
// ================================
//
// An HTTP request to the Lodestar BeaconNode API involves these steps regarding serialization:
// 1. Serialize request: api args => req params
// --- wire
// 2. Deserialize request: req params => api args
// --- exec api
// 3. Serialize api return => res body
// --- wire
// 4. Deserialize res body => api return
//
// In our case we define the client in the exact same interface as the API executor layer.
// Therefore we only need to define how to translate args <-> request, and return <-> response.
//
// All files in the /routes directory provide succint definitions to do those transformations plus:
// - URL + method, for each route ID
// - Runtime schema, for each route ID
//
// Almost all routes receive JSON and return JSON. So both the client and the server can be
// auto-generated from the definitions. Also, the design allows for customizability for the few
// routes that need non-JSON serialization (like debug.getState and lightclient.getProof)
//
// With this approach Typescript help us ensure that the client and server are compatible at build
// time, ensure there are tests for all routes and makes it very cheap to mantain and add new routes.
//
//
// How to add new routes
// =====================
//
// 1. Add the route function signature to the `Api` type. The function name MUST match the routeId from the spec.
// The arguments should use spec types if approapriate. Non-spec types MUST be defined in before the Api type
// so they are scoped by routes namespace. The all arguments MUST use camelCase casing.
// 2. Add URL + METHOD in `routesData` matching the spec.
// 3. Declare request serializers in `getReqSerializers()`. You MAY use `RouteReqTypeGenerator` to declare the
// ReqTypes and request serializers in the same place.
// 4. Add the return type of the route to `getReturnTypes()` if it has any. The return type doesn't have to be
// a full SSZ type, but just a TypeJson with allows to convert from struct -> json -> struct.

Description

Compact the client and server REST api logic into a separate package.

  • The client can be consumed directly, using cross-fetch. Consumers can provide their own http client if they want.
  • For the server, exposes a registerRoutes function which does that given the api implementation Api and a fastify server.
  • REST parsing tests are moved to the api package and test the client and server directly.
  • Re-write Api impl code in function form to automatically type all methods.

Other:

  • Bump Fastify to v3

BREAKING CHANGES

  • Ensures our Api implementation type matchs the JSON return type of all routes. This means adding a {data: T} wrapper to most routes

before

getGenesis(): Promise<phase0.Genesis>;

after

getGenesis(): Promise<{data: phase0.Genesis}>;
  • Flatten the Api object structure so beacon routes are not nested. This is necessary for the type tricks used through the routes definitions.

before

api.beacon.state.getFork()
api.beacon.pool.submitAttestations()

after

api.beacon.getStateFork()
api.beacon.submitPoolAttestations()
  • All types that exclusive to the API have been removed from lodestar-types and exist now only in that route namespace definition file.

before

import {SyncDuty} from "@chainsafe/lodestar-types";
const duty: SyncDuty = ...

after

import {routes} from "@chainsafe/lodestar-api";
const duty: routes.validator.SyncDuty = ...

@dapplion dapplion changed the title Dapplion/lodestar api Separate lodestar-api package May 27, 2021
@codeclimate
Copy link

codeclimate bot commented May 27, 2021

Code Climate has analyzed commit f08afa1 and detected 34 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 22
Duplication 12

View more on Code Climate.

@github-actions github-actions bot added the Api label May 27, 2021
@github-actions github-actions bot added CLI scope-networking All issues related to networking, gossip, and libp2p. labels May 27, 2021
return super.get((toHexString(key) as unknown) as ByteVector);
type PubkeyHex = string;

function toHexStringMaybe(hex: ByteVector | string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow to pass string as hex string to prevent unnecessary parsing in the API

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request fixes 1 alert when merging 0b53679 into f677874 - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to property

headSlot: BigInt(headSlot),
syncDistance: BigInt(0),
headSlot: headSlot,
syncDistance: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to return a slot value as BigInt, return as Slot

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request fixes 1 alert when merging c685f91 into f677874 - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to property

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request fixes 1 alert when merging f08afa1 into f677874 - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to property

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we discussed the details offline.
Long overdue update 🙏

@wemeetagain wemeetagain merged commit de1e121 into master May 27, 2021
@wemeetagain wemeetagain deleted the dapplion/lodestar-api branch May 27, 2021 17:17
@dapplion dapplion added the meta-breaking-change Introduces breaking changes to DB, Validator, Beacon Node, or CLI interfaces. Handle with care! label May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-breaking-change Introduces breaking changes to DB, Validator, Beacon Node, or CLI interfaces. Handle with care! scope-networking All issues related to networking, gossip, and libp2p.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants