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

API conversions and CI #26

Closed
djrtwo opened this issue Apr 9, 2020 · 8 comments
Closed

API conversions and CI #26

djrtwo opened this issue Apr 9, 2020 · 8 comments

Comments

@djrtwo
Copy link
Contributor

djrtwo commented Apr 9, 2020

It seems desirable for standard conversions to exist between formats (YAML, protosbufs, SSZ, etc), regardless of whichever format this API repo uses to spec things.

Base on conversations from the recent eth2 call, I propose the following

  1. Better understand the constraints put on each format if we retain easy convertibility (e.g. does maintaining OpenAPI -> protobuf eliminate some useful type or feature we would otherwise be able to use?)
  2. Do some experimentation on any existing tools (especially in going from OpenAPI to protobuf and protobuf to OpenAPI) to see if this a simple and clean task

If the above investigations prove fruitful, then (following @paulhauner's suggestion) we should integrate autoconversions in the to CI to ensure that the API remains usable across the formats we explicitly want to support.

@djrtwo
Copy link
Contributor Author

djrtwo commented Apr 9, 2020

One specific item mentioned in the last call was to try a one-off conversion of the Prysm protobuf to Swagger/OpenAPI to see what the output looks like

@prestonvanloon
Copy link
Contributor

The output of Prysm's generated OpenAPI can be found here: https://api.prylabs.net (raw json)

@mpetrunic
Copy link
Collaborator

I think it's easier to implement additional beacon apis manually with existing types (that contain validations and descriptions). I am in process of refactoring current openapi schema so routes/route groups are split into smaller files(less conflicts and it's easier to maintain smaller files).

Reusing types also enable us to switch from hex to base64 by simply changing validation at base types.

I find it cool to have ci generate multiple artifacts like protobufs and postman definitions and we can attach them to api releases.

@paulhauner
Copy link
Contributor

paulhauner commented Apr 9, 2020

YAML, protosbufs, SSZ

Regarding YAML, Lighthouse supports --header "Accept: application/yaml" which gives a YAML encoding of whatever struct we would have encoded with JSON. Our mapping between the two is just whatever serde does, which may or may not be contentious. However, I do know that it matches the test format encoding (at least in the places I've checked).

Regarding SSZ, Lighthouse presently supports --header "Accept: application/ssz" which just sends back the SSZ encoding of whatever struct we would have encoded with JSON. However, this is not perfect for the following reasons:

  • No String type in SSZ: string could easily be utf-8 List[uint8, 2**32], however we've avoided this for the time being. For now, any endpoint that has a string doesn't support SSZ.
  • All lists in SSZ need to be fixed or have a maximum length: we could say "any JS array maps to a List[N, 2**32]" however this wouldn't be true for all objects since some fields (e.g., block.body.attestations) have smaller max lengths.

I haven't looked into mapping to protobuf any more than I've documented in ethereum/eth2.0-pm#141 (comment). I couldn't speak on how onerous this might be.

So in terms of supporting YAML and SSZ via the REST API as alternative content-types to JSON, I don't think it's going to be particularly onerous or limiting from an implementation perspective. I think the challenge might be with documenting them. I need to look into OpenAPI a bit more, but I expect we can add YAML encoding to the spec for free. I'd expect defining the SSZ encoding would be more onerous. Perhaps @mpetrunic has thought about this.

There's also the question about whether actually we want these encodings or not. I don't see a whole lot of value in YAML; I only added since it was a useful tool for the 2019 interop, however complying with eth2-testnet means that genesis states are now SSZ (not YAML). I could take it or leave it with YAML.

SSZ might come in handy for a few reasons:

  1. It's more compact (no schema, bytes not ASCII-hex, etc) and it is perfectly designed for all the structs in the spec (what a coincidence!)
  2. SSZ is designed to support a bunch of fancy Merkle stuff; if someone is producing partials or whatever then perhaps grabbing the SSZ first avoids an annoying JSON->SSZ conversion.
  3. Clients (hopefully) already have a hardened SSZ decoder. It might be advantageous from a security perspective to do VC<>BN comms via SSZ to remove the JSON parser from the attack surface.

I'd be interested to hear about (2) from those researching phase 1+; does it seem like a common use case to want to manipulate SSZ from the BN API?

@mpetrunic
Copy link
Collaborator

So in terms of supporting YAML and SSZ via the REST API as alternative content-types to JSON, I don't think it's going to be particularly onerous or limiting from an implementation perspective. I think the challenge might be with documenting them. I need to look into OpenAPI a bit more, but I expect we can add YAML encoding to the spec for free. I'd expect defining the SSZ encoding would be more onerous. Perhaps @mpetrunic has thought about this.

We thought about that and it shouldn't be much of and issue to document them.
For example, currently we have:

  responses:
    200:
      description: Success response
      content:
        application/json:
          schema:
            $ref: '#/components/schemas/Attestation'

we can modify it like this for api endpoints that makes sense to return ssz

  responses:
    200:
      description: Success response
      content:
        application/json:
          schema:
            $ref: '../../beacon-node-oapi.yaml#/components/schemas/Attestation'
        application/ssz:
          schema:
            type: string
            description: "Base64 encoded output of SSZ serialized [Attestation](link to md file with object description) object"
            example: "YXNkYXNkYXNk"

Which looks like this in swagger:
image

We would probably need to add md files containing "ssz description" for custom types that are not in spec repo so we can link them.

@djrtwo
Copy link
Contributor Author

djrtwo commented Apr 14, 2020

I suppose if desired, we can also maintain application/x-protobuf responses and link to types defined by prysmatic.

That said, I'm not certain if mixing protos in here makes much sense. Any thoughts on that?

@mpetrunic
Copy link
Collaborator

mpetrunic commented May 24, 2020

Played around with generating a description of a gRPC service from openapi schema.
Here is summary of that: https://hackmd.io/@mpetrunic/BJMAwwusI
Seems like going with gnostic (attempt 2) is better option but maybe @prestonvanloon could check it out also.

@dapplion
Copy link
Collaborator

dapplion commented Dec 8, 2023

API format is stable now

@dapplion dapplion closed this as completed Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants