feat: expose local_socket_addrs on Transport trait for ICE candidate extraction#478
Draft
HexaField wants to merge 1 commit intoholochain:mainfrom
Draft
feat: expose local_socket_addrs on Transport trait for ICE candidate extraction#478HexaField wants to merge 1 commit intoholochain:mainfrom
HexaField wants to merge 1 commit intoholochain:mainfrom
Conversation
|
✔️ 7ba61fe - Conventional commits check succeeded. |
WalkthroughThis PR introduces a new transport API for discovering local socket addresses. It adds trait methods across the transport abstraction layers, implements address discovery in the Iroh transport with IPv4/IPv6 filtering, and validates the functionality through integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Mar 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api/src/transport.rs (1)
529-534: Consider documenting behavioral differences across transport implementations.Based on the relevant code snippets,
Tx5Transporthasget_listening_addresses()which could potentially expose socket addresses, but it doesn't overridelocal_socket_addrs()and will return an emptyVec. Similarly,MemTransportwill silently return empty.This is acceptable given the PR's design intent (feature only for Iroh), but consider adding a note in the documentation that this method currently only returns meaningful results for the Iroh transport implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/api/src/transport.rs` around lines 529 - 534, Add documentation to the trait method local_socket_addrs explaining that not all Transport implementations return socket addresses; specifically note that Tx5Transport (see get_listening_addresses) and MemTransport currently return an empty Vec and that meaningful results are only provided by the Iroh transport implementation. Update the doc comment on fn local_socket_addrs(&self) to call out this behavioral difference and the reason (feature currently implemented only for Iroh), so callers know to expect empty results for other transports like Tx5Transport and MemTransport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/transport_iroh/tests/integration.rs`:
- Around line 1254-1278: The test local_socket_addrs_excludes_unspecified can
pass with empty results; after the retry loop that sets addrs via
transport.local_socket_addrs(), add an assertion that addrs is non-empty (e.g.,
assert!(!addrs.is_empty(), "...")) before iterating to filter unspecified
addresses so the test fails if discovery did not produce any addresses;
reference the IrohTransportTestHarness and transport.local_socket_addrs() in the
change and mirror the behavior used in local_socket_addrs_returns_addresses if
helpful.
---
Nitpick comments:
In `@crates/api/src/transport.rs`:
- Around line 529-534: Add documentation to the trait method local_socket_addrs
explaining that not all Transport implementations return socket addresses;
specifically note that Tx5Transport (see get_listening_addresses) and
MemTransport currently return an empty Vec and that meaningful results are only
provided by the Iroh transport implementation. Update the doc comment on fn
local_socket_addrs(&self) to call out this behavioral difference and the reason
(feature currently implemented only for Iroh), so callers know to expect empty
results for other transports like Tx5Transport and MemTransport.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b75408c-0e1c-460f-95f2-052a3a52118c
📒 Files selected for processing (4)
crates/api/src/transport.rscrates/transport_iroh/src/endpoint.rscrates/transport_iroh/src/lib.rscrates/transport_iroh/tests/integration.rs
Comment on lines
+1254
to
+1278
| #[tokio::test] | ||
| async fn local_socket_addrs_excludes_unspecified() { | ||
| let harness = IrohTransportTestHarness::new().await; | ||
| let handler = Arc::new(MockTxHandler::default()); | ||
| let transport = harness.build_transport(handler.clone()).await; | ||
|
|
||
| // Retry for up to 5 seconds to allow address discovery. | ||
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); | ||
| let mut addrs; | ||
| loop { | ||
| addrs = transport.local_socket_addrs().await.unwrap(); | ||
| if !addrs.is_empty() || std::time::Instant::now() >= deadline { | ||
| break; | ||
| } | ||
| tokio::time::sleep(std::time::Duration::from_millis(200)).await; | ||
| } | ||
|
|
||
| // Verify no unspecified addresses (0.0.0.0 or ::) are returned. | ||
| for addr in &addrs { | ||
| assert!( | ||
| !addr.ip().is_unspecified(), | ||
| "Unspecified address should be filtered out: {addr}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Test may pass trivially with empty results.
The local_socket_addrs_excludes_unspecified test loops until addresses are found or timeout, but doesn't assert that addresses were actually discovered. If address discovery fails, the for loop body never executes and the test passes without validating anything.
Consider adding a non-empty assertion or combining this with local_socket_addrs_returns_addresses:
Proposed fix
// Verify no unspecified addresses (0.0.0.0 or ::) are returned.
+ assert!(
+ !addrs.is_empty(),
+ "Expected at least one address to verify unspecified filtering"
+ );
for addr in &addrs {
assert!(
!addr.ip().is_unspecified(),
"Unspecified address should be filtered out: {addr}"
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[tokio::test] | |
| async fn local_socket_addrs_excludes_unspecified() { | |
| let harness = IrohTransportTestHarness::new().await; | |
| let handler = Arc::new(MockTxHandler::default()); | |
| let transport = harness.build_transport(handler.clone()).await; | |
| // Retry for up to 5 seconds to allow address discovery. | |
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); | |
| let mut addrs; | |
| loop { | |
| addrs = transport.local_socket_addrs().await.unwrap(); | |
| if !addrs.is_empty() || std::time::Instant::now() >= deadline { | |
| break; | |
| } | |
| tokio::time::sleep(std::time::Duration::from_millis(200)).await; | |
| } | |
| // Verify no unspecified addresses (0.0.0.0 or ::) are returned. | |
| for addr in &addrs { | |
| assert!( | |
| !addr.ip().is_unspecified(), | |
| "Unspecified address should be filtered out: {addr}" | |
| ); | |
| } | |
| } | |
| #[tokio::test] | |
| async fn local_socket_addrs_excludes_unspecified() { | |
| let harness = IrohTransportTestHarness::new().await; | |
| let handler = Arc::new(MockTxHandler::default()); | |
| let transport = harness.build_transport(handler.clone()).await; | |
| // Retry for up to 5 seconds to allow address discovery. | |
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); | |
| let mut addrs; | |
| loop { | |
| addrs = transport.local_socket_addrs().await.unwrap(); | |
| if !addrs.is_empty() || std::time::Instant::now() >= deadline { | |
| break; | |
| } | |
| tokio::time::sleep(std::time::Duration::from_millis(200)).await; | |
| } | |
| // Verify no unspecified addresses (0.0.0.0 or ::) are returned. | |
| assert!( | |
| !addrs.is_empty(), | |
| "Expected at least one address to verify unspecified filtering" | |
| ); | |
| for addr in &addrs { | |
| assert!( | |
| !addr.ip().is_unspecified(), | |
| "Unspecified address should be filtered out: {addr}" | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/transport_iroh/tests/integration.rs` around lines 1254 - 1278, The
test local_socket_addrs_excludes_unspecified can pass with empty results; after
the retry loop that sets addrs via transport.local_socket_addrs(), add an
assertion that addrs is non-empty (e.g., assert!(!addrs.is_empty(), "..."))
before iterating to filter unspecified addresses so the test fails if discovery
did not produce any addresses; reference the IrohTransportTestHarness and
transport.local_socket_addrs() in the change and mirror the behavior used in
local_socket_addrs_returns_addresses if helpful.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
local_socket_addrs()to theTransporttrait, enabling consumers to extract local and reflexive socket addresses discovered by the Iroh transport. These addresses are used as WebRTC ICE candidates, replacing external STUN server dependencies.Changes
crates/api/src/transport.rs: Addedlocal_socket_addrs()toTxBaseImptrait with default empty implementation. Surfaced throughDefaultTransportandTransport.crates/transport_iroh/src/endpoint.rs: ExtractsTransportAddr::Ipvariants fromEndpoint::addr(), filters unspecified addresses (0.0.0.0,::).crates/transport_iroh/src/lib.rs: Implements trait method, delegating to endpoint.Design Decisions
std::net::SocketAddr(not Iroh types) to keepkitsune2_apitransport-agnostichosttype — only real STUN responses should producesrflxTests
16 tests pass in
kitsune2_transport_irohincluding new integration tests for address extraction.Part of: coasys/ad4m#719
Summary by CodeRabbit
New Features
Tests