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

Deduplicate primitive types and use alloy's existing types #7050

Closed
mattsse opened this issue Mar 8, 2024 · 13 comments · Fixed by #7931
Closed

Deduplicate primitive types and use alloy's existing types #7050

mattsse opened this issue Mar 8, 2024 · 13 comments · Fixed by #7931
Labels
C-discussion A discussion about the direction and design of the project 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 Mar 8, 2024

Describe the feature

ref alloy-rs/alloy#255

ref #5746

currently all codec traits are implemented in reth-primitives via derive macros, therefor we're duplicating types that already exists in alloy:

/// AccessList as defined in [EIP-2930](https://eips.ethereum.org/EIPS/eip-2930)
#[main_codec(rlp)]
#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, RlpDecodableWrapper, RlpEncodableWrapper)]
pub struct AccessList(

/// Withdrawal represents a validator withdrawal from the consensus layer.
#[main_codec]
#[derive(Debug, Clone, PartialEq, Eq, Default, Hash, RlpEncodable, RlpDecodable)]
pub struct Withdrawal {

/// Ethereum Log
#[main_codec(rlp)]
#[derive(Clone, Debug, PartialEq, Eq, RlpDecodable, RlpEncodable, Default)]
pub struct Log {

In order to replace them, we need to impl all the codecs for alloy types.
This can be done in two ways:

  1. impl codecs in alloy directly
  2. impl codecs in reth's codec crate
    name = "reth-codecs"

since codecs are a reth thing, we could pursue option 2.
This however, would require implementing codecs manually, because derive is not possible
we could also spin out codecs which is just the Compact trait to it's standalone repo and make it a feature in alloy (option 1.)

pub trait Compact: Sized {

wdyt @DaniPopes

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Mar 8, 2024
@mattsse mattsse added S-needs-design This issue requires design work to think about how it would best be accomplished C-discussion A discussion about the direction and design of the project and removed S-needs-triage This issue needs to be labelled labels Mar 8, 2024
@gakonst
Copy link
Member

gakonst commented Mar 10, 2024

I'm open to either. I do think the Compact eth specific encoding is valuable (optimal / zero cost?) so if it's worth pulling out of reth we should do that.

Copy link
Contributor

github-actions bot commented Apr 1, 2024

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 Apr 1, 2024
@ghost
Copy link

ghost commented Apr 2, 2024

hi @mattsse, have we decided which direction we should take on this one. I think alloy can benefit from this codec, we should move the codecs to alloy and reuse it in reth.

@github-actions github-actions bot removed the S-stale This issue/PR is stale and will close with no further activity label Apr 3, 2024
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started and removed S-needs-design This issue requires design work to think about how it would best be accomplished labels Apr 3, 2024
@mattsse
Copy link
Collaborator Author

mattsse commented Apr 3, 2024

this should be integrated in the codecs crate for alloy-eips types
then replacing the duplicated reth-primitive types with alloy-eips

@ghost
Copy link

ghost commented Apr 3, 2024

I can give it a try

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 3, 2024

let's do this one by one, starting with:

  • alloy-primitives::Log, replacing reth-primitives::Log
  • then accesslist
  • then withdrawals

we need compact codec tests from main, that we can use as test vectors for this

I think you can use cargo expand on main and copy the trait impl to get started

@mattsse mattsse assigned ghost Apr 3, 2024
@ghost
Copy link

ghost commented Apr 4, 2024

regarding the reth_codecs where should we spin it to a standalone one, maybe we create something like alloy/codecs, we need it to avoid circular deps while moving code around. Can you create the repo? @mattsse

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 4, 2024

these should be implemented where the Compact trait is defined, we discarded the other option.
then there are no circular deps

we can feature gate this impls with like an alloy feature

does that make sense?

@ghost
Copy link

ghost commented Apr 4, 2024

oh I see, this is definitely easier to implement. I thought we wanted to move the whole Compact to alloy, it's clear now.

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 4, 2024

great!

I think we can move the impls in a new alloy.rs file here

@ghost
Copy link

ghost commented Apr 9, 2024

I will continue with Withdrawal, and AccessList then we can replace them in next PRs.

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 9, 2024

cool! ty

separately, we can now replace the reth_primitive::Log type in a separate pr

@ghost
Copy link

ghost commented Apr 27, 2024

Thanks @mattsse for replacing duplicate Withdrawl part, I was a bit busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion A discussion about the direction and design of the project C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants