keymanager API `voluntary_exit` spec compliance

[yorickdowne]

Description

When calling keymanager API /eth/v1/validator/<pubkey>/voluntary_exit, Lighthouse creates a JSON structure that does not place everything under data. By the spec, it should.

{"message":{"epoch":"196868","validator_index":"505597"},"signature":"0xhexsig"}

by spec should be

{"data":{"message":{"epoch":"196868","validator_index":"505597"},"signature":"0xhexsig"}}

To be sure, the current behavior makes it easier to just POST the result to the CL. Spec, however, wants a data structure for GET and this not to be present for POST. 😅

Version

v4.3.0

[jimmygchen]

This is my bad, I added the spec after I did the implementation in Lighthouse.. not sure what I was thinking when I wrote the spec 🤦‍♂️
I can't think of a good reason to have the extra data layer, but looks like half of the clients have implemented this endpoint, people might reluctant to make changes now?

[yorickdowne]

I mean I for one am happy either way … but does spec usually do the data layer on GET, but not POST? Checking a bit of the CL API it seems to, but I am really not “in” the spec to a degree where I can say what its usual patterns are.

Broadly keeping to an existing pattern is probably worthwhile.

And yes Prysm, Teku and Nimbus use the data layer. Lodestar hasn’t implemented yet.

[nflaig]

Just implemented this on Lodestar, got it wrong without data layer at first as well but noticed that after running spec tests. That got me thinking though, might it be worth to change the spec to use GET instead of POST for the API?

• the operation is idempotent
• would make it clearer that is does not actually submit the voluntary exit
• would fix data inconsistency as it is only used for GET

Haven't found any discussion on using GET vs POST for the API and there is no official release yet for this API so might still be ok to change the spec.

[jimmygchen]

Thanks for bringing this up, I agree GET would be cleaner too. It was an oversight - originally the endpoint was designed to submit to BN, and that functionality was removed but we didn't think about changing the method to GET. If we could get consensus on changing this, I'd be happy to see this change.. but not 100% sure if it's worth it since a few clients have implemented it, and it would be breaking changes for all users that have already integrated with it.

[nflaig]

I don't have a strong opinion on changing the spec but since this was mainly driven by Lighthouse and you have to break your API now anyways to be spec compliant it might be the last chance to get the change in. This API is not used by the client itself changing it doesn't introduce an incompatibility between versions there but might wanna get an overview of tooling that relies on this.

[yorickdowne]

Afaik Eth Docker and Siren are the only tooling that use this endpoint at present. I’m ok with having the spec change if that makes it clearer and avoids technical debt in future, and special-casing by client in Eth Docker until all clients are spec-compliant again.