refactor(l1): modularize forkchoice updated code #1406
Conversation
…across different versions.
| @@ -19,24 +20,18 @@ use crate::{ | |||
| #[derive(Debug)] | |||
| pub struct ForkChoiceUpdatedV3 { | |||
| pub fork_choice_state: ForkChoiceState, | |||
| #[allow(unused)] | |||
| pub payload_attributes: Result<Option<PayloadAttributesV3>, String>, | |||
There was a problem hiding this comment.
I changed this, since the object type should reflect the json schema that we receive via RPC. Also this was leaking to the outside (see rpc/engine/forkchoice)
| @@ -50,10 +45,20 @@ impl RpcHandler for ForkChoiceUpdatedV3 { | |||
| if params.len() != 2 { | |||
| return Err(RpcErr::BadParams("Expected 2 params".to_owned())); | |||
| } | |||
|
|
|||
| let forkchoice_state: ForkChoiceState = serde_json::from_value(params[0].clone())?; | |||
| // if there is an error when parsing, set to None | |||
There was a problem hiding this comment.
To avoid having the Result, if there is a parsing error I set the payload attribute to None. This is not spec compliant, but we should find another way of dealing with this case without changing the type.
| } | ||
| }; | ||
| let (head_block_opt, mut response) = | ||
| handle_forkchoice(&self.fork_choice_state, context.clone())?; |
There was a problem hiding this comment.
Most of the code has been extracted to handle_forkchoice, which we can reuse across the different FCU versions
| } | ||
|
|
||
| fn build_payload_v3( | ||
| attributes: &PayloadAttributesV3, |
There was a problem hiding this comment.
Here is where we could technically use a PayloadAttributes enum that contains all posibilities
pub enum PayloadAttributes {
PayloadAttributesV1,
PayloadAttributesV2,
PayloadAttributesV3,
}
| pub payload_attributes: Option<PayloadAttributesV3>, | ||
| } | ||
|
|
||
| impl RpcHandler for ForkChoiceUpdatedV2 { |
There was a problem hiding this comment.
A POC of a handler for ForkChoiceUpdatedV2 , theres a bit more of repeated code, but it is way more decoupled and has less abstractions.
mpaulucci
changed the title
**Motivation** We need to support post merge V1, V2 and V3 for `GetPayload`, `NewPayload` and `ForkChoiceUpdated` functions **Description** Moved @mpaulucci ideas from his [PR](#1406) into [my own PR](#1395) to get V2 code for `ForkChoiceUpdated` It includes the removal of payload_attributes being a result Closes #892, #1184, #1186
|
Included in #1411 |
Motivation
The motivation is to be able to reutilize code across the different versions of forkchoice updates, but without creating unnecesary abstractions and leaking them to the rest of the codebase.
This is a POC
Description
payload_attributesbeing a result. I think this was introduced to avoid erroing when we can parsepayload_attributessuccesfully (spec says we should move on with forkchoice updated)