feat(network): dial peer in network run function

[nagmo-starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[nagmo-starkware]

Current dependencies on/for this PR:

• feat(network): update main for network run function return error #1740
• feat(sync): add p2p header sync #1729
• feat(network): dial peer in network run function #1739 👈
• main

This stack of pull requests is managed by Graphite.

[ShahakShama]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nagmo-starkware)

---

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

    pub async fn run(mut self) -> Result<(), NetworkError> {
        if let Some(peer_id) = self.peer_id {
            debug!("Starting network manager with peer id: {peer_id:?}");

Consider "with" -> "connected to"
and "without peer id" -> "not connected to any peer"

---

crates/papyrus_network/src/network_manager/test.rs line 114 at r1 (raw file):

    fn dial(&mut self, peer_id: PeerId) -> Result<(), libp2p::swarm::DialError> {
        unimplemented!("MockSwarm::dial not implemented")

Consider just returning Ok(())

[nagmo-starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)

---

crates/papyrus_network/src/network_manager/test.rs line 114 at r1 (raw file):

Previously, ShahakShama wrote…

Consider just returning Ok(())

I prefer to make it obvious that it's not dealt with

[codecov]

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 72.98%. Comparing base (1e6bf4c) to head (cb26a60).

Files	Patch %	Lines
...papyrus_network/src/network_manager/swarm_trait.rs	0.00%	6 Missing ⚠️
crates/papyrus_network/src/network_manager/mod.rs	33.33%	3 Missing and 1 partial ⚠️
...apyrus_network/src/bin/p2p_integration_endpoint.rs	0.00%	1 Missing ⚠️
crates/papyrus_node/src/main.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1739      +/-   ##
==========================================
- Coverage   73.03%   72.98%   -0.05%     
==========================================
  Files         131      131              
  Lines       18955    18967      +12     
  Branches    18955    18967      +12     
==========================================
+ Hits        13843    13844       +1     
- Misses       3015     3025      +10     
- Partials     2097     2098       +1     

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

[ShahakShama]

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/bin/p2p_integration_endpoint.rs line 48 at r3 (raw file):

        network_manager.dial(dial_address);
    }
    let _ = network_manager.run().await;

You should handle the error, not ignore it

---

crates/papyrus_network/src/network_manager/swarm_trait.rs line 53 at r3 (raw file):

    fn dial(&mut self, peer_id: PeerId) -> Result<(), DialError> {
        let addresses = vec![
            Multiaddr::from_str("/ip4/127.0.0.1/tcp/10000").expect("string to multiaddr failed"),

Why is this a hardcoded address?

---

crates/papyrus_node/src/main.rs line 163 at r3 (raw file):

    let network_manager =
        network_manager::NetworkManager::new(network_config.clone(), storage_reader.clone());
    network_manager.run().await.expect("network manager failed");

Handle the error here in the same way it's handled with Sync. Mainly, I want to see the error when it happens

[nagmo-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_network/src/bin/p2p_integration_endpoint.rs line 48 at r3 (raw file):

Previously, ShahakShama wrote…

You should handle the error, not ignore it

it's the integration endpoint.. we expect it to not return and if it returns then the endpoint will shut down

---

crates/papyrus_network/src/network_manager/swarm_trait.rs line 53 at r3 (raw file):

Previously, ShahakShama wrote…

Why is this a hardcoded address?

it's temporary until I'll have the PR that adds the peer stuff to the config

---

crates/papyrus_node/src/main.rs line 163 at r3 (raw file):

Previously, ShahakShama wrote…

Handle the error here in the same way it's handled with Sync. Mainly, I want to see the error when it happens

you've already approved the PR that fixes it. it's merged into the p2psync connection to main branch.
I'll see if I can extract the part that handles this from it and merge it to main before the rest.
for now I suggest to merge it since I agree it looks better the other way but the functionality is the same.

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/bin/p2p_integration_endpoint.rs line 48 at r3 (raw file):

Previously, nagmo-starkware wrote…

it's the integration endpoint.. we expect it to not return and if it returns then the endpoint will shut down

Just do expect("Network manager failed")
so that the process will exit with error and not with 0
you can do it in a future PR if you prefer

[ShahakShama]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nagmo-starkware)