Fix Nullable Fields in V2 Objects

[ethDreamer]

No description provided.

[ralexstokes]

lgtm

[mkalinin]

Suggested change

	- `withdrawals`: `Array of WithdrawalV1|null` - Array of withdrawals, each object is an `OBJECT` containing the fields of a `WithdrawalV1` structure.
	- `withdrawals`: `(Array of WithdrawalV1) | null` - Array of withdrawals, each object is an `OBJECT` containing the fields of a `WithdrawalV1` structure.

A nit: it may read as null values are allowed in the array.

[ethDreamer]

You think this is necessary? I wouldn't mind it's just that the way it is currently written is consistent with everywhere else in the document.

[mkalinin]

I don't insist if it reads as intended for everyone

[mkalinin]

As an alternative, we can spec params: i. ExecutionPayloadV1 | ExecutionPayloadV2 for newPayloadV2 and say that V2 structure must be used when withdrawals are enabled.

[ethDreamer]

As an alternative, we can spec params: i. ExecutionPayloadV1 | ExecutionPayloadV2 for newPayloadV2 and say that V2 structure must be used when withdrawals are enabled.

strongly against this as it makes the definitions of the objects more complex for client implementors

[mkalinin]

As an alternative, we can spec params: i. ExecutionPayloadV1 | ExecutionPayloadV2 for newPayloadV2 and say that V2 structure must be used when withdrawals are enabled.

strongly against this as it makes the definitions of the objects more complex for client implementors

What do you mean? We have a rough agreement that newPayloadVN version supports ExecuptionPayloadV(N-1) and ExecutionPayloadVN when it's possible. The spec may look like as follows:

engine_newPayloadV2

Request

• method: engine_newPayloadV2
• params:
  1. ExecutionPayloadV1 | ExecutionPayloadV2, where:
     • ExecutionPayloadV1 MUST be used before Shanghai
     • ExecutionPayloadV2 MUST be used after Shanghai
     • If a method call uses wrong structure, client software MUST return -32602: Invalid params [or any other type of an error]

Response

Refer to the response for engine_newPayloadV1.

Specification

This method follows the same specification as engine_newPayloadV1.

The above does look clearer to me than what we currently have in the spec.

I am still against abusing INVALID status for the case when the wrong data structure is used. But that is a topic for separate debates.

[ethDreamer]

What do you mean? We have a rough agreement that newPayloadVN version supports ExecuptionPayloadV(N-1) and ExecutionPayloadVN when it's possible. The spec may look like as follows:

engine_newPayloadV2

Request

* method: `engine_newPayloadV2`

* params:
  
  1. [`ExecutionPayloadV1`](#ExecutionPayloadV1) | [`ExecutionPayloadV2`](#ExecutionPayloadV2), where:
     
     * `ExecutionPayloadV1` **MUST** be used before Shanghai
     * `ExecutionPayloadV2` **MUST** be used after Shanghai
     * If a method call uses wrong structure, client software **MUST** return `-32602: Invalid params` [or any other type of an error]

Kind of let this fall off the radar. When I originally wrote that I was thinking that it is trivial to deserialize optional fields in most languages. For example, in Rust, if a field is an Option<T> and it's either null or not present in the JSON structure, it will automatically deserialize to None and if it is present it will deserialize to Some(T).

Having a structure that deserialize to two different structures (an enum) often involves writing extra code. After thinking about it for a while, I'm not sure yet if this would be necessary. I have to think about the definitions of:

• engine_newPayloadV2 request (given above)
• engine_getPayloadV2 response
• a new definition of PayloadAttributesV2
• the corresponding call to engine_forkchoiceUpdatedV2

to determine if this actually poses as much of an issue as I thought. Were you thinking we would also define PayloadAttributesV2 to always include withdrawals and then modify the definition of engine_forkchoiceUpdatedV2?

[mkalinin]

Having a structure that deserialize to two different structures (an enum) often involves writing extra code.

My intention is to keep the spec clear about which version of a datastructure should be used given the context, and how API must respond when the version is incorrect. Client implementation may utilise JSON fields' optionality and use one structure for each or a part of a version set.

Were you thinking we would also define PayloadAttributesV2 to always include withdrawals and then modify the definition of engine_forkchoiceUpdatedV2?

I would do it exactly the same way as with newPayloadV2 and ExecutionPayloadV1,V2. We already have a discrepancy between handling improper withdrawals values in two methods (newPayload piggy backs on INVALID, forkchoiceUpdated uses an error), and I would like to unify an approach to signal an error in this case all over the API.

[mkalinin]

The change discussed above is proposed by #337

[ethDreamer]

The change discussed above is proposed by #337

Sounds good. I'll close this PR and we can go with that one.