sdk/python/localcluster/cluster.py:130

• [nitpick] The function name 'remove_nodes_to_network_registry' can be misleading; it may be clearer if renamed to 'remove_nodes_from_network_registry' to better reflect its purpose.

    def remove_nodes_to_network_registry(self):

transport/p2p/src/behavior/discovery.rs (1)

74-77: Improve log message clarity for connection decisions.

While the current trace messages are more descriptive than before, they could still be clearer about the actual decision being made (connection allowed vs denied).

Consider this improvement:

-            trace!(%peer, "Discovery::handle_established_inbound_connection in allowed_peers");
+            trace!(%peer, "Discovery::handle_established_inbound_connection: Connection allowed");

-            trace!(%peer, "Discovery::handle_established_inbound_connection NOT in allowed_peers");
+            trace!(%peer, "Discovery::handle_established_inbound_connection: Connection denied");

transport/protocol/src/lib.rs (1)

126-133: Document the breaking change and the boolean parameter's purpose.

The addition of a boolean parameter to PeerDiscovery::Announce is a breaking change that affects all consumers of this enum. Consider adding documentation to clarify:

1. The purpose of the boolean parameter (represents network registry allowance status)
2. The breaking nature of this change in the changelog or migration guide

Add documentation to the enum variant:

 pub enum PeerDiscovery {
     Allow(PeerId),
     Ban(PeerId),
+    /// Announce a peer with their multiaddresses and network registry allowance status.
+    /// The boolean parameter indicates whether the peer is allowed by the network registry.
     Announce(PeerId, Vec<Multiaddr>, bool),
 }

hopr/hopr-lib/src/lib.rs (1)

234-240: Consider logging database errors during network registry check.

While the implementation correctly checks the network registry, using .unwrap_or(false) silently ignores potential database errors. Consider logging these errors for better observability.

                ChainEventType::Announcement{peer, address, multiaddresses} => {
                    let allowed = db
                        .is_allowed_in_network_registry(None, &address)
                        .await
-                        .unwrap_or(false);
+                        .unwrap_or_else(|e| {
+                            error!(error = %e, %address, "Failed to check network registry status");
+                            false
+                        });
                    Some(PeerDiscovery::Announce(peer, multiaddresses, allowed))
                }

transport/p2p/src/behavior/discovery.rs (1)

100-100: Apply consistent log message clarity improvements.

Similar to the inbound connection handler, these trace messages could be clearer about the connection decision.

Apply consistent messaging:

-                trace!(%peer, "Discovery::handle_pending_outbound_connection in allowed_peers");
+                trace!(%peer, "Discovery::handle_pending_outbound_connection: Connection allowed");

-                trace!(%peer, "Discovery::handle_pending_outbound_connection NOT in allowed_peers");
+                trace!(%peer, "Discovery::handle_pending_outbound_connection: Connection denied");

Also applies to: 110-110

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6313
File: hopr/hopr-lib/src/lib.rs:0-0
Timestamp: 2024-07-28T07:26:06.634Z
Learning: The types `IndexerTransportEvent`, `Network`, `PeerEligibility`, and `PeerOrigin` in the `hopr/hopr-lib/src/lib.rs` file are internal to the transport mechanism and do not require public documentation.

transport/p2p/tests/p2p_transport_test.rs (1)

131-142: Consider adding test coverage for the new allowed parameter.

While the current changes correctly update the test for the new API, the test only covers the allowed=true scenario. Consider adding a separate test case to verify the behavior when allowed=false to ensure complete coverage of the new network registry functionality.

Do you want me to help create an additional test case that covers the allowed=false scenario?

scripts/protocol-config-anvil.json (1)

27-36: Reduce duplication in network definitions.

Both anvil-localhost and anvil-localhost2 share identical address blocks. To simplify maintenance and avoid drift, consider extracting a shared base configuration (e.g., via JSON merge/extend or a script) and referencing it for both networks.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro