-
Notifications
You must be signed in to change notification settings - Fork 131
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
Implement SerializeBytes
for TlsByteVecUX
types
#1133
Conversation
TlsByteVecUX
typesSerializeBytes
for TlsByteVecUX
types
9c745bd
to
0c23e29
Compare
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.
Thanks!
Only one nit.
tls_codec/src/tls_vec.rs
Outdated
@@ -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> { |
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 looks like a lot of duplicate code. Can you factor out some of it to reuse as much as possible?
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.
Done.
Bump, @franziskuskiefer could you please review the changes. |
@franziskuskiefer the changes you requested have been done, could you please review this PR. |
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.
Generally looks good to me but I think we lost the EncodingError
?
tls_serialized_len, written | ||
); | ||
if written != tls_serialized_len { | ||
return Err(Error::EncodingError(format!( |
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.
Why did you remove this check in assert_written_bytes
?
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 check is present in the assert_written_bytes
function: see line 191 in the new file.
Co-authored-by: Franziskus Kiefer <franziskuskiefer@gmail.com>
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.
Thanks! Sorry for the delay.
This PR implements the
SerializeBytes
trait for all theTlsByteVecUX
types. It also adds tests for serializing them using theSerializeBytes
trait.