Add ENR Record.init and update call for dual stack #748
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kdeme
commented
Oct 11, 2024
Comment on lines
+262
to
+263
| ip4: array[4, byte], | ||
| ip6: array[16, byte], |
There was a problem hiding this comment.
There is no Opt here, as in dual stack case I'm assuming both IPs or no IP is known (perhaps this is not correct?).
Also not using IpAddress as from that type I cannot be sure whether it is IPv4 or IPv6 (unless I check for it of course).
In the other use cases, the init call with only 1 address can be used.
Comment on lines
+291
to
+293
| # From the ENR specification: | ||
| # "Declaring the same port number in both tcp, tcp6 or udp, udp6 should be avoided | ||
| # but doesn't render the record invalid." |
There was a problem hiding this comment.
Perhaps I don't need to do this check and can let the user decide what they want to do in case of same ports.
Comment on lines
+428
to
+429
| ## Providing an `Opt.none` for `tcp4Port`/`udp4Port`/`tcp6Port`/`udp6Port`/ | ||
| ## will leave the corresponding field untouched. |
There was a problem hiding this comment.
Perhaps it makes more sense to unset the field in that case
| udp6Port: Opt[Port] = Opt.none(Port), | ||
| extraFields: openArray[FieldPair] = []): | ||
| EnrResult[void] = | ||
| ## Update a `Record` with given IPv4 and IPv6 address, optional TCP port and |
There was a problem hiding this comment.
The fact that I have to write such a lengthy documentation for this call probably is not a sign of good API.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempt to add dual stack support in ENR in line with the current API.
In draft however as I don't really like this API, it feels rather clumsy, both in implementation and in usage.
I'll probably implement dualstack in Fluffy to see how it would typically be used, and continue/adjust from there.
Might also go for some
EnrBuilderapproach instead to get to something in the trend of:let enr = EnrBuilder.init().tcp4(Port(9000)).ip4(ip4).ip6(ip6).build(privatekey)and
let enrUpdated = EnrBuilder.init(enr).tcp4(Port(9000)).ip4(ip4).build(privatekey)Ideas and/or use cases are welcome.