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

Framing refactor: framing.rs api #1033

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+198 −203
Draft

Framing refactor: framing.rs api #1033

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
005e6c9
Constrain `T` and `B` generics
jbesraa Jul 4, 2024
e613a46
Change `payload` fn signature to return Option
jbesraa Jul 4, 2024
d0aae41
Change `Sv2Frame::get_header` to `header` and ..
jbesraa Jul 4, 2024
d1d9247
Change `Sv2Frame` from struct to enum
jbesraa Jul 16, 2024
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
92 changes: 44 additions & 48 deletions protocols/v2/framing-sv2/src/framing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> From<Sv2Frame<T, B>>

/// Abstraction for a SV2 Frame.
Copy link
Collaborator

@plebhash plebhash Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we should expand this comment to make it clear what each enum variant is

maybe something like:

/// Both `Payload` and `Raw` variants carry the `Header` as metadata
/// `Payload` means the `Sv2Frame` carries a payload, but it has not yet been serialized
/// `Raw` means the `Sv2Frame` has been fully serialized, where the `serialized` field contains both `header` + `payload`

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[derive(Debug, Clone)]
pub struct Sv2Frame<T, B> {
header: Header,
payload: Option<T>,
/// Serialized header + payload
serialized: Option<B>,
pub enum Sv2Frame<T, B> {
Payload { header: Header, payload: T },
Raw { header: Header, serialized: B },
}

impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
Expand All @@ -57,23 +55,23 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// When called on a non serialized frame, it is not so cheap (because it serializes it).
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[inline]
pub fn serialize(self, dst: &mut [u8]) -> Result<(), Error> {
if let Some(mut serialized) = self.serialized {
dst.swap_with_slice(serialized.as_mut());
Ok(())
} else if let Some(payload) = self.payload {
#[cfg(not(feature = "with_serde"))]
to_writer(self.header, dst).map_err(Error::BinarySv2Error)?;
#[cfg(not(feature = "with_serde"))]
to_writer(payload, &mut dst[Header::SIZE..]).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&self.header, dst.as_mut()).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&payload, &mut dst.as_mut()[Header::SIZE..])
.map_err(Error::BinarySv2Error)?;
Ok(())
} else {
// Sv2Frame always has a payload or a serialized payload
panic!("Impossible state")
match self {
Sv2Frame::Raw { mut serialized, .. } => {
dst.swap_with_slice(serialized.as_mut());
Ok(())
}
Sv2Frame::Payload { header, payload } => {
#[cfg(not(feature = "with_serde"))]
to_writer(header, dst).map_err(Error::BinarySv2Error)?;
#[cfg(not(feature = "with_serde"))]
to_writer(payload, &mut dst[Header::SIZE..]).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&header, dst.as_mut()).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&payload, &mut dst.as_mut()[Header::SIZE..])
.map_err(Error::BinarySv2Error)?;
Ok(())
}
}
}

Expand All @@ -83,16 +81,18 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// already serialized payload. If the frame has not yet been
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

/// serialized, this function should never be used (it will panic).
pub fn payload(&mut self) -> Option<&mut [u8]> {
Copy link
Collaborator

@plebhash plebhash Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

shouldn't we rename this method?

with the new enum we are sort of establishing a convention where the Payload variant signals that Sv2Frame has not yet been serialized

so calling a method that is named payload for a Sv2Frame::Payload and getting a None is extremely counter intuitive!

I feel we would create a more sane user experience if this method was called raw or serialized

if let Some(serialized) = self.serialized.as_mut() {
Some(&mut serialized.as_mut()[Header::SIZE..])
} else {
None
match self {
Sv2Frame::Raw { serialized, .. } => Some(&mut serialized.as_mut()[Header::SIZE..]),
Sv2Frame::Payload { .. } => None,
}
}

/// `Sv2Frame` always returns `Some(self.header)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

pub fn header(&self) -> crate::header::Header {
self.header
match self {
Self::Payload { header, .. } => *header,
Self::Raw { header, .. } => *header,
}
}

/// Tries to build a `Sv2Frame` from raw bytes, assuming they represent a serialized `Sv2Frame` frame (`Self.serialized`).
Expand All @@ -113,10 +113,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
pub fn from_bytes_unchecked(mut bytes: B) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Github doesn't allow me to add this comment on the lines above. Ideally this comment should be placed on line 98, but placing it here should be sufficient

the comments on from_bytes are outdated with regards to the new enum-based Sv2Frame

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

// Unchecked function caller is supposed to already know that the passed bytes are valid
let header = Header::from_bytes(bytes.as_mut()).expect("Invalid header");
Self {
Self::Raw {
header,
payload: None,
serialized: Some(bytes),
serialized: bytes,
}
}

Expand Down Expand Up @@ -150,13 +149,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// otherwise, returns the length of `self.payload`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[inline]
pub fn encoded_length(&self) -> usize {
if let Some(serialized) = self.serialized.as_ref() {
serialized.as_ref().len()
} else if let Some(payload) = self.payload.as_ref() {
payload.get_size() + Header::SIZE
} else {
// Sv2Frame always has a payload or a serialized payload
panic!("Impossible state")
match self {
Sv2Frame::Raw { serialized, .. } => serialized.as_ref().len(),
Sv2Frame::Payload { payload, .. } => payload.get_size() + Header::SIZE,
}
}

Expand All @@ -170,25 +165,26 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
) -> Option<Self> {
let extension_type = update_extension_type(extension_type, channel_msg);
let len = message.get_size() as u32;
Header::from_len(len, message_type, extension_type).map(|header| Self {
Header::from_len(len, message_type, extension_type).map(|header| Self::Payload {
header,
payload: Some(message),
serialized: None,
payload: message,
})
}
}

impl<A: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<A, B> {
/// Maps a `Sv2Frame<A, B>` to `Sv2Frame<C, B>` by applying `fun`,
/// which is assumed to be a closure that converts `A` to `C`
pub fn map<C>(self, fun: fn(A) -> C) -> Sv2Frame<C, B> {
let serialized = self.serialized;
let header = self.header;
let payload = self.payload.map(fun);
Sv2Frame {
header,
payload,
serialized,
pub fn map<C>(self, fun: fn(A) -> C) -> Sv2Frame<C, B>
where
C: Serialize + GetSize,
{
match self {
Sv2Frame::Raw { header, serialized } => Sv2Frame::Raw { header, serialized },
Sv2Frame::Payload { header, payload } => Sv2Frame::Payload {
header,
payload: fun(payload),
},
}
}
}
Expand Down
Loading