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(enr): added enr builder waku capabilities extension #1599

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Mar 9, 2023

This PR introduces the Waku ENR capabilities field extension to the EnrBuilder type.

  • Added the EnrBuilder Waku node capabilities extension.
  • Added tests covering this functionality.

@LNSD LNSD self-assigned this Mar 9, 2023
@LNSD LNSD changed the title feat(common): added the enr builder feat(enr): added enr builder waku capabilities extension Mar 9, 2023
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! left a non blocking comment.

proc withWakuCapabilities*(builder: var EnrBuilder, caps: varargs[Capabilities]) =
withWakuCapabilities(builder, CapabilitiesBitfield.init(caps))

proc withWakuCapabilities*(builder: var EnrBuilder, caps: openArray[Capabilities]) =
Copy link
Contributor

Choose a reason for hiding this comment

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

just an opinion. I generally prefer one function rather than multiple ones. less code to read, more simple and clearer. not sure I see the value of having all three.

Isn't this too low level? Cool thing about the builder is abstracting away all this
withWakuCapabilities(..., caps: CapabilitiesBitfield))

Both are very similar.
withWakuCapabilities(Relay, Store)
withWakuCapabilities(@[Relay, Store])

tldr: I would just leave withWakuCapabilities(@[Relay, Store])

Copy link
Contributor Author

@LNSD LNSD Mar 10, 2023

Choose a reason for hiding this comment

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

Different proc signatures, cover different use cases.

At first sight, you can see it as too low level, for example. But IIRC, this is the exact API we need to create an ENR in the Waku discv5 module. A similar thing happens with the multiple capabilities methods: it is not the same to pass a seq[T] than a varargs[T]. You can convert easily a varargs instance into a sequence, but the opposite has some compilation-time implications. The varargs type can be understood as an array of known sizes at compile time, on the other hand, the sequence type is a dynamic size (pointer, length, and capacity backed) list of elements.

@LNSD LNSD force-pushed the feat-enr-caps-builder-ext branch from b95ed12 to edd4c8a Compare March 10, 2023 00:39
@LNSD LNSD marked this pull request as ready for review March 10, 2023 00:51
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.

LGTM, thanks!

@LNSD LNSD merged commit 075815b into master Mar 10, 2023
@LNSD LNSD deleted the feat-enr-caps-builder-ext branch March 10, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants