search

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

Inconsistency in `/eth/v1/node/peers` #366

Open mcdee opened 11 months ago

mcdee commented 11 months ago

The /eth/v1/node/peers returns a meta element that contains a count item. The schema for this item declares the value to be a number, which is inconsistent with the common format of providing numeric values as quoted strings.

A quick tour of the main beacon nodes shows the following for this value:

Options that I can think of:

  1. patch the spec so that the v1 endpoint returns a quoted value
  2. raise bugs with the relevant beacon nodes to ensure this value is returned unquoted
  3. deprecate the v1 endpoint and create a v2 endpoint with a quoted value
    1. retaining the meta element
    2. discarding the meta element (nowhere else do we return a value like this that can easily be calculated from the other returned information)
    3. moving the count value to the top level returned object where all other metadata lives

My inclination is for option 3 suboption 2, but I would be interested in hearing if anyone has a reason for this meta element to exist, and the count value within it.

rolfyone commented 11 months ago

i wouldn't be against a v2 without the count - its pretty trivial to get the count from an array...

I guess that translates to 3 suboption 2 being my preference.

arnetheduck commented 10 months ago

We will be fixing the count in nimbus - ie whether or not a v2 happens is orthogonal to whether v1 is correctly implemented in clients as far as I'm concerned.

nflaig commented 9 months ago

The meta element might have been a good idea if used consistently across all APIs but as it is right now, any top-level property should be considered metadata (execution_optimistic, finalized, etc.) and everything else (the actual data) must be part of data element.

As this API is an outlier and does not follow this pattern, I would also prefer option 3 suboption 2.

dapplion commented 9 months ago

+1 to burn the meta

dapplion commented 9 months ago

No one likes the meta element, but there's no appetite either for the effort to remove it

According to @mcdee review of clients seems that support of the meta element is poor. If no-one has complained should mean that no consumer is dependent on it.

To address this one-off oddity we could do a breaking change on route v1 and mark the meta property as optional. Then only Nimbus needs to fix the route to be compliant, which @arnetheduck is already keen on doing.

Thoughts?