feat(iroh-net)!: Allow using a NodeId directly in connect. #2774
Conversation
This file contains 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
Also deprecate connect_by_node_id since it is no longer needed.
rklaehn
changed the title
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2774/docs/iroh/ Last updated: 2024-10-02T12:18:06Z |
flub
reviewed
Oct 2, 2024
rklaehn
force-pushed
the
remove-connect-by-node-id
branch
from
d6405dd
to
16e56da
Compare
flub
reviewed
Oct 2, 2024
| /// A value that can be converted into a [`NodeAddr`] is required. This can be either a | ||
| /// [`NodeAddr`], a [`NodeId`] or a [`iroh_base::ticket::NodeTicket`]. | ||
| /// | ||
| /// The [`NodeAddr`] must contain the [`NodeId`] to dial and may also contain a [`RelayUrl`] |
There was a problem hiding this comment.
We don't use dial in any of the API names of iroh-net::Endpoint so I prefer not to use that in documentation either as that is probably confusing to users. I usually stick to "connect", "establish a connection" or something like that.
(yeah, there's the somewhat weird dialer module which is a bit of an odd duck...)
| alpn: &[u8], | ||
| ) -> Result<quinn::Connection> { | ||
| let node_addr = node_addr.into(); | ||
| tracing::Span::current().record("remote", node_addr.node_id.fmt_short()); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Allow using anything can be converted to a NodeAddr when connecting. This means that we can directly connect with NodeIds, and connect_by_node_id is no longer needed.
Also deprecate connect_by_node_id since it is no longer needed.
Breaking Changes
impl Into<NodeAddr>instead of aNodeAddrNotes & open questions
Note: Doing this means that the instrumentation of connect no longer contains the remote, which is bad I guess. Not sure how to work around this. One way would be to have connect and connect_impl - connect_impl is like the current connect and can be instrumented as before. Any other way to do this?
Note: since there is a From for NodeAddr, you can also directly dial using a ticket, which is kinda neat.
Change checklist