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

feat: support qvalue weighting in Accept headers #6014

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

jshufro
Copy link
Contributor

@jshufro jshufro commented Oct 1, 2023

Motivation

Improves compatibility with attestantio's beacon client, and other libraries in general.

Description

Adds a utility function to parse and prioritize Media Types based on the Accept header and RFC-9110.
Preserves the default behavior of returning json instead of a 406 (since the beacon-API doesn't seem to like HTTP 406).

Adds unit tests for the new function.

Closes #5966

Steps to test or reproduce

Tested with yarn test:unit --scope @lodestar/api

@jshufro jshufro requested a review from a team as a code owner October 1, 2023 21:33
@CLAassistant
Copy link

CLAassistant commented Oct 1, 2023

CLA assistant check
All committers have signed the CLA.

@jshufro jshufro changed the title Support qvalue weighting in Accept headers feat: support qvalue weighting in Accept headers Oct 1, 2023
@nflaig nflaig added this to the v1.12.0 milestone Oct 1, 2023
@jshufro
Copy link
Contributor Author

jshufro commented Oct 1, 2023

@nflaig lint / type errors from the previous CI runs should now be resolved

Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

Really nice job! Your rocked thoroughly on this PR. Please check out the comments and let me know your thoughts. It is also possible that some of the other team members will leave a few comments for you as well!

Copy link
Member

@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 pretty good overall

packages/api/src/beacon/routes/beacon/block.ts Outdated Show resolved Hide resolved
packages/api/src/utils/server/parseAcceptHeader.ts Outdated Show resolved Hide resolved
packages/api/src/utils/server/parseAcceptHeader.ts Outdated Show resolved Hide resolved
Copy link
Member

@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, thanks @jshufro. This will become increasingly important once more APIs support ssz and json format (#5128).

This is also a requirement outlined in ethereum/beacon-APIs#250

existing beacon API servers must at a minimum support the Accept header with multiple entries and quality values (e.g. Accept: application/octet-stream;q=1.0,application/json;q=0.9). Note that this appears to be the case for current beacon API server implementations, in that they at least will currently return JSON data when presented with this header

I also think defaulting to json is good for now, as far as I know fastify at least returns a 415 if no content-type header is provided. The spec issue suggests 415 error code but looking at the current spec it seems to have both 406 and 415

servers add support for the '415' error code when none of the options in the Accept header can be provided

Can revisit error behavior / codes another time.

@jshufro
Copy link
Contributor Author

jshufro commented Oct 2, 2023

This will become increasingly important once more APIs support ssz and json format

Can't come soon enough! Json is great for debugging but very, very slow :(

More immediately this will stop rescue-proxy from crashing lodestar here: https://github.com/Rocket-Rescue-Node/rescue-proxy/blob/master/consensuslayer/consensus-layer.go#L184

This function creates an index based on 0x01 credentials and is required for us to support non-rocketpool validators, a feature we're excited to roll out. Thanks for the quick reviews!

@nflaig
Copy link
Member

nflaig commented Oct 2, 2023

More immediately this will stop rescue-proxy from crashing lodestar here: https://github.com/Rocket-Rescue-Node/rescue-proxy/blob/master/consensuslayer/consensus-layer.go#L184

Right, the response when getting validators without specifying pubkeys / indexes is even bigger than the state response on mainnet. We should look into supporting ssz for this API, it is also quite frequently used by the validator client.

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.

Good job on this.
I'd love to hear more about your usecases wrt ssz responses. Feel free to comment on that issue in favor.

@matthewkeil
Copy link
Member

This looks really nice! Thank you for the contribution!

@nflaig nflaig merged commit d9e6f1a into ChainSafe:unstable Oct 3, 2023
13 of 14 checks passed
@jshufro jshufro mentioned this pull request Oct 3, 2023
@nflaig nflaig mentioned this pull request Nov 8, 2023
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.12.0 🎉

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.

Support q-factor weighting in Accept header
5 participants