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

feat: expose locally discovered node id and addresses #2612

Closed
wants to merge 5 commits into from

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Aug 12, 2024

Description

We've had multiple requests for user to get a list of nodes that have been discovered on the local network when using local-swarm-discovery. This is my attempt.

Breaking Changes

Adds locally_discovered_nodes method to Discovery trait

Adds locally_discovered_nodes method to the Endpoint

Adds rpc structs and a locally_discovered_nodes method on the client.

Notes & open questions

This may be a tragic addition to the Discovery trait, but wanted to get feedback.

Essentially, this adds a locally_discovered_nodes method to the Discovery trait, that returns None, unless your discovery mechanism discovers nodes locally.

I'm not sure how else we would expose this, without a large Discovery refactor.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@ramfox ramfox self-assigned this Aug 12, 2024
@ramfox ramfox added this to the v0.23.0 milestone Aug 12, 2024
Copy link

github-actions bot commented Aug 12, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2612/docs/iroh/

Last updated: 2024-08-12T01:08:18Z

@rklaehn
Copy link
Contributor

rklaehn commented Aug 12, 2024

Not sure if having a separate fn in Discovery is the way to go. We already have a provenance field in DiscoveryItem that is as yet mostly unused but can be used to distinguish between different discovery mechanisms.

Why not lean on that? People can just call discovery and filter out all items that don't have the right provenance. And for the address book or whatever, we should track provenance.

Maybe provenance should be an open enum instead of just a 'static str for more type safety, but that's another question.

enum Provenance {
  Local,
  Dns,
  Pkarr,
  Ticket,
   Other(&'static str),
}

or something...

@matheus23
Copy link
Contributor

I'm not sure how else we would expose this, without a large Discovery refactor.

Thoughts on putting this information into Endpoint::connection_info? I.e. reusing Endpoint::add_node_addr in the LocalSwarmDiscovery task.
(See this comment)

@ramfox
Copy link
Contributor Author

ramfox commented Aug 21, 2024

replaced by #2656

@ramfox ramfox closed this Aug 21, 2024
@ramfox ramfox removed this from the v0.24.0 milestone Aug 26, 2024
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.

3 participants