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

replace reth BlobTransactionSidecar with alloy's #8081

Closed
mattsse opened this issue May 3, 2024 · 10 comments
Closed

replace reth BlobTransactionSidecar with alloy's #8081

mattsse opened this issue May 3, 2024 · 10 comments
Assignees
Labels
C-debt Refactor of code section that is hard to understand or maintain 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 May 3, 2024

Describe the feature

with this alloy-rs/alloy#677

we can remove this entirely:

/// This represents a set of blobs, and its corresponding commitments and proofs.
#[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
#[repr(C)]
pub struct BlobTransactionSidecar {
/// The blob data.
pub blobs: Vec<Blob>,
/// The blob commitments.
pub commitments: Vec<Bytes48>,
/// The blob proofs.
pub proofs: Vec<Bytes48>,
}

and this implementation as well

#[cfg(feature = "c-kzg")]
pub fn validate_blob(
&self,
sidecar: &BlobTransactionSidecar,
proof_settings: &KzgSettings,
) -> Result<(), BlobTransactionValidationError> {

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started C-debt Refactor of code section that is hard to understand or maintain labels May 3, 2024
@mahesh-dalle
Copy link

hey there, is the issue open or blocked by the alloy pr?

@mattsse
Copy link
Collaborator Author

mattsse commented May 4, 2024

should be ready now, last blocker is possibly alloy-rs/alloy#679 but doesn't impact changes just --all-features compilation

do you want to take this?

first we need to bump the alloy deps here and https://github.com/paradigmxyz/evm-inspectors

@mahesh-dalle
Copy link

Yes, i would like to try this.

@mattsse
Copy link
Collaborator Author

mattsse commented May 4, 2024

nice, if you have any questions etc, please open a draft pr and we take it from there

@mahesh-dalle
Copy link

Well, before opening a draft can u please dump as many pointers as you can. Also from what i understand , we need to delete the struct and all the impls related to it what do we exactly import from alloy for BlobTransactionSidecar?, also we need to delete the validate_blob function and import it from alloy_consensus right?

@mattsse
Copy link
Collaborator Author

mattsse commented May 5, 2024

we need to delete the struct and all the impls related to it what do we exactly import from alloy for BlobTransactionSidecar?, also we need to delete the validate_blob function and import it from alloy_consensus right?

yes to all of this

@mahesh-dalle
Copy link

we need to delete the struct and all the impls related to it what do we exactly import from alloy for BlobTransactionSidecar?, also we need to delete the validate_blob function and import it from alloy_consensus right?

yes to all of this
Do we need this ?
impl proptest::arbitrary::Arbitrary for BlobTransactionSidecar and BlobTransactionSidecarRlp ?
cuz i am getting an error after removing the struct and impl for BlobTransactionSidecar.

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   --> crates/primitives/src/transaction/sidecar.rs:468:1
    |
468 | impl proptest::arbitrary::Arbitrary for BlobTransactionSidecar {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------------
    | |                                       |
    | |                                       `reth_rpc_types::BlobTransactionSidecar` is not defined in the current crate
    | impl doesn't use only types from inside the current crate
    |
    = note: define and implement a trait or new type instead

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   --> crates/primitives/src/transaction/sidecar.rs:443:1
    |
443 | impl<'a> arbitrary::Arbitrary<'a> for BlobTransactionSidecar {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------------
    | |                                     |
    | |                                     `reth_rpc_types::BlobTransactionSidecar` is not defined in the current crate
    | impl doesn't use only types from inside the current crate
    |
    = note: define and implement a trait or new type instead
    ```

@mattsse
Copy link
Collaborator Author

mattsse commented May 5, 2024

this is merged now:

alloy-rs/alloy#677

@mahesh-dalle
Copy link

Hey i dont think i would be able to continue on this one. Will be out on a lookout for more easier GFIs.

@guha-rahul
Copy link
Contributor

submitting a pr on this issue then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
Development

No branches or pull requests

3 participants