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: update beacon jsonrpc endpoints #1604

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Dec 10, 2024

What was wrong?

Exposing the updates rather than the headers themselves is more useful for displaying information in trin-desktop.

How was it fixed?

  • Added jsonrpc endpoints to expose the finality/optimistic updates

To-Do

@njgheorghita njgheorghita requested review from KolbyML, morph-dev and ogenev and removed request for KolbyML and morph-dev December 10, 2024 17:19
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good

Some(client) => {
let update = client.get_finality_update().await;
match update {
Ok(update) => Ok(json!((update))),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ok(update) => Ok(json!((update))),
Ok(update) => Ok(json!(update)),

isn't this extra bracket redundent?

Some(client) => {
let update = client.get_optimistic_update().await;
match update {
Ok(update) => Ok(json!((update))),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ok(update) => Ok(json!((update))),
Ok(update) => Ok(json!(update)),

isn't this extra bracket redundent?

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -60,6 +64,14 @@ pub trait BeaconNetworkApi {
#[method(name = "beaconFinalizedHeader")]
async fn finalized_header(&self) -> RpcResult<BeaconBlockHeader>;

/// Get the finality update
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get the finality update
/// Get the latest finality update

#[method(name = "beaconFinalityUpdate")]
async fn finality_update(&self) -> RpcResult<LightClientFinalityUpdate>;

/// Get the optimistic update
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get the optimistic update
/// Get the latest optimistic update

@@ -116,6 +120,18 @@ impl BeaconNetworkApiServer for BeaconNetworkApi {
Ok(proxy_to_subnet(&self.network, endpoint).await?)
}

/// Get the optimistic update.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get the optimistic update.
/// Get the latest optimistic update.

Ok(proxy_to_subnet(&self.network, endpoint).await?)
}

/// Get the finality update.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get the finality update.
/// Get the latest finality update.

@njgheorghita njgheorghita merged commit e18427e into ethereum:master Dec 11, 2024
9 checks passed
@njgheorghita njgheorghita deleted the beacon-api branch December 11, 2024 15:16
carver added a commit to carver/trin that referenced this pull request Dec 11, 2024
The only change is the new beacon endpoints merged in
e18427e
via
ethereum#1604
carver added a commit that referenced this pull request Dec 12, 2024
The only change is the new beacon endpoints merged in
e18427e
via
#1604
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.

3 participants