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

chore: discv5 move #1818

Merged
merged 1 commit into from
Jun 27, 2023
Merged

chore: discv5 move #1818

merged 1 commit into from
Jun 27, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jun 22, 2023

Description

Move discv5 out of node and into app

Changes

  • Remove redundant discv5 start and stop
  • Move all the logic into discv5 file
  • Renaming
  • Fix tests and examples

track #1812

@SionoiS SionoiS changed the title Chore discv5 move chore: discv5 move Jun 22, 2023
@SionoiS SionoiS force-pushed the chore-discv5-move branch 2 times, most recently from 80ad5fb to 024e08b Compare June 22, 2023 21:14
waku/v2/waku_discv5.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS marked this pull request as ready for review June 23, 2023 14:35
@SionoiS SionoiS force-pushed the chore-discv5-move branch 4 times, most recently from 5ca3c18 to eb6e41e Compare June 23, 2023 21:07
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Approving to not be a blocker here. Changes look good, but I think the actual discovery loop should be part of the app, where node, peer manager and discovery is integrated. See my comment below.

@@ -185,6 +158,63 @@ proc findRandomPeers*(wd: WakuDiscoveryV5, pred = none(WakuDiscv5Predicate)): Fu

return discoveredRecords

proc searchLoop*(wd: WakuDiscoveryV5, peerManager: PeerManager, record: Option[enr.Record]) {.async.} =
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm. Not sure I would have this loop be part of waku_discv5. Running a discovery loop on an instantiated discv5 instance, feeding discovered records to the peer manager and bringing all of these different components together, belong to the app IMO. I know we're trying to keep app.nim clean, but I'd say that waku_discv5 should not have knowledge of the peer manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point!

Copy link
Contributor Author

@SionoiS SionoiS Jun 26, 2023

Choose a reason for hiding this comment

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

Every test rely on discv5 having a search loop and before that it was node. app is not tested the same way.
The search loop testing would not be very uniform.

Also, even if app is the place to tie peer manager and discv5 the way it's done is not good IMO we should change it later.

I propose to add a comment & Issue for a later refactor of this part.

@SionoiS SionoiS force-pushed the chore-discv5-move branch 2 times, most recently from b65e0e4 to eb6e41e Compare June 26, 2023 20:51
- Refactor discv5 start, stop & loop.
- Fix tests.
@SionoiS SionoiS merged commit 62d3653 into master Jun 27, 2023
@SionoiS SionoiS deleted the chore-discv5-move branch June 27, 2023 13:50
@SionoiS SionoiS mentioned this pull request Jun 28, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants