feat: expose local_socket_addrs on Transport trait for ICE candidate extraction#476
Open
HexaField wants to merge 1 commit intoholochain:mainfrom
Open
feat: expose local_socket_addrs on Transport trait for ICE candidate extraction#476HexaField wants to merge 1 commit intoholochain:mainfrom
HexaField wants to merge 1 commit intoholochain:mainfrom
Conversation
|
✔️ 2313b07 - Conventional commits check succeeded. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a new async API to expose local socket addresses from transports: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
530-532: Clarify what an empty list means on the public API.
Transport::local_socket_addrs()is the method consumers call, but these docs currently read as though the list only reflects discovered candidates. SinceTxImp::local_socket_addrs()falls back toOk(Vec::new()),[]can also mean "unsupported" or "not discovered yet". Please document that here too so callers don't treat an empty list as a definitive result.📝 Suggested doc tweak
/// Get the local socket addresses discovered by the transport. - /// Returns direct addresses that can be used as WebRTC ICE candidates. + /// Returns direct addresses that can be used as WebRTC ICE candidates. + /// An empty list may mean either that no addresses have been discovered yet + /// or that the underlying transport does not expose socket addresses. fn local_socket_addrs(&self) -> BoxFut<'_, K2Result<Vec<std::net::SocketAddr>>>;🤖 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 530 - 532, The docs for Transport::local_socket_addrs currently imply the returned Vec only contains discovered addresses; clarify that an empty Vec (returned via BoxFut<'_, K2Result<Vec<std::net::SocketAddr>>>) may mean either "no candidates discovered yet" or "this transport does not support local socket discovery" (TxImp::local_socket_addrs can fall back to Ok(Vec::new())). Update the method docstring to state these two possibilities and advise callers not to treat an empty list as a definitive absence of addresses (suggest retrying/awaiting discovery or checking transport capability instead).
🤖 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 1224-1237: The test currently only checks for no error but must
assert the Iroh override is used; update the integration test
local_socket_addrs_returns_addresses (or add a small unit test) to verify a
transport-specific property by either (A) polling the transport/harness
(IrohTransportTestHarness / transport) until at least one Endpoint::addr()
returns a TransportAddr::Ip and convert/assert a valid SocketAddr is present
(use a short timeout/retries to avoid flakiness), or (B) add a deterministic
unit test that constructs an Endpoint with a known TransportAddr::Ip and asserts
Endpoint::addr() yields the expected SocketAddr; reference
TxImp::local_socket_addrs, IrohTransport, Endpoint::addr, and TransportAddr::Ip
when locating code to change.
---
Nitpick comments:
In `@crates/api/src/transport.rs`:
- Around line 530-532: The docs for Transport::local_socket_addrs currently
imply the returned Vec only contains discovered addresses; clarify that an empty
Vec (returned via BoxFut<'_, K2Result<Vec<std::net::SocketAddr>>>) may mean
either "no candidates discovered yet" or "this transport does not support local
socket discovery" (TxImp::local_socket_addrs can fall back to Ok(Vec::new())).
Update the method docstring to state these two possibilities and advise callers
not to treat an empty list as a definitive absence of addresses (suggest
retrying/awaiting discovery or checking transport capability instead).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e9d69d1-5c5b-43ae-b678-986babad71c9
📒 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
+1224
to
+1237
| #[tokio::test] | ||
| async fn local_socket_addrs_returns_addresses() { | ||
| let harness = IrohTransportTestHarness::new().await; | ||
| let handler = Arc::new(MockTxHandler::default()); | ||
| let transport = harness.build_transport(handler.clone()).await; | ||
|
|
||
| // The transport should discover at least one local socket address | ||
| // (loopback at minimum in a test environment) | ||
| let addrs = transport.local_socket_addrs().await.unwrap(); | ||
| // In a real network environment there would be addresses. | ||
| // In CI/test the endpoint may not have direct addrs yet, | ||
| // but the call should succeed without error. | ||
| println!("Discovered {} local socket addresses: {:?}", addrs.len(), addrs); | ||
| } |
There was a problem hiding this comment.
Make this test prove the Iroh override is actually working.
Right now it only checks that the call doesn't error. Since TxImp::local_socket_addrs() defaults to Ok(vec![]), this still passes if IrohTransport accidentally falls back to the default implementation. Please assert on a transport-specific property here: either wait until a real endpoint exposes at least one concrete SocketAddr, or add a deterministic unit test around Endpoint::addr() with known TransportAddr::Ip values.
✅ One way to tighten the integration test
#[tokio::test]
async fn local_socket_addrs_returns_addresses() {
let harness = IrohTransportTestHarness::new().await;
let handler = Arc::new(MockTxHandler::default());
let transport = harness.build_transport(handler.clone()).await;
- // The transport should discover at least one local socket address
- // (loopback at minimum in a test environment)
- let addrs = transport.local_socket_addrs().await.unwrap();
- // In a real network environment there would be addresses.
- // In CI/test the endpoint may not have direct addrs yet,
- // but the call should succeed without error.
- println!("Discovered {} local socket addresses: {:?}", addrs.len(), addrs);
+ retry_fn_until_timeout(
+ || async { !transport.local_socket_addrs().await.unwrap().is_empty() },
+ Some(5_000),
+ Some(100),
+ )
+ .await
+ .expect("expected iroh to publish at least one local socket address");
+
+ let addrs = transport.local_socket_addrs().await.unwrap();
+ assert!(!addrs.is_empty());
+ assert!(addrs
+ .iter()
+ .all(|addr| addr.port() != 0 && !addr.ip().is_unspecified()));
}🤖 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 1224 - 1237, The
test currently only checks for no error but must assert the Iroh override is
used; update the integration test local_socket_addrs_returns_addresses (or add a
small unit test) to verify a transport-specific property by either (A) polling
the transport/harness (IrohTransportTestHarness / transport) until at least one
Endpoint::addr() returns a TransportAddr::Ip and convert/assert a valid
SocketAddr is present (use a short timeout/retries to avoid flakiness), or (B)
add a deterministic unit test that constructs an Endpoint with a known
TransportAddr::Ip and asserts Endpoint::addr() yields the expected SocketAddr;
reference TxImp::local_socket_addrs, IrohTransport, Endpoint::addr, and
TransportAddr::Ip when locating code to change.
…extraction Adds local_socket_addrs() to TxImp (with default empty vec) and Transport traits. IrohTransport implements it by querying the Iroh endpoint EndpointAddr and extracting direct IP socket addresses. Also adds addr() to the internal Endpoint trait for synchronous address access. Part of: coasys/ad4m#719
HexaField
force-pushed
the
feat/expose-iroh-endpoint
branch
from
2313b07 to
babd735
Compare
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 theTxImpandTransporttraits, enabling consumers to retrieve the local socket addresses discovered by the transport layer. This is needed for WebRTC ICE candidate extraction — replacing external STUN servers with addresses already discovered by Iroh.Changes
TxImptrait: Addedlocal_socket_addrs()with default returning empty vec (non-breaking for other transport implementations)Transporttrait: Addedlocal_socket_addrs()that delegates to the underlyingTxImpDefaultTransport: Delegates toself.imp.local_socket_addrs()IrohTransport: Implements by queryingendpoint.addr()and filteringTransportAddr::IpvariantsEndpointtrait (internal): Addedaddr()method for synchronousEndpointAddraccesslocal_socket_addrs()succeeds on a real transportContext
Every Iroh endpoint already discovers its local and reflexive (public) socket addresses through relay connections and holepunching. This PR surfaces those addresses through the kitsune2 transport API so that higher-level consumers (e.g. AD4M's WebRTC system) can use them as ICE candidates without contacting external STUN servers.
See: coasys/ad4m#719
Summary by CodeRabbit