Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(l1): implement forkchoiceupdatedv2, newpayloadv2 and getpayloadv2 #1297

Closed
wants to merge 19 commits into from

Conversation

ElFantasma
Copy link
Contributor

@ElFantasma ElFantasma commented Nov 27, 2024

Motivation

We need to support post merge V1, V2 and V3 for GetPayload NewPayload nd ForkChoiceUpdated functions

Description

Implemented V2 for those functions

Closes #892, #1184, #1186

Makes to pass about 20 new hive tests

@ElFantasma ElFantasma linked an issue Nov 27, 2024 that may be closed by this pull request
@ElFantasma ElFantasma marked this pull request as ready for review November 28, 2024 19:42
@ElFantasma ElFantasma requested a review from a team as a code owner November 28, 2024 19:42
Copy link
Contributor

@fkrause98 fkrause98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

serde_json::json!(request.fork_choice_state),
serde_json::json!(attrs),
]),
..Default::default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd prefer if we used some explicit fields here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of prefer that too... but it's like repeating arbitrary code:

            id: RpcRequestId::Number(1),
            jsonrpc: "2.0".to_string(),

Copy link
Collaborator

@rodrigo-o rodrigo-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -157,3 +150,138 @@ impl RpcHandler for ForkChoiceUpdatedV3 {
serde_json::to_value(response).map_err(|error| RpcErr::Internal(error.to_string()))
}
}

trait ForkChoiceUpdatedImpl {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we just call it ForkChoiceUpdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a struct already named like that


impl RpcHandler for ForkChoiceUpdatedV2 {
fn parse(params: &Option<Vec<Value>>) -> Result<Self, RpcErr> {
Ok(Self(ForkChoiceUpdated::parse(params)?))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do you validate that params is in fact v2?

Copy link
Contributor Author

@ElFantasma ElFantasma Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference is the parent_beacon_block_root that shouldn't be present in V2. And it is checked in v2 validate() here

));
}
let chain_config = storage.get_chain_config()?;
fork_choice_updated.validate(attributes, chain_config, head_block)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the only thing that is different between the FCU calls? What if there is any actions besides validating?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this call the only difference are the validations and the version field in the PayloadArgs here. If some other action or validation is required we can extend the ForkChoiceUpdatedImpl trait to handle it in each version that implements it

@ElFantasma
Copy link
Contributor Author

Included in #1411

@ElFantasma ElFantasma closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ForkChoiceUpdatedV2
5 participants