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

Refactor DiscoveryBehaviour events in p2p #1065

Closed
leviathanbeak opened this issue Mar 17, 2023 · 1 comment · Fixed by #1377
Closed

Refactor DiscoveryBehaviour events in p2p #1065

leviathanbeak opened this issue Mar 17, 2023 · 1 comment · Fixed by #1377
Assignees
@MitchTurner
Labels
fuel-p2p

Comments

@leviathanbeak
Copy link
Contributor

At this moment there is no need for other Behaviours / Swarm to listen to internal Discovery events so they should be removed.
Upon removing the Discovery test should be updated too.
@leviathanbeak leviathanbeak mentioned this issue Mar 17, 2023
Merged
@xgreenx xgreenx self-assigned this Jul 10, 2023
@xgreenx xgreenx mentioned this issue Aug 29, 2023
Closed
@xgreenx xgreenx removed their assignment Sep 18, 2023
@MitchTurner MitchTurner self-assigned this Sep 19, 2023
@MitchTurner
Copy link
Member

At this moment there is no need for other Behaviours / Swarm to listen to internal Discovery events so they should be removed.

We want to keep the DiscoveryBehavior, right? I'm not sure how we remove the DiscoveryEvents without removing the behavior. I don't believe any other behaviors are currently using the DiscoveryEvents...

@MitchTurner MitchTurner mentioned this issue Sep 21, 2023
Merged
xgreenx pushed a commit that referenced this issue Sep 25, 2023
@MitchTurner
Remove DiscoveryEvent and use KademliaEvent instead (#1377)
2558946 
Closes #1065

The idea being that we had the `DiscoveryEvent` that didn't serve any
special purpose and wasn't being consumed anywhere. This simplifies
`DiscoveryBehavior`.

The issue also suggested:
> Upon removing the Discovery test should be updated too.

I'm not really sure what this test is for. The actual code uses `mdns`
to identify new peer addresses, whereas the test bypasses this and does
it manually on `UnroutablePeer` events. I don't understand the point of
this.
crypto523 pushed a commit to crypto523/fuel-core that referenced this issue Oct 7, 2024
Remove DiscoveryEvent and use KademliaEvent instead (#1377)
b59f93d 
Closes FuelLabs/fuel-core#1065

The idea being that we had the `DiscoveryEvent` that didn't serve any
special purpose and wasn't being consumed anywhere. This simplifies
`DiscoveryBehavior`.

The issue also suggested:
> Upon removing the Discovery test should be updated too.

I'm not really sure what this test is for. The actual code uses `mdns`
to identify new peer addresses, whereas the test bypasses this and does
it manually on `UnroutablePeer` events. I don't understand the point of
this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MitchTurner MitchTurner

Labels
fuel-p2p
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove DiscoveryEvent and use KademliaEvent instead FuelLabs/fuel-core
3 participants
@MitchTurner @leviathanbeak @xgreenx