Make execution_optimistic required field in GetProposerDuties

[StefanBratanov]

PR Description

• Make execution_optimistic a required field so it is included even when the value is false (consistent with sync and attester duties)
• Change order in the sync and attester duties endpoints to be consistent with the spec
• Removed finalized field from PostAttesterDuties since it was not in the spec

Fixed Issue(s)

fixes #7053

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[mehdi-aouadi]

Since it's not a spec requirement and we want it to be included anyway maybe we could add a test then

[StefanBratanov]

Since it's not a spec requirement and we want it to be included anyway maybe we could add a test then

There are already tests. I just changed the expected json.

[zilm13]

At first I don't see finalized in duties spec, am I missing something in spec?
https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/duties/attester.yaml#L42-L58
Everything else looks good

[StefanBratanov]

At first I don't see finalized in duties spec, am I missing something in spec? https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/duties/attester.yaml#L42-L58 Everything else looks good

Good point. Seems to be part of this PR ethereum/beacon-APIs#254 but it is not added to the attester duties response and I don't think it is relevant to it as well. So removed it. @rolfyone any thoughts on this?

[zilm13]

LGTM

[rolfyone]

Sorry,
The main issue here is its effectively a breaking change, because initially the api had it not required... so if we ever do call a server that doesn't return it we'd break...

my preference would have been we kept the optional and defaulted the second part to false so that we always had a value, but the structure sees it as optional still...

[StefanBratanov]

Sorry, The main issue here is its effectively a breaking change, because initially the api had it not required... so if we ever do call a server that doesn't return it we'd break...

my preference would have been we kept the optional and defaulted the second part to false so that we always had a value, but the structure sees it as optional still...

Hmm not sure, we are only modifying the json type definition of the API response. The VC which can call different servers uses a jackson class to deserialise and that can still parse without that field exisiting. I think main ambition was to make it consistent with the sync duties and attester duties response which had the field as required.

[rolfyone]

if this is server side purely, we're probably ok. it's fine to send all the time, just if we're using this object to receive a response it needs to be optional... hope that clarifies...

[StefanBratanov]

Yeah, we use this class for deserialisation https://github.com/ConsenSys/teku/blob/master/data/serializer/src/main/java/tech/pegasys/teku/api/response/v1/validator/GetProposerDutiesResponse.java