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(alloy-rpc-types-eth): Optional serde #1276

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

refcell
Copy link
Contributor

@refcell refcell commented Sep 11, 2024

Description

Downstream crates would like to use types from alloy-rpc-types-eth without the need for serde support. This PR places serialization for core eth types behind the serde feature flag that is enabled-by-default.

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 seems fine, since all tests basically use serde we could derive serde for tests as well, but this would now require changing all attributes, so I'm fine with this.

would appreciate one additional sanity review @klkvr @DaniPopes

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

lgtm! wondering if need for this means we are missing something in consensus types?

had a feeling that rpc-types were supposed to always implement serde as they should only be used to communicate with RPC, so curious to learn more about usecases needing this change

@mattsse
Copy link
Member

mattsse commented Sep 12, 2024

actually, @refcell is this a size related issue?

because this will still pull in serde via alloy-serde I believe

@refcell
Copy link
Contributor Author

refcell commented Sep 12, 2024

actually, @refcell is this a size related issue?

because this will still pull in serde via alloy-serde I believe

Good point, it was to decrease size and generally make available lighter types. Wonder if I can place alloy-serde behind the feature flag as well

@mattsse
Copy link
Member

mattsse commented Sep 12, 2024

should be possible to make it optional

@mattsse
Copy link
Member

mattsse commented Sep 12, 2024

actually, let me take a look at the engine type crate

@mattsse
Copy link
Member

mattsse commented Sep 12, 2024

ref #1280

@mattsse mattsse merged commit 8c5aff5 into alloy-rs:main Sep 12, 2024
52 checks passed
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