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

primitives: use alloy Sealed<Header> instead of SealedHeader #11123

Open
tcoratger opened this issue Sep 23, 2024 · 14 comments
Open

primitives: use alloy Sealed<Header> instead of SealedHeader #11123

tcoratger opened this issue Sep 23, 2024 · 14 comments
Labels
A-dependencies Pull requests or issues that are about dependencies C-debt Refactor of code section that is hard to understand or maintain

Comments

@tcoratger
Copy link
Contributor

Describe the feature

The current codebase uses the SealedHeader struct:

/// A [`Header`] that is sealed at a precalculated hash, use [`SealedHeader::unseal()`] if you want
/// to modify header.
#[derive(Debug, Clone, PartialEq, Eq, Hash, AsRef, Deref, Serialize, Deserialize, Compact)]
#[add_arbitrary_tests(rlp, compact)]
pub struct SealedHeader {
/// Locked Header hash.
hash: BlockHash,
/// Locked Header fields.
#[as_ref]
#[deref]
header: Header,
}

This is redundant with Alloy's Sealed<Header> type:

https://github.com/alloy-rs/core/blob/4b823f7977fcdc181a0bd04ba1b2c51f7a29b4e2/crates/primitives/src/sealed.rs#L3-L14

After the merge of #10691, we have multiple unnecessary conversions between Sealed<Header> and SealedHeader when sealing headers, as we now rely on the Alloy consensus Header type.

Task

  • Remove SealedHeader from the codebase.
  • Re-export and use Sealed<Header> from Alloy consistently.

Related #10691 (comment)

Additional context

No response

@tcoratger tcoratger added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Sep 23, 2024
@greged93
Copy link
Contributor

hey I'm down to work on this

@emhane emhane added C-debt Refactor of code section that is hard to understand or maintain A-dependencies Pull requests or issues that are about dependencies and removed C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Oct 3, 2024
@emhane
Copy link
Member

emhane commented Oct 3, 2024

did you get around to starting on this @greged93 ?

@greged93
Copy link
Contributor

greged93 commented Oct 3, 2024

@emhane I started and got caught in something else, let me jump back on it.
I think there were a couple of issues to fix regarding derives on Sealed (arbitrary, rlp encode, default, compact,...). Is it ok to add all these derives in Alloy?

@emhane
Copy link
Member

emhane commented Oct 3, 2024

@greged93 see #11442 (comment)

@greged93
Copy link
Contributor

greged93 commented Oct 4, 2024

@emhane this solves it for compact but we still have the same issue for Default, Encodable, Decodable and Arbitrary. Ok to add all these to alloy? Or use a new type ?

@emhane
Copy link
Member

emhane commented Oct 4, 2024

thanks for the investigation @greged93 . blocked by alloy-rs/core#754, alloy-rs/core#755 and alloy-rs/core#762.

@emhane emhane added the S-blocked This cannot more forward until something else changes label Oct 4, 2024
@mattsse
Copy link
Collaborator

mattsse commented Oct 4, 2024

there's no reason this type should even have rlp support.

the current arbitrary derive is also problematic as highlighted here alloy-rs/core#762

@greged93
Copy link
Contributor

greged93 commented Oct 4, 2024

I'll check where it is needed but currently Encodable and Decodable are implemented on SealedHeader.

@mattsse
Copy link
Collaborator

mattsse commented Oct 4, 2024

none of this should be there because using rlp on a sealed type is never used in any protocol

@greged93
Copy link
Contributor

greged93 commented Oct 4, 2024

Ok! Will wait on arbitrary and remove rlp then!

@greged93
Copy link
Contributor

greged93 commented Oct 4, 2024

@mattsse what should we do for Sealed<Header> then? If you want to keep the rlp implementation then we can add it to Alloy or add a newtype

@emhane emhane removed the S-blocked This cannot more forward until something else changes label Oct 7, 2024
@emhane
Copy link
Member

emhane commented Oct 7, 2024

main reason we want this is to use the alloy Sealable trait

https://github.com/alloy-rs/core/blob/172aa98d4c0d20b0f1fe1812a8d378d7005a4011/crates/primitives/src/sealed.rs#L90-L99

rlp traits will have to be implemented on a wrapper type in reth @greged93

@greged93
Copy link
Contributor

greged93 commented Oct 7, 2024

noted! just waiting on arbitrary which should be in alloy-primitives 0.8.6 then

Copy link
Contributor

This issue is stale because it has been open for 21 days with no activity.

@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Oct 29, 2024
@emhane emhane removed the S-stale This issue/PR is stale and will close with no further activity label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Pull requests or issues that are about dependencies C-debt Refactor of code section that is hard to understand or maintain
Projects
Status: Todo
Development

No branches or pull requests

4 participants