fix: get_block_by_number endpoint #310
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fmoletta
reviewed
Aug 27, 2024
crates/rpc/rpc.rs
Outdated
| let request = ForkChoiceUpdatedV3::parse(&req.params).ok_or(RpcErr::BadParams)?; | ||
| Ok( | ||
| serde_json::to_value(fork_choice::forkchoice_updated_v3(request, storage)?) | ||
| .unwrap(), |
There was a problem hiding this comment.
Could we return an error instead of unwrapping here?
fmoletta
reviewed
Aug 27, 2024
crates/rpc/rpc.rs
Outdated
| "engine_newPayloadV3" => { | ||
| let request = NewPayloadV3Request::parse(&req.params).ok_or(RpcErr::BadParams)?; | ||
| Ok(serde_json::to_value(engine::new_payload_v3(request, storage)?).unwrap()) | ||
| Ok(serde_json::to_value(payload::new_payload_v3(request, storage)?).unwrap()) |
There was a problem hiding this comment.
Could we return an error instead of unwrapping here?
mpaulucci
reviewed
Aug 28, 2024
| let safe = storage.get_block_number(request.fork_choice_state.safe_block_hash); | ||
| let finalized = storage.get_block_number(request.fork_choice_state.finalized_block_hash); | ||
|
|
||
| // Check if we already have the blocks stored. |
There was a problem hiding this comment.
no need for this check, you might get multiple updates that have the same safe/finalized block. You don't need to return an error here.
There was a problem hiding this comment.
Hmmm, in this step I'm actually checking that we have those blocks stored beforehand, rather than if we have already marked them as safe/finalized. But maybe I didn't fully understand what you meant.
mpaulucci
approved these changes
Aug 29, 2024
mpaulucci
pushed a commit
to mpaulucci/lambda_ethereum_rust
that referenced
this pull request
Oct 16, 2024
**Motivation** The `get-finalized` and `get-safe` endpoints were returning null values because we weren't handling the forkchoice updates from the engine. **Description** This PR adds a minimal implementation to store forkchoice updates and pass the tests. Additionally, it takes the opportunity to implement a refactor. - Moves `core/types/engine/payload.rs` to `rpc/types/payload.rs`. - Creates `rpc/engine/payload.rs` and `rpc/engine/fork_choice.rs` to modularize `rpc/engine/mod.rs`. - Adds a minimal implementation of `forkchoice_updated_v3()`. - Fixes`eth_getBlockByNumber`. **Observations** This PR doesn't implement the `engine_forkchoiceUpdatedV3` endpoint, It only stores the minimal data needed to pass `eth_getBlockByNumber`. Closes lambdaclass#270
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
get-finalizedandget-safeendpoints were returning null values because we weren't handling the forkchoice updates from the engine.Description
This PR adds a minimal implementation to store forkchoice updates and pass the tests. Additionally, it takes the opportunity to implement a refactor.
core/types/engine/payload.rstorpc/types/payload.rs.rpc/engine/payload.rsandrpc/engine/fork_choice.rsto modularizerpc/engine/mod.rs.forkchoice_updated_v3().eth_getBlockByNumber.Observations
This PR doesn't implement the
engine_forkchoiceUpdatedV3endpoint, It only stores the minimal data needed to passeth_getBlockByNumber.Closes #270