ethereum / beacon-APIs

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

Make Error code property optional #420

Closed jeluard closed 9 months ago

jeluard commented 9 months ago

HTTP Errors define a code property that replicates the error response id. It complexifies the spec and forces implementations to duplicate this information.

Is there a reason for this? I couldn't find any obvious one.

If not, I would propose to mark those as not required. This is not a breaking change as no released spec defines those as required (unless explicitly stated, every property is optional).

mcdee commented 9 months ago

This is not a breaking change as no released spec defines those as required (unless explicitly stated, every property is optional).

This is not a good take on the previous situation. Although the required key wasn't supplied, it was assumed that all items were required unless explicitly stated as optional.

As for the actual issue here, code states "Either specific error code in case of invalid request or http status code" so it's possible for a specific error code to be something other than the HTTP request. It also allows for simpler clients, which can receive a single JSON object in response to a request and handle it accordingly without needing to obtain an HTTP status code separately.

I don't think this adds a lot of complexity, and the use case above should be enough to include it. I'm also not a fan of making properties optional unless there is a good reason for them to be optional (e.g. them not being available in certain situations). Given that this value is available, I'd prefer to keep it in the error response.

jeluard commented 9 months ago

This is not a good take on the previous situation. Although the required key wasn't supplied, it was assumed that all items were required unless explicitly stated as optional.

Makes sense.

Given that this value is available, I'd prefer to keep it in the error response.

Sounds good. Unusual for an HTTP API but no strong downside either.