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

Ledger rpc type changes #72

Merged
merged 4 commits into from
Jan 29, 2025
Merged

Ledger rpc type changes #72

merged 4 commits into from
Jan 29, 2025

Conversation

exeokan
Copy link
Contributor

@exeokan exeokan commented Jan 29, 2025

Updates the types in line with chainwayxyz/citrea#1743

{
return Ok(());
}
let mut response = Err(anyhow!("initial response value"));
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 thought we might return the Err context here, as it was hard for me to debug why nodes were not becoming ready. But it is dependent on ledger_get_head_soft_confirmation_height, so we can revert this as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

How valuable is the error from ledger_get_head_soft_confirmation_height 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.

only if we change the endpoint again, but I don't think it is significant. It was hard for me to debug the type error here, so I thought I would include and see what you think

@exeokan exeokan marked this pull request as ready for review January 29, 2025 08:48
@exeokan exeokan requested a review from jfldde January 29, 2025 08:48
@@ -44,14 +45,16 @@ impl Client {
Ok(self
.client
.request("ledger_getLastScannedL1Height", rpc_params![])
.await?)
.await
.map(|v: U64| v.try_into().expect("U64 to u64 must succeed"))?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the need for that and alloy_prinitives import ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ledger_getLastScannedL1Height's return type was changed to Result<U64> in the PR at the description

@jfldde jfldde merged commit 928f681 into main Jan 29, 2025
4 checks passed
@jfldde jfldde deleted the ege/rpc-type-change branch January 29, 2025 09:02
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.

2 participants