-
Notifications
You must be signed in to change notification settings - Fork 12
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
Humanize array ser/deser #468
Conversation
consensus-engine/src/types.rs
Outdated
let mut s = <&str>::deserialize(deserializer)?; | ||
let extra_len = if s.starts_with("0x") || s.starts_with("0X") { | ||
s = &s[2..]; | ||
2 | ||
} else { | ||
0 | ||
}; | ||
if s.len() != N * 2 { | ||
return Err(<D::Error as serde::de::Error>::invalid_length( | ||
N + extra_len, | ||
&format!("{N}").as_str(), | ||
)); | ||
} | ||
|
||
let mut output = [0u8; N]; | ||
|
||
for (chars, byte) in s.as_bytes().chunks_exact(2).zip(output.iter_mut()) { | ||
let (l, r) = (chars[0] as char, chars[1] as char); | ||
match (l.to_digit(16), r.to_digit(16)) { | ||
(Some(l), Some(r)) => *byte = (l as u8) << 4 | r as u8, | ||
(_, _) => { | ||
return Err(<D::Error as serde::de::Error>::custom(format!( | ||
"Invalid character pair '{l}{r}'" | ||
))) | ||
} | ||
}; | ||
} | ||
Ok(output) |
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.
This should be a single function.
consensus-engine/src/types.rs
Outdated
<&[u8]>::deserialize(deserializer).and_then(|bytes| { | ||
if bytes.len() != N { | ||
Err(<D::Error as serde::de::Error>::invalid_length( | ||
bytes.len(), | ||
&format!("{N}").as_str(), | ||
)) | ||
} else { | ||
let mut output = [0u8; N]; | ||
output.copy_from_slice(bytes); | ||
Ok(output) | ||
} | ||
}) |
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.
This as well a single function
consensus-engine/src/types.rs
Outdated
@@ -190,6 +190,81 @@ impl Qc { | |||
} | |||
} | |||
|
|||
mod util { |
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.
Maybe we can have a utils module, inside a serde one:
consensus-engine/utils/serde
?
consensus-engine/src/types.rs
Outdated
let size = N * 2 + 2; | ||
let mut s = String::with_capacity(size); | ||
s.push_str("0x"); | ||
for v in src { | ||
// unwrap is safe because we allocate enough space | ||
write!(&mut s, "{:02x}", v).unwrap(); | ||
} | ||
s.serialize(serializer) |
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.
Also a function.
consensus-engine/src/utils.rs
Outdated
} | ||
} | ||
|
||
fn deserialize_human_readable_bytes_array<'de, const N: usize, D: serde::Deserializer<'de>>( |
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.
there has to be a crate that does this (e.g. hex
)
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.
Yeah, find a good one, switch to const-hex
.
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.
Looks good apart from the comment that I left
consensus-engine/src/utils.rs
Outdated
@@ -0,0 +1,66 @@ | |||
#[cfg(feature = "serde")] | |||
pub(crate) mod serde { |
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.
this should probably go in some more general module instead of consensus engine
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.
Yeah, cleaner, move it to the nomos-utils
crate.
* Human readable serde for CommitteeId * Deserialize bytes to string if human readable * Don't allocate if possible in human serde bytes
This PR solves the serialization/deserialization problem in OpenAPI, which was mentioned in Discord.