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

[Merged by Bors] - add quoted serialization util for FixedVector and VariableList #1794

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions consensus/serde_utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ edition = "2018"
[dependencies]
serde = { version = "1.0.116", features = ["derive"] }
serde_derive = "1.0.116"
eth2_ssz_types = {path = "../ssz_types"}
hex = "0.4.2"

[dev-dependencies]
Expand Down
2 changes: 2 additions & 0 deletions consensus/serde_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ mod quoted_int;

pub mod bytes_4_hex;
pub mod hex;
pub mod quoted_u64_fixed_vec;
pub mod quoted_u64_var_list;
pub mod quoted_u64_vec;
pub mod u32_hex;
pub mod u8_hex;
Expand Down
129 changes: 129 additions & 0 deletions consensus/serde_utils/src/quoted_u64_fixed_vec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
//! Formats `FixedVector<u64,N>` using quotes.
//!
//! E.g., `FixedVector::from(vec![0, 1, 2])` serializes as `["0", "1", "2"]`.
//!
//! Quotes can be optional during decoding. If `N` does not equal the length of the `Vec`, the `Vec` is truncated.

use serde::ser::SerializeSeq;
use serde::{Deserializer, Serializer};
use serde_derive::{Deserialize, Serialize};
use ssz_types::typenum::Unsigned;
use ssz_types::FixedVector;
use std::marker::PhantomData;

#[derive(Serialize, Deserialize)]
#[serde(transparent)]
pub struct QuotedIntWrapper {
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty minor, but perhaps we could import this from crate::quoted_u64_vec?

#[serde(with = "crate::quoted_u64")]
pub int: u64,
}

pub struct QuotedIntFixedVecVisitor<N> {
_phantom: PhantomData<N>,
}

impl<'a, N> serde::de::Visitor<'a> for QuotedIntFixedVecVisitor<N>
where
N: Unsigned,
{
type Value = FixedVector<u64, N>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "a list of quoted or unquoted integers")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: serde::de::SeqAccess<'a>,
{
let mut vec = vec![];

while let Some(val) = seq.next_element()? {
let val: QuotedIntWrapper = val;
vec.push(val.int);
}
let fix: FixedVector<u64, N> = FixedVector::from(vec);
Copy link
Member

Choose a reason for hiding this comment

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

I can see it's subjective, but I think it would be safest if we reject anything that doesn't match the N length. This is what we do in ssz, so it seems fitting here.

Ideally, we would want to avoid reading any more than N elements from seq (as to prevent unnecessary allocations).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think you're totally right. It's even explicit in the API spec: https://github.com/ethereum/eth2.0-APIs/blob/master/types/state.yaml#L71

Ok(fix)
}
}

pub fn serialize<S>(value: &[u64], serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut seq = serializer.serialize_seq(Some(value.len()))?;
Copy link
Member

Choose a reason for hiding this comment

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

IMO, no need to check the len against N here. We can rely on the upstream type (FixedVector).

No change requested.

for &int in value {
seq.serialize_element(&QuotedIntWrapper { int })?;
}
seq.end()
}

pub fn deserialize<'de, D, N>(deserializer: D) -> Result<FixedVector<u64, N>, D::Error>
where
D: Deserializer<'de>,
N: Unsigned,
{
deserializer.deserialize_any(QuotedIntFixedVecVisitor {
_phantom: PhantomData,
})
}

#[cfg(test)]
mod test {
use super::*;
use ssz_types::typenum::U4;

#[derive(Debug, Serialize, Deserialize)]
struct Obj {
#[serde(with = "crate::quoted_u64_fixed_vec")]
values: FixedVector<u64, U4>,
}

#[test]
fn quoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", "2", "3", "4"] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn unquoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn mixed_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", 2, "3", "4"] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn empty_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![0, 0, 0, 0]);
assert_eq!(obj.values, expected);
}

#[test]
fn short_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 0, 0]);
assert_eq!(obj.values, expected);
}

#[test]
fn long_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4, 5] }"#).unwrap();
dbg!(&obj.values);
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn whole_list_quoted_err() {
serde_json::from_str::<Obj>(r#"{ "values": "[1, 2, 3, 4]" }"#).unwrap_err();
}
}
128 changes: 128 additions & 0 deletions consensus/serde_utils/src/quoted_u64_var_list.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//! Formats `VariableList<u64,N>` using quotes.
//!
//! E.g., `VariableList::from(vec![0, 1, 2])` serializes as `["0", "1", "2"]`.
//!
//! Quotes can be optional during decoding. If `N` is greater than the length of the `Vec`, the `Vec` is truncated.

use serde::ser::SerializeSeq;
use serde::{Deserializer, Serializer};
use serde_derive::{Deserialize, Serialize};
use ssz_types::typenum::Unsigned;
use ssz_types::VariableList;
use std::marker::PhantomData;

#[derive(Serialize, Deserialize)]
#[serde(transparent)]
pub struct QuotedIntWrapper {
Copy link
Member

Choose a reason for hiding this comment

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

As above, regarding importing existing definition.

#[serde(with = "crate::quoted_u64")]
pub int: u64,
}

pub struct QuotedIntVarListVisitor<N> {
_phantom: PhantomData<N>,
}

impl<'a, N> serde::de::Visitor<'a> for QuotedIntVarListVisitor<N>
where
N: Unsigned,
{
type Value = VariableList<u64, N>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "a list of quoted or unquoted integers")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: serde::de::SeqAccess<'a>,
{
let mut vec = vec![];

while let Some(val) = seq.next_element()? {
let val: QuotedIntWrapper = val;
vec.push(val.int);
}
let fix: VariableList<u64, N> = VariableList::from(vec);
Copy link
Member

Choose a reason for hiding this comment

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

As per previous comment, I think it would be safest to check we don't exceed the N length.

Ok(fix)
}
}

pub fn serialize<S>(value: &[u64], serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut seq = serializer.serialize_seq(Some(value.len()))?;
for &int in value {
seq.serialize_element(&QuotedIntWrapper { int })?;
}
seq.end()
}

pub fn deserialize<'de, D, N>(deserializer: D) -> Result<VariableList<u64, N>, D::Error>
where
D: Deserializer<'de>,
N: Unsigned,
{
deserializer.deserialize_any(QuotedIntVarListVisitor {
_phantom: PhantomData,
})
}

#[cfg(test)]
mod test {
use super::*;
use ssz_types::typenum::U4;

#[derive(Debug, Serialize, Deserialize)]
struct Obj {
#[serde(with = "crate::quoted_u64_var_list")]
values: VariableList<u64, U4>,
}

#[test]
fn quoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", "2", "3", "4"] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn unquoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn mixed_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", 2, "3", "4"] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn empty_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [] }"#).unwrap();
assert!(obj.values.is_empty());
}

#[test]
fn short_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2]);
assert_eq!(obj.values, expected);
}

#[test]
fn long_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4, 5] }"#).unwrap();
dbg!(&obj.values);
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn whole_list_quoted_err() {
serde_json::from_str::<Obj>(r#"{ "values": "[1, 2, 3, 4]" }"#).unwrap_err();
}
}
4 changes: 2 additions & 2 deletions consensus/ssz_types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ name = "ssz_types"

[dependencies]
tree_hash = "0.1.1"
serde = "1.0.116"
serde = { version = "1.0.116", features = ["derive"] }
serde_derive = "1.0.116"
serde_utils = { path = "../serde_utils" }
eth2_ssz = "0.1.2"
typenum = "1.12.0"
arbitrary = { version = "0.4.6", features = ["derive"], optional = true }
hex = "0.4.2"

[dev-dependencies]
tree_hash_derive = "0.2.0"
34 changes: 32 additions & 2 deletions consensus/ssz_types/src/bitfield.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::tree_hash::bitfield_bytes_tree_hash_root;
use crate::Error;
use core::marker::PhantomData;
use serde::de::{Deserialize, Deserializer};
use serde::de::{self, Deserialize, Deserializer, Visitor};
use serde::ser::{Serialize, Serializer};
use serde_utils::hex::{encode as hex_encode, PrefixedHexVisitor};
use ssz::{Decode, Encode};
use std::fmt;
use tree_hash::Hash256;
use typenum::Unsigned;

Expand Down Expand Up @@ -617,6 +617,36 @@ impl<N: Unsigned + Clone> tree_hash::TreeHash for Bitfield<Fixed<N>> {
}
}

/// Encode `data` as a 0x-prefixed hex string.
Copy link
Member

Choose a reason for hiding this comment

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

See primary review comment regarding code duplication.

fn hex_encode<T: AsRef<[u8]>>(data: T) -> String {
let hex = hex::encode(data);
let mut s = "0x".to_string();
s.push_str(hex.as_str());
s
}

struct PrefixedHexVisitor;

impl<'de> Visitor<'de> for PrefixedHexVisitor {
type Value = Vec<u8>;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a hex string with 0x prefix")
}

fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
if value.starts_with("0x") {
Ok(hex::decode(&value[2..])
.map_err(|e| de::Error::custom(format!("invalid hex ({:?})", e)))?)
} else {
Err(de::Error::custom("missing 0x prefix"))
}
}
}

#[cfg(feature = "arbitrary")]
impl<N: 'static + Unsigned> arbitrary::Arbitrary for Bitfield<Fixed<N>> {
fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result<Self> {
Expand Down
2 changes: 2 additions & 0 deletions consensus/types/src/beacon_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,14 @@ where
#[compare_fields(as_slice)]
pub validators: VariableList<Validator, T::ValidatorRegistryLimit>,
#[compare_fields(as_slice)]
#[serde(with = "serde_utils::quoted_u64_var_list")]
pub balances: VariableList<u64, T::ValidatorRegistryLimit>,

// Randomness
pub randao_mixes: FixedVector<Hash256, T::EpochsPerHistoricalVector>,

// Slashings
#[serde(with = "serde_utils::quoted_u64_fixed_vec")]
pub slashings: FixedVector<u64, T::EpochsPerSlashingsVector>,

// Attestations
Expand Down