Skip to content
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

refactor: Use iroh_net::PeerAddr more #1493

Merged
merged 20 commits into from
Sep 21, 2023
Merged

Conversation

Frando
Copy link
Member

@Frando Frando commented Sep 19, 2023

Description

Replaces custom types with iroh_net::PeerAddr

Notes & open questions

na

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@Frando Frando changed the base branch from main to api-refactor September 19, 2023 07:14
Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did more or less just this in #1488 based on #1486 with a different take, since I need to serialize the (derp_region, endpoints) by itself (just like gossip does), thus using it there as well.

Can you take a look? I think in principle our object is the same and I would then add the convenience methods you created like with_derp_region to extend the api in the same fashion

@b5 b5 added this to the v0.6.0 milestone Sep 20, 2023
@Frando
Copy link
Member Author

Frando commented Sep 20, 2023

Alright, yes this is pretty similar.

We can bikeshed if we want to keep the NodeAddr type to combine the node id and the addr info and use that for connect as in this PR, or change connect to take node id, addr info, and alpn.

So it would be one of these two:

MagicEndpoint::connect(&self, node_id: PublicKey, addr_info: AddrInfo, alpn: &[u8]);
MagicEndpoint::connect(&self, addr: NodeAddr, alpn: &[u8])

I still kinda like the latter, because in most situations you will get the full NodeAddr from somewhere and can just pass it through. But either works fine. What do others think?

But let's maybe get #1488 in first and then I can update this PR afterwards.

github-merge-queue bot pushed a commit that referenced this pull request Sep 21, 2023
## Description

- Adds a `AddrInfo` that contains the addressing information of peers.
This is necessary as it's own type since it's serialized both in gossip
and in #1488
- Renames `NodeAddr` to `PeerData` (better name suggestions are well
received) this helps consolidate the "peer" terminology already present
throughout gossip, sync, downloader, and many other places
- Use this type in different parts of the code. Some other types that
can be replaced by this are left to #1493

## Notes & open questions

This applies the suggestion of doing `connect` using the full type. It's
debatable if this is better than `connect(PublicKey, AddrInfo)` but both
are imo an improvement over the current way since the idea if "what does
iroh need to connect to a peer" is now fully described in a type

## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
- [ ] Tests if relevant.
@divagant-martian divagant-martian changed the base branch from api-refactor to main September 21, 2023 21:55
@divagant-martian divagant-martian force-pushed the api-refactor-node-addr branch 3 times, most recently from c2bd15a to add7df9 Compare September 21, 2023 22:16
@divagant-martian divagant-martian changed the title refactor: Use NodeAddr more refactor: Use iroh_net::PeerAddr more Sep 21, 2023
@divagant-martian divagant-martian added this pull request to the merge queue Sep 21, 2023
Merged via the queue into main with commit 2b4b27c Sep 21, 2023
15 checks passed
@dignifiedquire dignifiedquire deleted the api-refactor-node-addr branch November 1, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants