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

Message Generator: fix serialization and add into_static for serde_sv2 primitives, broken unit tests, new unit test for NMJ message, #949

Closed
wants to merge 1 commit into from
Closed

Message Generator: fix serialization and add into_static for serde_sv2 primitives, broken unit tests, new unit test for NMJ message, #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion protocols/Cargo.lock
View file
Open in desktop

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

30 changes: 30 additions & 0 deletions protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::{ShortTxId, Signature, B016M, B0255, B064K, U24, U256};
use crate::Error;
use alloc::vec::Vec;
use core::convert::TryInto;
use serde::{de::Visitor, Serialize};

Expand All @@ -22,6 +23,23 @@ struct Seq<'s, T: Clone + Serialize + TryFromBSlice<'s>> {
_a: core::marker::PhantomData<T>,
}

impl<'s, T: Clone + Serialize + TryFromBSlice<'s>> Seq<'s, T> {
pub fn parse(self) -> Result<Vec<T>, ()> {
let mut ret = Vec::with_capacity(self.size as usize);
let elem_len = self.size as usize;
let mut first = 0;
let mut last = elem_len;
while last <= self.data.len() {
let slice = &self.data[first..last];
let elem = T::try_from_slice(slice).map_err(|_| ())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an error variant here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An crate::error::Error::PrimitiveConversionError should be the right solution, what do you think?
BTW this part of code is used only in the message generator and not for production.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we can leave as is then

Copy link
Contributor

Choose a reason for hiding this comment

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

Having second thoughts about this. As it is exposed through the public API, it might make more sense to handle errors properly here.
Also, I have a small question:
If self.size is bigger than self.data.len, is returning empty array with self.size cap(the current behavior) makes sense or an error should be returned?

ret.push(elem);
first += elem_len;
last += elem_len;
}
Ok(ret)
}
}

struct SeqVisitor<T> {
inner_type_size: u8,
max_len: SeqMaxLen,
Expand Down Expand Up @@ -81,6 +99,18 @@ impl<'a> TryFromBSlice<'a> for bool {
}
}

impl<'a> TryFromBSlice<'a> for u8 {
type Error = Error;

#[inline]
fn try_from_slice(val: &'a [u8]) -> Result<Self, Error> {
if val.len() != 1 {
return Err(Error::InvalidU16Size(val.len()));
}
Ok(u8::from_le_bytes([val[0]]))
}
}

impl<'a> TryFromBSlice<'a> for u16 {
type Error = Error;

Expand Down
87 changes: 68 additions & 19 deletions protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ use super::{
};
use crate::primitives::{FixedSize, GetSize};
use alloc::vec::Vec;
use serde::{ser, ser::SerializeTuple, Deserialize, Deserializer, Serialize};
use debug_assert;
use serde::{
ser::{self, SerializeSeq, SerializeTuple},
Deserialize, Deserializer, Serialize,
};

#[derive(Debug, Clone)]
pub struct Sv2Option<'s, T: Serialize + TryFromBSlice<'s> + Clone> {
Expand Down Expand Up @@ -65,13 +69,28 @@ impl<'s, T: Clone + Serialize + TryFromBSlice<'s>> Serialize for Sv2Option<'s, T
seq.end()
}
(None, Some(data)) => {
let tuple = (data.len() as u8, &data[..]);
let mut seq = serializer.serialize_tuple(2)?;
seq.serialize_element(&tuple.0)?;
seq.serialize_element(tuple.1)?;
seq.end()
if serializer.is_human_readable() {
let data_ = data.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

u can use masking and do let data = data.clone();. I think its more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

you might also be able to remove this clone as serialize_element takes a reference

let mut seq = serializer.serialize_seq(Some(data_.len()))?;
for item in data_ {
seq.serialize_element(&item)?;
}
seq.end()
} else {
let tuple = (data.len() as u8, &data[..]);
let mut seq = serializer.serialize_tuple(2)?;
seq.serialize_element(&tuple.0)?;
seq.serialize_element(tuple.1)?;
seq.end()
}
}
_ => {
debug_assert!(
false,
"sv2option can never have boh fields of the same type"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an action expected from a user who hit this? regarding how to debug it? If is, its worth mentioning it IMO

);
panic!()
}
_ => panic!(),
}
}
}
Expand Down Expand Up @@ -219,15 +238,30 @@ impl<'a, T: Clone + FixedSize + Serialize + TryFromBSlice<'a>> GetSize for Sv2Op
}
impl<'s> Sv2Option<'s, U256<'s>> {
pub fn into_static(self) -> Sv2Option<'static, U256<'static>> {
if let Some(inner) = self.data {
let inner = inner.clone();
let data = inner.into_iter().map(|i| i.into_static()).collect();
Sv2Option {
seq: None,
data: Some(data),
match (self.data, self.seq) {
(None, Some(seq)) => {
let data = seq.parse().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to use .expect and explain why this should be Some

let data = data.into_iter().map(|i| i.into_static()).collect();
Sv2Option {
seq: None,
data: Some(data),
}
}
(Some(inner), None) => {
let inner = inner.clone();
let data = inner.into_iter().map(|i| i.into_static()).collect();
Sv2Option {
seq: None,
data: Some(data),
}
}
_ => {
debug_assert!(
false,
"sv2option can never have boh fields of the same type"
);
panic!()
}
} else {
panic!()
}
}
pub fn inner_as_ref(&self) -> &[&[u8]] {
Expand All @@ -236,13 +270,28 @@ impl<'s> Sv2Option<'s, U256<'s>> {
}
impl<'s> Sv2Option<'s, u32> {
pub fn into_static(self) -> Sv2Option<'static, u32> {
if let Some(inner) = self.data {
Sv2Option {
match (self.data, self.seq) {
(None, Some(seq)) => {
// this is an already valid seq should be safe to call the unwraps.
// also this library shouldn't be used for priduction envs so is ok do thigs like this
// one
let data = seq.parse().unwrap();
Sv2Option {
seq: None,
data: Some(data),
}
}
(Some(inner), None) => Sv2Option {
seq: None,
data: Some(inner),
},
_ => {
debug_assert!(
false,
"sv2option can never have boh fields of the same type"
);
panic!()
}
} else {
panic!()
}
}
}
Expand Down
84 changes: 65 additions & 19 deletions protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/seq0255.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ use crate::{
Error,
};
use alloc::vec::Vec;
use serde::{ser, ser::SerializeTuple, Deserialize, Deserializer, Serialize};
use serde::{
ser,
ser::{SerializeSeq, SerializeTuple},
Deserialize, Deserializer, Serialize,
};

#[derive(Debug, Clone)]
pub struct Seq0255<'s, T: Serialize + TryFromBSlice<'s> + Clone> {
Expand Down Expand Up @@ -68,11 +72,20 @@ impl<'s, T: Clone + Serialize + TryFromBSlice<'s>> Serialize for Seq0255<'s, T>
seq.end()
}
(None, Some(data)) => {
let tuple = (data.len() as u8, &data[..]);
let mut seq = serializer.serialize_tuple(2)?;
seq.serialize_element(&tuple.0)?;
seq.serialize_element(tuple.1)?;
seq.end()
if serializer.is_human_readable() {
let data_ = data.clone();
let mut seq = serializer.serialize_seq(Some(data_.len()))?;
for item in data_ {
seq.serialize_element(&item)?;
}
seq.end()
} else {
let tuple = (data.len() as u8, &data[..]);
let mut seq = serializer.serialize_tuple(2)?;
seq.serialize_element(&tuple.0)?;
seq.serialize_element(tuple.1)?;
seq.end()
}
}
_ => panic!(),
}
Expand Down Expand Up @@ -222,15 +235,33 @@ impl<'a, T: Clone + FixedSize + Serialize + TryFromBSlice<'a>> GetSize for Seq02
}
impl<'s> Seq0255<'s, U256<'s>> {
pub fn into_static(self) -> Seq0255<'static, U256<'static>> {
if let Some(inner) = self.data {
let inner = inner.clone();
let data = inner.into_iter().map(|i| i.into_static()).collect();
Seq0255 {
seq: None,
data: Some(data),
match (self.data, self.seq) {
(None, Some(seq)) => {
// this is an already valid seq should be safe to call the unwraps.
// also this library shouldn't be used for priduction envs so is ok do thigs like this
// one
let data = seq.parse().unwrap();
let data = data.into_iter().map(|i| i.into_static()).collect();
Seq0255 {
seq: None,
data: Some(data),
}
}
(Some(inner), None) => {
let inner = inner.clone();
let data = inner.into_iter().map(|i| i.into_static()).collect();
Seq0255 {
seq: None,
data: Some(data),
}
}
_ => {
debug_assert!(
false,
"sequences can never have boh fields of the same type"
);
panic!()
}
} else {
panic!()
}
}
pub fn inner_as_ref(&self) -> &[&[u8]] {
Expand All @@ -239,13 +270,28 @@ impl<'s> Seq0255<'s, U256<'s>> {
}
impl<'s> Seq0255<'s, u32> {
pub fn into_static(self) -> Seq0255<'static, u32> {
if let Some(inner) = self.data {
Seq0255 {
match (self.data, self.seq) {
(None, Some(seq)) => {
// this is an already valid seq should be safe to call the unwraps.
// also this library shouldn't be used for priduction envs so is ok do thigs like this
// one
let data = seq.parse().unwrap();
Seq0255 {
seq: None,
data: Some(data),
}
}
(Some(data), None) => Seq0255 {
seq: None,
data: Some(inner),
data: Some(data),
},
_ => {
debug_assert!(
false,
"sequences can never have boh fields of the same type"
);
panic!()
}
} else {
panic!()
}
}
}
Expand Down
Loading
Loading