chore(network): create docstrings for network manager #2250

eitanm-starkware · 2024-07-24T10:11:57Z

This change is

codecov · 2024-07-24T10:27:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.34%. Comparing base (3bcfcc6) to head (a5d3ac0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2250   +/-   ##
=======================================
  Coverage   66.34%   66.34%           
=======================================
  Files         139      139           
  Lines       18379    18379           
  Branches    18379    18379           
=======================================
  Hits        12194    12194           
  Misses       4890     4890           
  Partials     1295     1295

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ShahakShama

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 14 unresolved discussions (waiting on @eitanm-starkware)

crates/papyrus_network/src/network_manager/mod.rs line 1 at r1 (raw file):

mod swarm_trait;

Add a mod documentation block (//!), stating that this module contains a NetworkManager which runs the network

crates/papyrus_network/src/network_manager/mod.rs line 58 at r1 (raw file):

impl<SwarmT: SwarmTrait> GenericNetworkManager<SwarmT> {
    /// Starts processing network events. This function runs indefinitely, handling connections,

This docstring is a bit long. Try to move as much information as possible to the module documentation or to the struct documentation and state here that this runs the NetworkManager

crates/papyrus_network/src/network_manager/mod.rs line 59 at r1 (raw file):

impl<SwarmT: SwarmTrait> GenericNetworkManager<SwarmT> {
    /// Starts processing network events. This function runs indefinitely, handling connections,
    /// disconnections, messages, and other network-related events as they occur.

Put messages first, as it is the most important of the stuff you've mentioned here.

You should also probably mention discovery and peer choosing before connections and disconnections. Maybe you can use other words for that, I'll leave it up to you

crates/papyrus_network/src/network_manager/mod.rs line 61 at r1 (raw file):

    /// disconnections, messages, and other network-related events as they occur.
    ///
    /// The function processes events from the `swarm`, handles incoming and outgoing SQMR messages,

Remove the swarm part. It's an implementation detail

crates/papyrus_network/src/network_manager/mod.rs line 62 at r1 (raw file):

    ///
    /// The function processes events from the `swarm`, handles incoming and outgoing SQMR messages,
    /// and manages broadcasts and peer reports. It utilizes `tokio::select!` to concurrently

remove the part about tokio::select. It's also an implementation detail

crates/papyrus_network/src/network_manager/mod.rs line 66 at r1 (raw file):

    ///
    /// # Examples
    /// ```rust

I've already added code examples in #2243. I think you can remove them from all this file (those that I added a TODO to write you can copy what you did here and paste it there)`

crates/papyrus_network/src/network_manager/mod.rs line 126 at r1 (raw file):

    /// This function will panic if the protocol is already registered.
    ///
    /// The `SqmrServerReceiver` returned by this function can be used in an asynchronous loop

remove "in an asynchronous loop". IMO it's redundant

crates/papyrus_network/src/network_manager/mod.rs line 127 at r1 (raw file):

    ///
    /// The `SqmrServerReceiver` returned by this function can be used in an asynchronous loop
    /// to process incoming queries.

and send responses and reports to those queries

crates/papyrus_network/src/network_manager/mod.rs line 129 at r1 (raw file):

    /// to process incoming queries.
    ///
    /// # Parameters

Could you make sure this is the format for documenting the parameters and return type of a function in rust

crates/papyrus_network/src/network_manager/mod.rs line 131 at r1 (raw file):

    /// # Parameters
    /// - `protocol`: Identifier for the protocol.
    /// - `buffer_size`: The size of the internal message buffer.

The size of the buffer in the returned channel

crates/papyrus_network/src/network_manager/mod.rs line 193 at r1 (raw file):

    }

    /// Registers a client-side SQMR protocol with a buffer for outgoing messages.

messages -> queries

crates/papyrus_network/src/network_manager/mod.rs line 197 at r1 (raw file):

    ///
    /// The `SqmrClientSender` returned by this function is used to send queries and asynchronously
    /// handle responses.

and report them

crates/papyrus_network/src/network_manager/mod.rs line 201 at r1 (raw file):

    /// # Parameters
    /// - `protocol`: Identifier for the protocol.
    /// - `buffer_size`: The size of the internal message buffer.

Same as before

crates/papyrus_network/src/network_manager/mod.rs line 272 at r1 (raw file):

    /// The `BroadcastSubscriberChannels` structure contains senders and receivers for managing
    /// broadcast messages.
    ///

Add an important note that if the node is done for a short period all messages sent in that period will be missed, and that while most messages will reach all transitively-connected peers, it is not guaranteed and misses can occur

eitanm-starkware

Reviewable status: all files reviewed (commit messages unreviewed), 14 unresolved discussions (waiting on @ShahakShama)

crates/papyrus_network/src/network_manager/mod.rs line 1 at r1 (raw file):