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.

Sign up for GitHub

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

Jump to bottom

feat: introduce dedicated types for Any type aliases #2046

Merged
merged 9 commits into from
Mar 4, 2025
Merged

feat: introduce dedicated types for Any type aliases #2046

merged 9 commits into from
Mar 4, 2025

Conversation

stevencartavia
Copy link
Contributor

@stevencartavia stevencartavia commented Feb 12, 2025 •
edited by yash-atreya
Loading

Motivation

closes #2020

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

mattsse
mattsse requested changes Feb 12, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, good start

crates/network/src/any/mod.rs Outdated

/// A wrapper for [`AnyRpcBlock`] that allows for handling unknown block types.
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct AnyRpcBlockWrapper(pub AnyRpcBlock);
Copy link
Member

Choose a reason for hiding this comment

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

in order to make this non breaking we need to rename this struct to AnyRpcBlock

we also need to expose all the functions that WithOtherFields has so that becomes non breaking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can I avoid errors when there is already a type defined with that name above?

Copy link
Member

Choose a reason for hiding this comment

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

remove the alias and place it where pub AnyRpcBlock is

mattsse
mattsse requested changes Feb 12, 2025
View reviewed changes
crates/network/src/any/mod.rs Outdated
Comment on lines 156 to 164
impl<'de> Deserialize<'de> for AnyRpcTransaction {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let inner = WithOtherFields::<Transaction<AnyTxEnvelope>>::deserialize(deserializer)?;
Ok(Self(inner))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

all of this can be derived

crates/network/src/any/mod.rs Outdated
Comment on lines 70 to 71
type BlockResponse =
WithOtherFields<Block<WithOtherFields<Transaction<AnyTxEnvelope>>, AnyRpcHeader>>;
Copy link
Member

Choose a reason for hiding this comment

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

this must now use the wrapper type

mattsse
mattsse requested changes Feb 15, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

last nit

this change could be breaking, so it would be helpful if you could open a reth pr with the alloy deps patched to this branch:

https://github.com/paradigmxyz/reth/blob/16c3c5b733702f31daa4bf4e2c688ce37ccd5b58/Cargo.toml#L614-L642

and patch to your fork and branch

crates/network/src/any/mod.rs Outdated
/// A wrapper for [`AnyRpcBlock`] that allows for handling unknown block types.
#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
pub struct AnyRpcBlock(
WithOtherFields<Block<WithOtherFields<Transaction<AnyTxEnvelope>>, AnyRpcHeader>>,
Copy link
Member

Choose a reason for hiding this comment

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

we also need from impls for these, you can use derive_more::From for all of these

@stevencartavia
Copy link
Contributor Author

stevencartavia commented Feb 15, 2025 •
edited
Loading

last nit

this change could be breaking, so it would be helpful if you could open a reth pr with the alloy deps patched to this branch:

https://github.com/paradigmxyz/reth/blob/16c3c5b733702f31daa4bf4e2c688ce37ccd5b58/Cargo.toml#L614-L642

and patch to your fork and branch

in line 643 ?

# alloy_rpc_types_any = { git = "https://github.com/my_user_name/alloy.git", branch = "my_user_name:this_branch" }
# alloy_rpc_types_eth = { git = "https://github.com/my_user_name/alloy.git", branch = "my_user_name:this_branch" }
# alloy_serde = { git = "https://github.com/my_user_name/alloy.git", branch = "my_user_name:this_branch" }

@zerosnacks zerosnacks requested a review from mattsse February 20, 2025 08:24
@mattsse mattsse added the blocked This cannot move forward until something else changes label Feb 20, 2025
mattsse
mattsse reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks good,

since this is technically breaking, I'm putting this on hold until next release

@yash-atreya yash-atreya added the enhancement New feature or request label Feb 28, 2025
@jenpaff jenpaff added this to the v1.0 milestone Mar 3, 2025
@yash-atreya
Copy link
Member

yash-atreya commented Mar 4, 2025 •
edited
Loading

Re: a234b7a
The new helpers should make this https://github.com/paradigmxyz/reth/blob/54485df18a740daa786b6fa97fb2e9aba66c0c1e/bin/reth-bench/src/bench/new_payload_fcu.rs#L180

and this

https://github.com/paradigmxyz/reth/blob/54485df18a740daa786b6fa97fb2e9aba66c0c1e/crates/ethereum/primitives/src/alloy_compat.rs#L13

easier and cleaner in reth

@yash-atreya yash-atreya added breaking Breaking changes have been made and removed blocked This cannot move forward until something else changes labels Mar 4, 2025
@yash-atreya yash-atreya self-assigned this Mar 4, 2025
@yash-atreya yash-atreya requested a review from mattsse March 4, 2025 06:55
@yash-atreya yash-atreya added the P-high High Priority label Mar 4, 2025
mattsse
mattsse requested changes Mar 4, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nits and q re TransactionResponse

@jenpaff jenpaff modified the milestones: v1.0, v0.2 Mar 4, 2025
@jenpaff jenpaff removed this from the v0.2 milestone Mar 4, 2025
@yash-atreya yash-atreya requested a review from mattsse March 4, 2025 14:15
mattsse
mattsse approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit cbbb698 into alloy-rs:main Mar 4, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yash-atreya yash-atreya yash-atreya left review comments

@mattsse mattsse mattsse approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

Assignees

@yash-atreya yash-atreya

Labels
breaking Breaking changes have been made enhancement New feature or request P-high High Priority
Projects
Alloy
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Introduce dedicated types for Any type aliases
4 participants
@stevencartavia @yash-atreya @mattsse @jenpaff