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

Move more engine API impl logic into type EngineAPI #12276

Closed
mattsse opened this issue Nov 2, 2024 · 2 comments · Fixed by #12442 or #12445
Closed

Move more engine API impl logic into type EngineAPI #12276

mattsse opened this issue Nov 2, 2024 · 2 comments · Fixed by #12442 or #12445
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Nov 2, 2024

Describe the feature

This type implements the engine API for both ethereum and OP

pub struct EngineApi<Provider, EngineT: EngineTypes, Pool, Validator, ChainSpec> {

via:

impl<Provider, EngineT, Pool, Validator, ChainSpec> EngineApiServer<EngineT>
for EngineApi<Provider, EngineT, Pool, Validator, ChainSpec>

most of the logic is already directly implemented on EngineApi, but some logic is still part of the trait impl, e.g.:

/// Handler for `engine_newPayloadV1`
/// See also <https://github.com/ethereum/execution-apis/blob/3d627c95a4d3510a8187dd02e0250ecb4331d27e/src/engine/paris.md#engine_newpayloadv1>
/// Caution: This should not accept the `withdrawals` field
async fn new_payload_v1(&self, payload: ExecutionPayloadV1) -> RpcResult<PayloadStatus> {
trace!(target: "rpc::engine", "Serving engine_newPayloadV1");
let start = Instant::now();
let gas_used = payload.gas_used;
let res = Self::new_payload_v1(self, payload).await;
let elapsed = start.elapsed();
self.inner.metrics.latency.new_payload_v1.record(elapsed);
self.inner.metrics.new_payload_response.update_response_metrics(&res, gas_used, elapsed);
Ok(res?)
}

Eventually we want to use a different rpc server implementation for ethereum and OP but it would be nice if this only requires a trait impl and delegation.

for example:

impl EngineApiServer for EngineApi {
       async fn new_payload_v1(&self, payload: ExecutionPayloadV1) -> RpcResult<PayloadStatus> {
        trace!(target: "rpc::engine", "Serving engine_newPayloadV1");
        Ok(Self::new_payload_v1_metered(payload).await?)
      }
}

impl EngineApi {
   
    async fn new_payload_v1_metered(&self, payload: ExecutionPayloadV1) -> RpcResult<PayloadStatus> {
        let start = Instant::now();
        let res = Self::fork_choice_updated_v3(self, fork_choice_state, payload_attributes).await;
        self.inner.metrics.latency.fork_choice_updated_v3.record(start.elapsed());
        self.inner.metrics.fcu_response.update_response_metrics(&res);
    }
} 

effectively duplicating the function in impl EngineApi

most of this already exists, but we can move even more, like metric updates.

this should be done as 1 PR per function

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Nov 2, 2024
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started and removed S-needs-triage This issue needs to be labelled labels Nov 2, 2024
@Parikalp-Bhardwaj
Copy link
Contributor

Hi @mattsse, can I work on this issue

@Gerson2102
Copy link
Contributor

I would like to do this mate @mattsse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
3 participants