-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix bincode serialization #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after nit and rebase to fix clippy
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use bincode as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't think this does anything (same in ../bits
)
use bincode as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without those I got
warning: external crate `bincode` unused in `alloy_primitives`: remove the dependency or add `use bincode as _;`
when building the tests without the serde
feature.
I don't know how to avoid this warning. Is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, it's fine then. I think the problem is that it gets included in the test binaries but not in the library, so it flags it as unused in the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we decided to have the use
in? In both places?
What I don't really understand is why the same is not needed for serde_json
. Seems like it's only used in the serde tests as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is:
core/crates/primitives/src/lib.rs
Lines 20 to 24 in 4f7a72c
// Used in Serde tests. | |
#[cfg(test)] | |
use serde as _; | |
#[cfg(test)] | |
use serde_json as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Motivation
Fixes
bincode
deserialization ofFixedBytes
andBytes
.Solution
Distinguish between
is_human_readable
and not also in the deserialization. If it is not human readable (e.g.bincode
) allow only bytes instead of also strings.This fixes #221 .
Added tests for the
bincode
roundtrip that previously failed.PR Checklist