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: waku rendezvous wrapper #2962

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: waku rendezvous wrapper #2962

wants to merge 10 commits into from

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Aug 8, 2024

Description

I've wrapped libp2p rendezvous protocol so that at the waku layer we can register at rdv points and also request peer records.

spec -> https://github.com/waku-org/specs/blob/master/standards/core/rendezvous.md

Changes

  • rendezvous wrapper
  • server periodic registration
  • config

Copy link

github-actions bot commented Aug 13, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2962

Built from 8228856

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 29, 2024

blocked by vacp2p/nim-libp2p#1183 (new libp2p version)

Copy link
Contributor

@darshankabariya darshankabariya left a comment

Choose a reason for hiding this comment

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

Thanks ! LGTM

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Looks amazing, thanks so much! 🤩

We should probably add tests though, to make sure that the procs defined in protocol.nim work as expected

./common

logScope:
topics = "waku rendez vous"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
topics = "waku rendez vous"
topics = "waku rendezvous"

I would personally write "rendezvous" everywhere instead of "rendez vous" for consistency (it will also make everything easier when performing searches)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry for that in French it's 2 words not one...

continue

# Ask for 12 peer records for that shard
self.request(namespace, 12, @[rpi.peerId])
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't be better to have a constant instead of hardcoding 12?

# start registering forever
self.periodicRegistrationFut = self.periodicRegistration()

proc stopWait*(self: WakuRendezVous) {.async: (raises: []).} =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're not calling stopWait() anywhere. Shouldn't we add it to a stop() proc so it gets executed when calling node.wakuRendezvous.stop()?

@SionoiS
Copy link
Contributor Author

SionoiS commented Dec 2, 2024

We should probably add tests though, to make sure that the procs defined in protocol.nim work as expected

I will add some tests but nim-libp2p already tests the functionalities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Code Review / QA
Development

Successfully merging this pull request may close these issues.

3 participants