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

fix: remove unneeded allOf usages #407

Merged
merged 1 commit into from
Jan 17, 2024
Merged

fix: remove unneeded allOf usages #407

merged 1 commit into from
Jan 17, 2024

Conversation

jeluard
Copy link
Contributor

@jeluard jeluard commented Jan 11, 2024

A number of Schema and Response $refs embed description and example in a way that is not supported by the openapi spec (relying on an incorrect allOf indirection).

Starting with OpenAPI 3.1 description can be overridden at the $ref level.
example can be defined at the Response level, fixing most issues. Some others were duplicates or irrelevant.

Note that some components were already leveraging this syntax, so this PR merely aligns on a single syntax. This also reduces the validation and error messages complexity.

I also validated that example still show up in the swagger UI after this change.

requires #409

types/rewards.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

To make this easier to review and merge I think it should be split into three separate PRs

  • update openapi version (this might require separate discussion)
  • remove unneeded allOf usages
  • correct example definitions / indentation fixes

Removing example values should only be done if those are set incorrectly, otherwise I would also be in favor of keeping them.

@jeluard
Copy link
Contributor Author

jeluard commented Jan 12, 2024

@nflaig @dapplion This PR now depends on #409 for the openapi upgrade.
I removed all irrelevant example usages.

So we are left with what is required to remove the incorrect allOf usages:

  • rely on the new description override feature
  • pull up example in Responses

Note that some allOf were already useless.

apis/validator/beacon_committee_subscriptions.yaml Outdated Show resolved Hide resolved
types/altair/sync_committee.yaml Outdated Show resolved Hide resolved
types/api.yaml Outdated Show resolved Hide resolved
types/p2p.yaml Show resolved Hide resolved
types/p2p.yaml Show resolved Hide resolved
types/p2p.yaml Show resolved Hide resolved
description: "The genesis_time configured for the beacon node, which is the unix time in seconds at which the Eth2.0 chain began."
example: "1590832934"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should make sure to use same order of properties everywhere, or how did you determine if description vs example should be 2nd property? Looking at ExecutionOptimistic below, the original order was swapped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no consistent ordering in general. But that's probably for a different PR.
That said, I would be in favor of the following:

  • type or $ref
  • description
  • example

Copy link
Collaborator

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm, just a few nits regarding consistent ordering of properties.

This fixes error examples when working on spec in dev mode (without bundling step)

Comment on lines +75 to +77
description: 'A bit is set if a signature from the validator at the corresponding index in the subcommittee is present in the aggregate `signature`.'
$ref: "../primitive.yaml#/Bitvector"
example: "0xffffffffffffffffffffffffffffffff"
Copy link
Collaborator

Choose a reason for hiding this comment

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

does not follow ordering discussed in #407 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to keep the order unchanged in this PR (should be the case now) and have another one introducing a consistent order with associated CI and command to fix it.

apis/node/syncing.yaml Outdated Show resolved Hide resolved
types/p2p.yaml Show resolved Hide resolved
types/p2p.yaml Show resolved Hide resolved
types/primitive.yaml Show resolved Hide resolved
types/primitive.yaml Show resolved Hide resolved
types/primitive.yaml Show resolved Hide resolved
Copy link
Collaborator

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Looks good to me, as discussed in #407 (comment), previous ordering is kept to keep changes as minimal as possible

@rolfyone
Copy link
Collaborator

This runs ok, are we ok to merge?

@jeluard
Copy link
Contributor Author

jeluard commented Jan 16, 2024

@rolfyone fine with me!

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM

@rolfyone rolfyone merged commit 4bbb87d into ethereum:master Jan 17, 2024
3 checks passed
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

Successfully merging this pull request may close these issues.

4 participants