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

Derive OpenAPI type schema from pyspec #402

Open
1 task
dapplion opened this issue Jan 8, 2024 · 7 comments
Open
1 task

Derive OpenAPI type schema from pyspec #402

dapplion opened this issue Jan 8, 2024 · 7 comments

Comments

@dapplion
Copy link
Collaborator

dapplion commented Jan 8, 2024

Thanks to ethereum/consensus-specs/pull/3506 we have canonical mapping of SSZ -> YAML, so it is possible to derive OpenAPI schemas for all the spec types.

Since the vast majority of API types are spec types auto-deriving the types would reduce future fork development and maintenance. Plus it paves the way to SSZ-ing most API routes.

We can define API-only extra types in SSZ syntax, such as:

// extra_types/bellatrix.md

## Containers

### Registration

From the Builder API specification

```python
class ValidatorRegistration(Container):
    fee_recipient: ExecutionAddress  # Address to receive fees from the block
    gas_limit: uint64  # Preferred gas limit of validator
    timestamp: uint64  # Unix timestamp of registration
    pubkey: BLSPubkey  # BLS public key of validator

class SignedValidatorRegistration(Container):
    message: ValidatorRegistration
    signature: BLSSignature

Current status

I've created a simple python script to translate the pyspec to OpenAPI schemas https://pypi.org/project/pyspec2openapi. Here is a demo integration into the beacon-APIs repo dapplion#1

The diff is quite large since it merges all types into a single output file.

One can compare the bundled outputs with

git checkout master
swagger-cli bundle ./beacon-node-oapi.yaml -r -t yaml -o ./deploy/beacon-node-oapi-master.yaml
git checkout dapplion/derive-spec-types
swagger-cli bundle ./beacon-node-oapi.yaml -r -t yaml -o ./deploy/beacon-node-oapi-derive.yaml
diff -l -u deploy/beacon-node-oapi-master.yaml deploy/beacon-node-oapi-derive.yaml | colordiff | more -R 

The bundled diff is also quite substantial due to some key characteristics

  • Usage of allOf to define block types. Generated spec does not de-duplicate keys, resulting in less indentations
  BeaconBlock:
    allOf:
      - $ref: '.../BeaconBlockCommon'
      - $ref: '#/Deneb/BeaconBlockBody'
  • Lots of comments per-property and per type not present in the generated specs
    description: "The [`BeaconBlockBody`](https://github.com/ethereum/consensus-specs/blob/v1.3.0/specs/phase0/beacon-chain.md#beaconblockbody) object from the CL spec."

Next steps

  • Get buy-in from repo mantainer's, or how could I tune this to align with your prefered DX

If yes, decide how to merge

  • One big diff PR switching directly to generated types, a-la Derive spec types dapplion/eth2.0-APIs#1
  • Big diff PR merging existing existing schemas into a single spec.yaml file, then replace with generated schemas for a clearer diff
  • Modify current master branch to align with generated output (remove block allOf usage, remove comments), then merge a smaller diff PR switching to generated types
@arnetheduck
Copy link
Contributor

ooh, great stuff! 👍 from me, was hoping it would lead to this.

@rkapka
Copy link
Collaborator

rkapka commented Jan 12, 2024

It would be good to maintain the script either in this repo or another repo under ethereum. I feel uneasy about one person maintaining it privately (not from a security perspective, but because of bus factor == 1)

@dapplion
Copy link
Collaborator Author

It would be good to maintain the script either in this repo or another repo under ethereum. I feel uneasy about one person maintaining it privately (not from a security perspective, but because of bus factor == 1)

Sounds good, will look how to integrate it without polluting this repo too much. @rolfyone thoughts?

@rolfyone
Copy link
Collaborator

Can we have the conversion script in this repo or in consensus-specs and just run it during build or something? or produce something in consensus-specs as part of that build that we then ingest would be ideal maybe?

It looks like we're stripping out a bunch of types, can we just ingest it and ignore the types we dont need or something? I like the direction, agree polluting this repo would be less than ideal

@dapplion
Copy link
Collaborator Author

Can we have the conversion script in this repo or in consensus-specs and just run it during build or something? or produce something in consensus-specs as part of that build that we then ingest would be ideal maybe?

I don't think we should pollute the spec tests, since this is purely derivative it should leave in this repo

can we just ingest it and ignore the types we dont need or something?

There may be some types that are un-used but looks like the minority. For that we should either maintain and opt-in list or an exclusion list. Is it worth the effort to have a few less lines?

@rolfyone
Copy link
Collaborator

If we can just ingest the spec and it works, then the extra objects are pretty un-important, does make it easy when we add endpoints... The only thing would be if people get annoyed at unused objects if they cause issues in code generation maybe?

@jeluard
Copy link
Contributor

jeluard commented Jan 18, 2024

https://redocly.com/docs/cli/commands/bundle/ offers a --remove-unused-components option that should take care of unused objects.
Note that we are currently not using redocly for bundling but swagger-cli. swagger-cli is deprecated and recommends migrating to redocly

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