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

fix(examples/file-sharing): set Kademlia Mode::Server #4197

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jul 16, 2023

Description

Explicitly set libp2p-kad Kademlia::set_mode to Mode::Server to reduce complexity of example, i.e. not having to explicitly set external addresses.

Notes & open questions

Similar to #4194.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Explicitly set `libp2p-kad` `Kademlia::set_mode` to `Mode::Server` to reduce complexity of example,
i.e. not having to explicitly set external addresses.
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jul 20, 2023

Explicitly set libp2p-kad Kademlia::set_mode to Mode::Server to reduce complexity of example, i.e. not having to explicitly set external addresses.

Is that a good thing? Would it be better if we teach users to "correctly" configure their Swarm with an external address?

@mxinden
Copy link
Member Author

mxinden commented Jul 31, 2023

In my eyes, explicitly setting Mode::Server reduces complexity. This example showcases how to use libp2p in a larger application architecture. I don't think users should worry about Kademlia details like requiring an external address. In addition, I expect users to run this locally. Setting a localhost address as an external address would be confusing to me.

Does the above change your position here @thomaseizinger?

@mxinden mxinden added the bug label Jul 31, 2023
@thomaseizinger
Copy link
Contributor

In my eyes, explicitly setting Mode::Server reduces complexity. This example showcases how to use libp2p in a larger application architecture. I don't think users should worry about Kademlia details like requiring an external address. In addition, I expect users to run this locally. Setting a localhost address as an external address would be confusing to me.

Does the above change your position here @thomaseizinger?

Sounds good to me!

@mxinden
Copy link
Member Author

mxinden commented Jul 31, 2023

Mind approving @thomaseizinger?

@mxinden
Copy link
Member Author

mxinden commented Aug 1, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2023

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit fbf35e1 into libp2p:master Aug 1, 2023
63 of 64 checks passed
hanabi1224 pushed a commit to hanabi1224/rust-libp2p that referenced this pull request Aug 3, 2023
Explicitly set `libp2p-kad` `Kademlia::set_mode` to `Mode::Server` to reduce complexity of example, i.e. not having to explicitly set external addresses.

Pull-Request: libp2p#4197.
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
Explicitly set `libp2p-kad` `Kademlia::set_mode` to `Mode::Server` to reduce complexity of example, i.e. not having to explicitly set external addresses.

Pull-Request: #4197.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants