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

Clarify requirement for validator balances body #453

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

mcdee
Copy link
Contributor

@mcdee mcdee commented Jun 12, 2024

Looking at the validator_balances endpoint there is some ambiguity around what to do in some cases. Specifically, for POST to /eth/v1/beacon/states/head/validator_balances:

With no body:

  • lighthouse returns an error
  • lodestar returns an error
  • nimbus returns an error
  • prysm returns an error
  • teku returns all balances

With an empty array in the body:

  • lighthouse returns no balances
  • lodestar returns no balances
  • nimbus returns all balances
  • prysm returns all balances
  • teku returns all balances

I think that the best balance of attempting to retain existing functionality whilst firming up the spec is to state that no body will return an error, and an empty array will return all balances. This also ensures that users have a way to obtain all balances easily..

@nflaig
Copy link
Collaborator

nflaig commented Jun 12, 2024

an empty array will return all balances

Agreed, this behavior is likely for the best. And just yesterday I noticed few interop issues between clients when using postStateValidators.

E.g. Prysm VC sends an empty statuses array which leads to some clients not returning any data. We might wanna add the same note there as well.

Empty array should equal omitting the property and not apply any filtering as otherwise it will always return no data.

nflaig
nflaig previously approved these changes Jun 12, 2024
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.

It's a bit weird but feels like the best solution given current behaviour

rkapka
rkapka previously approved these changes Jun 14, 2024
@rolfyone
Copy link
Collaborator

I'm not sure I 100% agree that 'no body' equates to error. It's logically the same as an empty array... We can throw an error, but it's a bit weird IMO.

In real terms you haven't requested anything specifically in both cases, and the default is 'return all data'.

@nflaig
Copy link
Collaborator

nflaig commented Jun 17, 2024

I'm not sure I 100% agree that 'no body' equates to error. It's logically the same as an empty array.

We might wanna keep the body as required : false, we don't really gain anything if server throws an error if body is omitted, or just leave it up to the server implementation to decide.

This is also much more user friendly if you wanna do a quick curl as you can just do

curl -X POST http://localhost:9596/eth/v1/beacon/states/head/validator_balances

no extra headers and -d "[]" required

@rolfyone
Copy link
Collaborator

I'm not sure I 100% agree that 'no body' equates to error. It's logically the same as an empty array.

We might wanna keep the body as required : false, we don't really gain anything if server throws an error if body is omitted, or just leave it up to the server implementation to decide.

This is also much more user friendly if you wanna do a quick curl as you can just do

curl -X POST http://localhost:9596/eth/v1/beacon/states/head/validator_balances

no extra headers and -d "[]" required

It's handy. technically adding required probably could be argued as breaking but given that most clients already error i'm not going to make that argument :) I'm fairly sure that accepting it as equivalent to an empty array is about 1-2 lines of code if people would just do that...
Teku generally has tests for empty array of a post as well as the empty body as equivalence for endpoints that query in that way.

@nflaig
Copy link
Collaborator

nflaig commented Jun 18, 2024

Teku generally has tests for empty array of a post as well as the empty body as equivalence for endpoints that query in that way.

Have updated Lodestar to behave the same as there is no downside if the server accepts an empty body for routes that only apply it to filter the response, but the client will still send an empty array if no filtering is applied.

@rolfyone
Copy link
Collaborator

Have updated Lodestar to behave the same as there is no downside if the server accepts an empty body for routes that only apply it to filter the response, but the client will still send an empty array if no filtering is applied.

this sounds like basically what we do - we always do send empty array if required, as technically that is correct, but no real down side to allowing no body - ta for update.

@mcdee mcdee dismissed stale reviews from nflaig and rkapka via 32ef522 August 13, 2024 10:25
@mcdee
Copy link
Contributor Author

mcdee commented Aug 13, 2024

Latest update tweaks the wording to consider both [] and an empty body as "return all validators".

@rolfyone rolfyone merged commit 0628244 into ethereum:master Aug 13, 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.

5 participants