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: add serde impls to Blob and Bytes48 #342

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

Rjected
Copy link
Contributor

@Rjected Rjected commented Aug 15, 2023

This adds serde Serialize and Deserialize implementations that work with a 0x prefix.

@Rjected Rjected marked this pull request as ready for review August 22, 2023 19:00
@asn-d6
Copy link
Contributor

asn-d6 commented Aug 24, 2023

Hey. Can you tell us why you need the serde impls and the from_hex() is not sufficient for your needs? Is it just for testing purposes?

@Rjected
Copy link
Contributor Author

Rjected commented Aug 24, 2023

Hey. Can you tell us why you need the serde impls and the from_hex() is not sufficient for your needs? Is it just for testing purposes?

I'd like to use Blob and Bytes48 in other types in reth and not have to write a manual Serialize or Deserialize impl for these higher level types, for example using this branch lets us derive here:
https://github.com/paradigmxyz/reth/blob/bfa130d1875f1e1510e181e09fced33578d70563/crates/primitives/src/transaction/eip4844.rs#L547-L557

We provide these traits on almost all basic types so it's possible for API consumers to use the serde ecosystem in their own applications. With the serde impl, it would also become possible to use these types directly for RPC serialization instead of requiring conversion to another type that uses Bytes:
https://github.com/paradigmxyz/reth/blob/bfa130d1875f1e1510e181e09fced33578d70563/crates/rpc/rpc-types/src/eth/engine/payload.rs#L215-L221

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Makes sense. For completeness, could we add serde impls for Bytes32 too?

@Rjected
Copy link
Contributor Author

Rjected commented Aug 24, 2023

Makes sense. For completeness, could we add serde impls for Bytes32 too?

just added!

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Great. Thanks for adding that. LGTM 👍

@asn-d6
Copy link
Contributor

asn-d6 commented Aug 25, 2023

Thanks!

@asn-d6 asn-d6 merged commit 666a9de into ethereum:main Aug 25, 2023
30 of 32 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