refactor(iroh)!: remove Endpoint::add_node_addr#3485
Conversation
flub
left a comment
There was a problem hiding this comment.
seems reasonable to me.
Wrt removal of remote_info, the conn_type API still exists so might be usable? We're also unlikely to release in this state? Though it could happen I guess.
It is, but it doesn't provide latency information, which is something I know some users do care about |
|
I thought this might break big-block-syc, but it seems like in the latest version of big-block-sync I only use Connection::rtt(). So I guess it's fine. I will have to refactor a lot of code to get rid of add_node_addr, e.g. in the dht experiment, but it should be fine. Not a fan of having any kind of mutable default StaticDiscovery in the endpoint. Just accept the inconvenience instead of creating chaos later when multiple places try to modify it. https://github.com/n0-computer/big-block-sync/blob/main/src/lib.rs |
|
This currently breaks the advertised way to see (passively) discovered MDNS nodes, need to figure this part out. |
|
Latency should be fine, |
|
Nous started using This is a valid use case IMO, and it only works easily this way if this info is available for any node directly from the endpoint, not from Connection only, so this might need some more thought? Or what would our alternative suggestion be? |
Yea, I think the only option is to change the example to show you need to build the mdns discovery service, subscribe to the stream, and THEN add the discovery service to the endpoint, now that we can add discovery services after the endpoint has been built. Then the example can show a loop through the stream, which will all be locally discovered nodes, rather than relying on a provenance string. |
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3485/docs/iroh/ Last updated: 2025-10-03T01:56:08Z |
|
Hmmm, to @Frando 's point, I think as long as the docs specify that the |
|
Cleaned things up so tests are passing:
API thoughts:
Also, we now have to including
|
yeah, a bit annoying, I want to try and fix it |
|
Added |
…`locally-discovered-nodes` example
…ogical "end" to the example
b32e0c8 to
c04bc64
Compare
fixed
fixed
fixed |
|
🎉 |
Description
Simplifies the public API. If you need to provide static information about node addresses, please use
StaticProviderfrom now on.Closes #3329
Breaking Changes
iroh::Endpoint::add_node_addriroh::Endpoint::remote_infoiroh::Endpoint::remote_info_iteriroh::RemoteInfoiroh::discovery::DiscoveryItemiroh::discovery::mdns::DiscoveryItemiroh::Endpoint::latencyNotes & open questions
Is it okay to have these all removed now, given the replacements for connection details have not yet landed (will only with multipath)?