refactor(*): rework NodeAddr
#1506
Merged
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
3 tasks
Frando
reviewed
Sep 21, 2023
iroh-net/src/magic_endpoint.rs
Outdated
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| pub struct NodeAddr { | ||
| pub struct PeerData { |
There was a problem hiding this comment.
About the name - What about PeerAddr? To me having addr in there suggests "this is what you need to connect", which is what it is. So if we decided for Peer, not Node, then I'd say let's do PeerAddr.
And then maybe have info: AddrInfo, without the addr_ prefix.
iroh-net/src/magic_endpoint.rs
Outdated
| ) -> Result<quinn::Connection> { | ||
| self.add_known_addrs(node_id, derp_region, known_addrs) | ||
| .await?; | ||
| pub async fn connect(&self, peer_data: PeerData, alpn: &[u8]) -> Result<quinn::Connection> { |
There was a problem hiding this comment.
I think here peer_addr: PeerAddr reads much nicer than PeerData
Frando
approved these changes
Sep 21, 2023
iroh-net/src/magic_endpoint.rs
Outdated
| endpoints: endpoints.to_vec(), | ||
| }; | ||
| self.msock.add_known_addr(addr).await?; | ||
| pub async fn add_peer_data(&self, peer_data: PeerData) -> Result<()> { |
There was a problem hiding this comment.
This could be add_peer_addr with the other suggested rename, which sounds fine to me too.
3 tasks
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 22, 2023
…able tests (#1513) ## Description * **fix(gossip): properly encode peer data.** #1506 introduced a bug: The `PeerData` was encoded from the `PeerAddr` (including the peer id) but decoded to `AddrInfo` (without the peer id). So it failed, and dialing peers failed. It only did not matter much because most tests use tickets separately, so do not rely on the `PeerData` gossip. * **feat: expose neighbor events** through document subscriptions. for now used to write better and less flakey tests. also useful for stats like usecases, and potentially others. * **tests: improve sync tests** and make `sync_full_basic` not flakey anymore (hopefully). the main change is that for some events, we don't care about the exact order in tests anymore, because the exact order is too unpredictable timing-wise for things that happen concurrently. instead they are matched in chunks. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. Replaces #1512
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
AddrInfothat contains the addressing information of peers. This is necessary as it's own type since it's serialized both in gossip and in feat(iroh-net): persist known peer info #1488NodeAddrtoPeerData(better name suggestions are well received) this helps consolidate the "peer" terminology already present throughout gossip, sync, downloader, and many other placesiroh_net::PeerAddrmore #1493Notes & open questions
This applies the suggestion of doing
connectusing the full type. It's debatable if this is better thanconnect(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 typeChange checklist