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

Make serde-bincode opt in exexs #14237

Open
mattsse opened this issue Feb 5, 2025 · 1 comment · May be fixed by #14809
Open

Make serde-bincode opt in exexs #14237

mattsse opened this issue Feb 5, 2025 · 1 comment · May be fixed by #14809
Assignees
@temaniarpit27
Labels
A-sdk Related to reth's use as a library 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 Feb 5, 2025

Describe the feature

we currently always use bincode for handling exexnotifcations in exex's:

reth/crates/exex/exex/src/wal/storage.rs

Lines 144 to 147 in 8d35f26

// Deserialize using the bincode- and msgpack-compatible serde wrapper
let notification: reth_exex_types::serde_bincode_compat::ExExNotification<'_, N> =
rmp_serde::decode::from_read(&mut file)
.map_err(|err| WalError::Decode(file_id, file_path, err))?;

reth/crates/exex/exex/src/wal/storage.rs

Lines 166 to 168 in 8d35f26

// Serialize using the bincode- and msgpack-compatible serde wrapper
let notification =
reth_exex_types::serde_bincode_compat::ExExNotification::<N>::from(notification);

this adds a lot of code for a feature that can only be used programmatically.

Ideally this format can be configured programmatically.
we can use json encoding by default and make bincode opt-in with the expectation that exex developers will enable this.

TODO

  • feature gate exex notification serialization: introduce a feature in exex crate that configures this

Additional context

No response
@mattsse mattsse added A-sdk Related to reth's use as a library C-enhancement New feature or request labels Feb 5, 2025
@temaniarpit27
Copy link
Contributor

i would like to pick this one @mattsse

@mattsse mattsse added the D-good-first-issue Nice and easy! A great choice to get started label Feb 6, 2025
@mattsse mattsse moved this from Todo to In Progress in Reth Tracker Feb 6, 2025
@startup-dreamer startup-dreamer linked a pull request Mar 3, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@temaniarpit27 temaniarpit27

Labels
A-sdk Related to reth's use as a library C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Reth Tracker
Status: In Progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feat: Add bincode-serialization/deserialization feature to exexs notifications startup-dreamer/reth
2 participants
@temaniarpit27 @mattsse