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: can read eigenda v1 cert #17

Closed
wants to merge 7 commits into from
Closed

Conversation

bxue-l2
Copy link
Collaborator

@bxue-l2 bxue-l2 commented Dec 20, 2024

This is PR add data struct for EigenDA v1 cert, and is able to parse the bytes from EigenDA proxy memstore.

@bxue-l2 bxue-l2 requested a review from samlaf December 20, 2024 22:24
@samlaf samlaf changed the title can read eigenda v1 cert feat: can read eigenda v1 cert Dec 21, 2024
README.md Show resolved Hide resolved
// the fourth because 0x01010000 in the beginnin is metadata
match BlobInfo::decode(&mut &item_slice[4..]) {
Ok(cert_blob_info) => info!("cert_blob_info {:?}", cert_blob_info),
Err(e) => info!("cannot decode cert_blob_info {:?}", e),
Copy link
Collaborator

Choose a reason for hiding this comment

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

return an error instead of logging like this?

Copy link
Collaborator Author

@bxue-l2 bxue-l2 Dec 21, 2024

Choose a reason for hiding this comment

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

Actually, the main reason to do it, is such that you can still run with optimism da-server. And that won't break.

After this PR, we should always use eigenda-proxy

I can work on error cleanup in the above PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm. just add a TODO comment here so that we don't forget.

Copy link
Collaborator

Choose a reason for hiding this comment

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

or is the plan to not ever merge this PR? In this case let's just close it?

Comment on lines +1 to +15
use alloy_primitives::Bytes;
use alloy_rlp::{RlpDecodable, RlpEncodable};

use alloc::vec::Vec;

#[derive(Debug, PartialEq, Clone, RlpEncodable, RlpDecodable)]
pub struct G1Commitment {
pub x: [u8; 32],
pub y: [u8; 32],
}

#[derive(Debug, PartialEq, Clone, RlpEncodable, RlpDecodable)]
pub struct BlobQuorumParam {
pub quorum_number: u32,
pub adversary_threshold_percentage: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We prob want to auto generate these using https://docs.rs/prost-build/latest/prost_build/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a good idea. But prost probably can't work.

Remember we discuss bytes should not go to Vec, otherwise, rlp isn't compatible.

That is why, I imported alloy_primitives::Bytes, it will have the right rlp encoding behavior.

prost converts bytes to Vec, see https://github.com/tokio-rs/prost?tab=readme-ov-file#fields.

We can create an issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can customize the type that prost generates. For eg a build.rs with

use prost_build::Config;
use std::io::Result;

fn main() -> Result<()> {
    let mut config = Config::new();
    config.bytes(&["."]);
    config.compile_protos(&["src/scratch.proto"], &["src/"])?;
    Ok(())
}

will generate

    #[prost(bytes = "bytes", tag = "3")]
    pub batch_root: ::prost::bytes::Bytes,

This uses the prost Bytes type instead of the alloy. I'm sure there's a way to generate the alloy one, but it might require some fiddling around. Let's at least add a TODO comment to eventually switch to using prost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in PR 19

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see 781000e

@@ -136,6 +138,14 @@ where
trace!(target: "fetcher_with_eigenda_support", "Fetching hint: {hint_type} {hint_data}");

if hint_type == ExtendedHintType::EigenDACommitment {
let item_slice = hint_data.as_ref();

// the fourth because 0x01010000 in the beginnin is metadata
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: beginnin -> beginning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

close here, 781000e

@samlaf
Copy link
Collaborator

samlaf commented Jan 2, 2025

@bxue-l2 let's close this if it's already part of #19 and we don't plan to merge it? I copied or linked the relevant comments here in #19.

@bxue-l2 bxue-l2 closed this Jan 2, 2025
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