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

Implement SerializeBytes for TlsByteVecUX types #1133

Merged

Conversation

imor
Copy link
Contributor

@imor imor commented Jul 2, 2023

This PR implements the SerializeBytes trait for all the TlsByteVecUX types. It also adds tests for serializing them using the SerializeBytes trait.

@imor imor changed the title Implement SerializeBytes for TlsByteVecUX types Implement SerializeBytes for TlsByteVecUX types Jul 2, 2023
@imor imor marked this pull request as ready for review July 2, 2023 11:23
@imor imor force-pushed the impl_serialize_bytes_for_vecs branch from 9c745bd to 0c23e29 Compare July 3, 2023 02:59
@franziskuskiefer franziskuskiefer self-requested a review July 3, 2023 08:02
Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks!
Only one nit.

@@ -78,6 +78,45 @@ macro_rules! impl_byte_deserialize {
let result = Self { vec: vec.to_vec() };
Ok((result, &remainder.get(len..).ok_or(Error::EndOfStream)?))
}

fn serialize_bytes_bytes(&$self) -> Result<Vec<u8>, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a lot of duplicate code. Can you factor out some of it to reuse as much as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@imor
Copy link
Contributor Author

imor commented Jul 9, 2023

Bump, @franziskuskiefer could you please review the changes.

@imor
Copy link
Contributor Author

imor commented Jul 20, 2023

@franziskuskiefer the changes you requested have been done, could you please review this PR.

Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Generally looks good to me but I think we lost the EncodingError?

tls_codec/src/tls_vec.rs Show resolved Hide resolved
tls_serialized_len, written
);
if written != tls_serialized_len {
return Err(Error::EncodingError(format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this check in assert_written_bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is present in the assert_written_bytes function: see line 191 in the new file.

Co-authored-by: Franziskus Kiefer <franziskuskiefer@gmail.com>
Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry for the delay.

@franziskuskiefer franziskuskiefer merged commit 90a4324 into RustCrypto:master Aug 10, 2023
12 checks passed
@imor imor deleted the impl_serialize_bytes_for_vecs branch August 10, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants