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

fix bincode serialization #223

Merged
merged 4 commits into from
Aug 3, 2023
Merged

fix bincode serialization #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2723486
fix bincode serialization
Wollac Aug 2, 2023
d640422
fmt
Wollac Aug 2, 2023
549b632
re-use visitor
Wollac Aug 2, 2023
53d2d88
fix macro indention
Wollac Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ alloy-rlp = { version = "0.3.0", default-features = false }
alloy-rlp-derive = { version = "0.3.0", default-features = false }
arbitrary = "1.3"
arrayvec = { version = "0.7", default-features = false }
bincode = "1.3"
bytes = { version = "1.4", default-features = false }
criterion = "0.5"
derive_arbitrary = "1.3"
Expand Down
1 change: 1 addition & 0 deletions crates/primitives/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ proptest = { workspace = true, optional = true }
proptest-derive = { workspace = true, optional = true }

[dev-dependencies]
bincode.workspace = true
serde_json.workspace = true
serde = { workspace = true, features = ["derive"] }

Expand Down
22 changes: 21 additions & 1 deletion crates/primitives/src/bits/serde.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,18 @@ impl<'de, const N: usize> Deserialize<'de> for FixedBytes<N> {
}
}

deserializer.deserialize_any(FixedVisitor::<N>)
if deserializer.is_human_readable() {
deserializer.deserialize_any(FixedVisitor::<N>)
} else {
deserializer.deserialize_bytes(FixedVisitor::<N>)
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use bincode as _;
use serde::Deserialize;
#[derive(Debug, Deserialize)]
struct TestCase<const N: usize> {
Expand All @@ -82,6 +87,13 @@ mod tests {
let ser = serde_json::to_string(&bytes).unwrap();
assert_eq!(ser, "\"0x000000000123456789abcdef\"");
assert_eq!(serde_json::from_str::<FixedBytes<12>>(&ser).unwrap(), bytes);

let val = serde_json::to_value(&bytes).unwrap();
assert_eq!(val, serde_json::json! {"0x000000000123456789abcdef"});
assert_eq!(
serde_json::from_value::<FixedBytes<12>>(val).unwrap(),
bytes
);
}

#[test]
Expand All @@ -101,4 +113,12 @@ mod tests {
)
.contains("invalid length 5, expected exactly 4 bytes"),);
}

#[test]
fn test_bincode_roundtrip() {
let bytes = FixedBytes([0, 0, 0, 0, 1, 35, 69, 103, 137, 171, 205, 239]);

let bin = bincode::serialize(&bytes).unwrap();
assert_eq!(bincode::deserialize::<FixedBytes<12>>(&bin).unwrap(), bytes);
}
}
19 changes: 18 additions & 1 deletion crates/primitives/src/bytes/serde.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,18 @@ impl<'de> serde::Deserialize<'de> for Bytes {
}
}

deserializer.deserialize_any(BytesVisitor)
if deserializer.is_human_readable() {
deserializer.deserialize_any(BytesVisitor)
} else {
deserializer.deserialize_byte_buf(BytesVisitor)
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use bincode as _;
Copy link
Member

@DaniPopes DaniPopes Aug 2, 2023 •
edited
Loading

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)

Suggested change
use bincode as _;

Copy link
Contributor Author

@Wollac Wollac Aug 2, 2023 •
edited
Loading

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?

Copy link
Member

@DaniPopes DaniPopes Aug 2, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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...

Copy link
Member

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 _;

use serde::Deserialize;
#[derive(Debug, Deserialize)]
struct TestCase {
Expand All @@ -72,6 +77,10 @@ mod tests {
let ser = serde_json::to_string(&bytes).unwrap();
assert_eq!(ser, "\"0x0123456789abcdef\"");
assert_eq!(serde_json::from_str::<Bytes>(&ser).unwrap(), bytes);

let val = serde_json::to_value(&bytes).unwrap();
assert_eq!(val, serde_json::json! {"0x0123456789abcdef"});
assert_eq!(serde_json::from_value::<Bytes>(val).unwrap(), bytes);
}

#[test]
Expand All @@ -85,4 +94,12 @@ mod tests {
Bytes::from(Vec::from([0, 1, 2, 3, 4]))
);
}

#[test]
fn test_bincode_roundtrip() {
let bytes = Bytes::from_static(&[1, 35, 69, 103, 137, 171, 205, 239]);

let bin = bincode::serialize(&bytes).unwrap();
assert_eq!(bincode::deserialize::<Bytes>(&bin).unwrap(), bytes);
}
}
Loading