-
Notifications
You must be signed in to change notification settings - Fork 122
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
Rust docs for protocols::v2::codec-sv2
#1040
base: main
Are you sure you want to change the base?
Rust docs for protocols::v2::codec-sv2
#1040
Conversation
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
This comment was marked as resolved.
This comment was marked as resolved.
protocols/v2/codec-sv2/src/error.rs
Outdated
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.
I did not change any code here, but I did reorder to be in alphabetical order.
@rrybarczyk let me know when you want a review here |
sv2_buffer: B, | ||
} | ||
|
||
#[cfg(feature = "noise_sv2")] | ||
impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> WithNoise<B, T> { | ||
/// Attempts to decode the next frame, returning either a frame or an error indicating how many |
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.
Can also return other kind of error. So it return either a frame, an error, or an error with missing bytes. I did it like that but I think that maybe here having and enum into the Ok and the error would have been a better interface. Not sure if make sense to change it now.
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 here an example would be nice. I just stop writing this but I think that when is possible we should add examples
@@ -157,17 +233,24 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit | |||
frame.into() | |||
} | |||
|
|||
/// Provides a writable buffer for incoming data. |
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 I would add that the buffer have the right number of bytes needed by the codec to go to the next step. I would also add an example of how to use the returned buffer.
pub type StandardDecoder<T> = WithoutNoise<Buffer, T>; | ||
|
||
/// Decoder for Sv2 frames with Noise protocol support. |
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.
here we can add an example of how to create and use a codec. If is possible to do it vai a doc test would be fantastic.
#[inline] | ||
pub fn writable(&mut self) -> &mut [u8] { | ||
self.noise_buffer.get_writable(self.missing_noise_b) | ||
} | ||
|
||
/// Checks if the buffers are droppable. |
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 link the Buffer trait docs (now is undocumented but as soon as we will document it we will have a meaningful link
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.
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
//! ## Modules | ||
//! | ||
//! * `decoder`: Provides functionality to decode Stratum V2 messages. | ||
//! * `encoder`: Provides functionality to encode Stratum V2 messages. | ||
//! * `error`: Defines error types and result aliases used throughout the crate. |
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.
a # Modules
section is automatically generated by Rust docs, so it is redundant to add one here
also, the encoder
and decoder
modules are not exposed
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.
assuming we will make encoder
and decoder
modules public, we should still remove this ## Modules
section
//! # Stratum V2 Codec Encoder | ||
//! | ||
//! This module provides functionality for encoding Stratum V2 messages, including support for the | ||
//! Noise protocol for secure communication. | ||
//! | ||
//! ## Features | ||
//! | ||
//! Two primary encoders are defined: | ||
//! * **Standard Encoder**: | ||
//! * **Noise Encoder**: | ||
//! | ||
//! ## Types | ||
//! | ||
//! * `Encoder`: Encoder for Sv2 frames with Noise protocol support. | ||
//! * `NoiseEncoder`: Encoder for Sv2 frames without Noise protocol support (requires the | ||
//! `noise_sv2` feature). | ||
//! * `Item<T>`: Represents an SV2 frame that is to be encoded and optionally encrypted using the | ||
//! Noise protocol. | ||
//! * `Buffer`: Holds intermediate data during encoding, which can be either a pool-allocated | ||
//! buffer or a system memory buffer depending on the feature flags. | ||
//! * `Slice`: Stores the serialized SV2 frame data before transmission. | ||
//! | ||
//! ## Usage | ||
//! | ||
//! This module is designed to be used to encode outgoing Sv2 messages, with optional Noise | ||
//! protocol encryption support for secure communication. | ||
//! | ||
//! ## Example | ||
//! | ||
//! ```ignore | ||
//! use codec_sv2::encoder::{Encoder, NoiseEncoder}; | ||
//! use framing_sv2::framing::Sv2Frame; | ||
//! | ||
//! // Create a standard encoder for unencrypted SV2 frames | ||
//! let mut encoder: Encoder<MyFrameType> = Encoder::new(); | ||
//! | ||
//! // Encode a frame into a byte slice | ||
//! let frame = Sv2Frame::new( /* frame details */ ); | ||
//! let encoded_data = encoder.encode(frame).unwrap(); | ||
//! | ||
//! // Create a Noise encoder for encrypted SV2 frames (requires the `noise_sv2` feature) | ||
//! #[cfg(feature = "noise_sv2")] | ||
//! let mut noise_encoder: NoiseEncoder<MyFrameType> = NoiseEncoder::new(); | ||
//! ``` |
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 header is not going to be displayed in Rust Docs, because the encoder
module is not exposed
overall I'd recommend to regularly do cargo doc --all-features --open
to render the docs so you can have a good idea of what the end-user is going to see
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.
I think we should still impose our documentation standards on private modules, regardless of whether it will be generated. Thoughts?
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.
no if a module is private we don't have to document it for user but for maintainers so is a different thing. In that case the module should not be private. A rule of thumb is, if you thing that you need to document a private module for the user maybe the module should not be private.
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.
I also think we should not document private modules. If there is some private module that is very vague, and we feel strongly about adding a comment, maybe we can use regular comment and not ///
?
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.
I agree with @rrybarczyk in the sense that it's important that we don't fully ignore parts of the code just because they are not exposed. Afterall, one of the main goals of this collective effort is for SRI contributors to refine their understanding (and ultimately, their ownership) over these crates.
I see @rrybarczyk also suggested cargo doc --document-private-items --all-features --open
on the other comment thread. I feel this could be dangerous because it could create a blindspot because we're not seeing what the user sees.
But I would follow @Fi3 advice and simply make encoder
and decoder
modules public:
--- a/protocols/v2/codec-sv2/src/lib.rs
+++ b/protocols/v2/codec-sv2/src/lib.rs
@@ -59,8 +59,8 @@ extern crate alloc;
#[cfg(feature = "noise_sv2")]
use alloc::boxed::Box;
-mod decoder;
-mod encoder;
+pub mod decoder;
+pub mod encoder;
But for everything that will not be public and we still want to document for maintainer eyes, we should do Non Doc comments, which are essentially //
inlines and /* ... */
blocks
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.
I formalized my proposal around Non Docs comments on the Discussion for Rust Docs Standards here #970 (comment)
//! # Stratum V2 Codec Decoder | ||
//! | ||
//! This module provides functionality for decoding Stratum V2 messages, including support for | ||
//! the Noise protocol for secure communication. | ||
//! | ||
//! ## Features | ||
//! | ||
//! * **Standard Decoder**: Decodes Stratum V2 frames without encryption. | ||
//! * **Noise Decoder**: Decodes Stratum V2 frames with Noise protocol encryption. | ||
//! | ||
//! ## Types | ||
//! | ||
//! * `WithoutNoise`: Decoder for Sv2 frames without Noise protocol support. | ||
//! * `WithNoise`: Decoder for Sv2 frames with Noise protocol support (requires the `noise_sv2` | ||
//! feature). | ||
//! * `StandardEitherFrame`: Represents an encoded or decoded frame that could be either a regular | ||
//! or Noise-protected frame. | ||
//! * `StandardSv2Frame`: Represents an encoded or decoded Stratum V2 frame. | ||
//! * `StandardNoiseDecoder`: Decoder for Stratum V2 frames with Noise protocol support. | ||
//! * `StandardDecoder`: Decoder for Stratum V2 frames without Noise protocol support. | ||
//! | ||
//! ## Usage | ||
//! | ||
//! This module is designed to be used to decode incoming Sv2 messages, with optional Noise | ||
//! protocol encryption support for secure communication. | ||
//! | ||
//! ## Example | ||
//! | ||
//! ```ignore | ||
//! use codec_sv2::decoder::{StandardDecoder, StandardNoiseDecoder}; | ||
//! | ||
//! // Create a standard decoder | ||
//! let mut decoder: StandardDecoder<MyFrameType> = StandardDecoder::new(); | ||
//! | ||
//! // Create a noise decoder (requires the `noise_sv2` feature) | ||
//! #[cfg(feature = "noise_sv2")] | ||
//! let mut noise_decoder: StandardNoiseDecoder<MyFrameType> = StandardNoiseDecoder::new(); | ||
//! ``` | ||
//! | ||
|
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 header is not going to be displayed in Rust Docs, because the decoder
module is not exposed
overall I'd recommend to regularly do cargo doc --all-features --open
to render the docs so you can have a good idea of what the end-user is going to see
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.
I think that we can change how we export thing in order to include it without changing the API
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.
sorry @Fi3 I don't fully understand, can you elaborate?
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.
We can add the --document-private-items
to expose documentation for private modules like:
cargo doc --document-private-items --all-features --open
I left a comment here with some thoughts.
Essentially, I am not sure if you can publish the private crate documentation to crates.io, or if we would want to do that. BUT, we can use it to guide us and view our docs on private modules while we develop the documentation.
Either way, my example here sucks 😝.
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.
yep or we can make the module public in this case: we make codec_sv2::decoder public and in the module we make private all the things that are not exported by lib.rs so we don't change the api.
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.
I agree, we should make these modules public.
Co-authored-by: plebhash <[email protected]>
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.
Few nits.
/// Transport mode where AEAD is fully operational. The `TransportMode` object in this variant | ||
/// as able to perform encryption and decryption resp. | ||
|
||
/// The codec is in transport mode, where AEAS encryption and decryption are fully operational. |
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.
/// The codec is in transport mode, where AEAS encryption and decryption are fully operational. | |
/// The codec is in transport mode, where AEAD encryption and decryption are fully operational. |
//! * `Encoder`: Encoder for Sv2 frames with Noise protocol support. | ||
//! * `NoiseEncoder`: Encoder for Sv2 frames without Noise protocol support (requires the | ||
//! `noise_sv2` feature). |
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.
//! * `Encoder`: Encoder for Sv2 frames with Noise protocol support. | |
//! * `NoiseEncoder`: Encoder for Sv2 frames without Noise protocol support (requires the | |
//! `noise_sv2` feature). | |
//! * `Encoder`: Encoder for Sv2 frames without Noise protocol support (requires the | |
//! `noise_sv2` feature). | |
//! * `NoiseEncoder`: Encoder for Sv2 frames with Noise protocol support. |
This should be the other way around.
//! ## Example | ||
//! | ||
//! ```ignore | ||
//! use codec_sv2::encoder::{Encoder, NoiseEncoder}; | ||
//! use framing_sv2::framing::Sv2Frame; | ||
//! | ||
//! // Create a standard encoder for unencrypted SV2 frames | ||
//! let mut encoder: Encoder<MyFrameType> = Encoder::new(); | ||
//! | ||
//! // Encode a frame into a byte slice | ||
//! let frame = Sv2Frame::new( /* frame details */ ); | ||
//! let encoded_data = encoder.encode(frame).unwrap(); | ||
//! | ||
//! // Create a Noise encoder for encrypted SV2 frames (requires the `noise_sv2` feature) | ||
//! #[cfg(feature = "noise_sv2")] | ||
//! let mut noise_encoder: NoiseEncoder<MyFrameType> = NoiseEncoder::new(); | ||
//! ``` |
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.
I'm not entirely sure if writing this is really helpful. It could be misleading for someone referencing it, as they might assume these are public-facing APIs. I believe we should include code snippets only for public-facing modules, rather than for all modules.
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.
I think we should make these modules public
but with regards to the broader discussion around this topic, let's try to reach consensus here #970 (comment)
//! ```ignore | ||
//! use codec_sv2::{State, HandshakeRole, NoiseCodec}; | ||
//! use noise_sv2::Initiator; | ||
//! | ||
//! // Initialize the codec state | ||
//! let role = HandshakeRole::Initiator(Box::new(Initiator::new())); | ||
//! let mut state = State::not_initialized(&role); | ||
//! state = State::initialized(role); | ||
//! | ||
//! // Proceed with the handshake steps | ||
//! match state.step_0() { | ||
//! Ok(frame) => { | ||
//! // Send the initial handshake frame | ||
//! } | ||
//! Err(e) => { | ||
//! // Handle 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.
Here, we can have a doc test.
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.
we should do some research to figure out the best way to integrate doc tests into our CI
//! ```ignore | ||
//! use codec_sv2::{State, HandshakeRole, NoiseCodec}; | ||
//! use noise_sv2::Initiator; | ||
//! | ||
//! // Initialize the codec state | ||
//! let role = HandshakeRole::Initiator(Box::new(Initiator::new())); | ||
//! let mut state = State::not_initialized(&role); | ||
//! state = State::initialized(role); | ||
//! | ||
//! // Proceed with the handshake steps | ||
//! match state.step_0() { | ||
//! Ok(frame) => { | ||
//! // Send the initial handshake frame | ||
//! } | ||
//! Err(e) => { | ||
//! // Handle 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 example is going to be the first thing the reader sees when the look at the Rust Docs for codec_sv2
crate
if the main example is going to include encryption, we should do at least two code blocks:
- one example where
Initiator
does the full handshake cycle, then encodes a message, and sends it - one example where
Receiver
does the full handshake cycle, then decodes a received message
as an alternative, we could also just simplify things by doing an unencrypted example
I'm working on something, I'll propose it here once I know it compiles.
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.
I wrote a replit with an atomic unencrypted encode/decode example: https://replit.com/@plebhash/GummyLividClicks
(replit is good because whatever we write as an example, we know it's functional and buildable code)
we have a good chunk of ~90 lines of code there, and a fully functional example including encryption would probably take a lot more lines
so I would lean towards either:
- showing an unencrypted example code on the top of
codec_sv2
Rust Docs - not showing any example code on the top of
codec_sv2
Rust Docs, and create aprotocols/v2/codec_sv2/examples
directory where we have encrypted and unencrypted examples
if we decide to create an examples directory, we should create an issue so we can tackle that in the future (instead of this PR)
/// Used to maintain type information for the generic parameter `T`, which represents the type | ||
/// of frames being decoded. | ||
/// | ||
/// `T` refers to a type that implements the necessary traits for serialization | ||
/// (`binary_sv2::Serialize`), deserialization (`binary_sv2::Deserialize`), and size | ||
/// calculation (`binary_sv2::GetSize`). |
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.
private fields are never exposed to reader eyes
it's good to document this for maintainer eyes, but we should use Non Doc comments (//
inlines and /* ... */
blocks), as proposed in #970 (comment)
|
||
/// Number of missing bytes needed to complete the Noise header or payload. | ||
/// | ||
/// Keeps track of how many more bytes are required to fully decode the current Noise frame. |
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.
ditto
|
||
/// Buffer for holding encrypted Noise data. | ||
/// | ||
/// Stores the incoming encrypted data until it is ready to be decrypted and processed. |
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.
ditto
|
||
/// Buffer for holding decrypted Sv2 data. | ||
/// | ||
/// Stores the decrypted data from the Noise protocol until it is ready to be processed and | ||
/// concerted into a Sv2 frame. |
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.
ditto
/// When the `with_buffer_pool` feature is enabled, `Buffer` is a pool-allocated buffer type | ||
/// (`BufferPool`), which allows for more efficient memory management. Otherwise, it defaults to | ||
/// `BufferFromSystemMemory`. |
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.
we should reproduce this comment on decoder.rs
because it also has a type alias for Buffer
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.
great progess!
left some comments
Addresses #1012.
See discussion for potential code debt discovered during this documentation effort in this discussion.