ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
328 stars 167 forks source link

Clarify requirement for validator balances body #453

Closed mcdee closed 1 month ago

mcdee commented 3 months ago

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:

With an empty array in the body:

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 commented 3 months ago

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.

rolfyone commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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