-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(iroh-net)!: Create our own connection struct #2768
base: main
Are you sure you want to change the base?
Conversation
We used to re-export quinn::Connection, this makes this an owned connection so we can add our own methods to this. This adds the `.remote_peer_id()` method and removes the helper function for this.
Seen from iroh_net it makes sense. Is there anything preventing us from implementing |
This is a good idea! |
Note that there are no code changes since |
Back to this, is there any reason to not implement |
I'm not really a fan of |
Yeah, I wish rust had a better way to do or at least show this when all we need is delegation. |
Can you elaborate your future plans with this? If the only thing this allows us to do is to change the get_remote_node_id fn into a member fn, I would say it is not worth it. It would help with making examples more approachable, but you could do the same (except for an extra import) with an extension method. |
This can enable a number of things which we have shied away from in the past simply because taking the step of adding this struct was not yet done:
Probably some more things I'm not remembering right now. Admittedly this PR doesn't tackle many of these things. But it makes it so much easier to do in the future. |
Given that we have to hide all of quinn for 1.0 anyway, I think this is reasonable to do. |
@@ -221,7 +221,7 @@ async fn handle_connection( | |||
) -> anyhow::Result<()> { | |||
let alpn = conn.alpn().await?; | |||
let conn = conn.await?; | |||
let peer_id = iroh_net::endpoint::get_remote_node_id(&conn)?; | |||
let peer_id = conn.remote_node_id()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call this node_id while we're at it?
@@ -116,7 +116,7 @@ where | |||
{ | |||
let t_start = Instant::now(); | |||
let connection = connecting.await.map_err(AcceptError::connect)?; | |||
let peer = get_remote_node_id(&connection).map_err(AcceptError::connect)?; | |||
let peer = connection.remote_node_id().map_err(AcceptError::connect)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node_id?
@@ -792,7 +792,7 @@ async fn accept( | |||
match connecting.await { | |||
Ok(connection) => { | |||
if n == 0 { | |||
let Ok(remote_peer_id) = endpoint::get_remote_node_id(&connection) else { | |||
let Ok(remote_peer_id) = connection.remote_node_id() else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let Ok(remote_node_id)
/// | ||
/// May be cloned to obtain another handle to the same connection. | ||
#[derive(Debug, Clone)] | ||
pub struct Connection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have #[repr(transparent)] on this? Then we could use ref_cast to get a &quinn::Connection from a &Connection as a (possibly feature flagged) compat feature.
We might or might not do this, but the repr(transparent) certainly does not do any harm unless we plan to have additional stuff here...
PR came first checking approved ones (candidates for merging) but this has a long list of conflicts. Since this has essentially passed the approval bar, here's a friendly ping to refresh it @flub |
Yeah, I was waiting for the dust to settle on all the renames and moving crates before getting back to this. Doing backwards-incompatible changes was very difficult at some point during this churn. Converted it back to draft for now, will pick it up after the next release probably. |
Description
We used to re-export quinn::Connection, this makes this an owned
connection so we can add our own methods to this. This adds the
.remote_peer_id()
method and removes the helper function for this.Breaking Changes
iroh_net::endpoint::get_remote_node_id
has been removed. Useiroh_net::endpoint::Connection::remote_node_id
instead.Notes & open questions
We have talked about wanting to do this for so long, decided to just
give this a go.
Strictly speaking we do not need to remove
get_remote_node_id
right now.
This completely removes the ability of iroh-blobs to work with
upstream Quinn connections. It now only works with iroh. We could
probably use the
h3::quic
traits in iroh-blobs to make it genericover QUIC implementation.
The code just lives in
iroh_net::endpoint
now. That's probablyfine, but it's a large file by now. Easy enough to move it later
though.
The public API is at
iroh_net::endpoint::Connection
. Perhaps itshould also be exported at
iroh_net::Connection
just likeEndpoint
is. Though what belongs there? What about all the othertypes? Hence I'm leaving this as the status quo for this PR and
leave it where it is.
There are a bunch of new Quinn structs exported. This probably
should have happened earlier, we knew we were not exporting enough
of those but never did a proper check on what is needed. Now maybe
everything is there, bar possibly some things from quinn-udp still
missing.
Change checklist
[ ] Tests if relevant.