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.

Sign up for GitHub

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

Jump to bottom

feat: reuse host #59

Closed
wants to merge 1 commit into from
Closed

feat: reuse host #59

wants to merge 1 commit into from

Conversation

2color
Copy link
Member

@2color 2color commented Aug 30, 2024

Fixes #53

@2color 2color added the status/blocked Unable to be worked further until needs are met label Aug 30, 2024
SgtPooki
SgtPooki reviewed Aug 30, 2024
View reviewed changes
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

i'm not sure I understand enough of this one to give a quality review even after reading #53

things I see changing in this PR:

  • It seems like we're no longer using the testHost to make the initial connection, instead using the libp2p daemon host.
  • It looks like the code used to only use the result of createTestHost to try connections, and we're no longer doing that. Makes sense when considering Reuse long running libp2p host for checks #53

Questions:

  • whats the difference between the result of createTestHost and d.h ? I'm guessing the d.h is the libp2p daemon host, but what exactly is the testHost?

@2color
Copy link
Member Author

2color commented Aug 30, 2024

whats the difference between the result of createTestHost and d.h ? I'm guessing the d.h is the libp2p daemon host, but what exactly is the testHost?

They're both libp2p hosts/peers.

They are configured slightly differently. For example, d.h is a DHT Client, so it connects to other peers (which helps with things like discovering your own public IPs etc.)

The test host we create is a "fresh clean" peer that is used just for the purpose of the test.

lidel
lidel reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Triage question: is this still blocked @2color ? (if so, on what?)

@2color 2color closed this Sep 23, 2024
@2color
Copy link
Member Author

2color commented Sep 23, 2024

Closing this for now. Need to rethink this (ideally in #53)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidel lidel lidel left review comments

@SgtPooki SgtPooki SgtPooki left review comments

Assignees
No one assigned
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reuse long running libp2p host for checks
3 participants
@2color @lidel @SgtPooki