feat(common): added the enr builder

[LNSD]

Following, and respecting, the Open-close SOLID principle:

Open-closed Principle (OCP) states:

Objects or entities should be open for extension but closed for modification.

This means that a class should be extendable without modifying the class itself.

I added an extensible EnrBuilder type that will replace the Record.init() constructor:

• Added the nim-eth ENR extension module to waku/common module.
• Added the EnrBuilder type and the "IP address and TCP/UDP ports" extension.
• Added the associated test cases. Tested against the EIP-778 test vector.

After merging this, I will add the extensions for the existing Waku ENR fields (waku2 and multiaddr).

[alrevuelta]

Nice one, some comments.

How about following nim-libp2p switch using:

let enr = EnrBuilder.init()
.withIp(ip)
.withTcpPort(tcpPort)
.withUdpPort(udpPort)
.withKey(key)
.withShards(shards)
.withCapabilities(capabilities)
.build()

So:

• use with instead
• one with one variable
• no Option fields. If a field is not used, we just avoid calling withXXX

Unrelated to this, but this pattern would be interesting for nwaku in general. withFilter(xxx).withStore(xxx)

[LNSD]

Nice one, some comments.

How about following nim-libp2p switch using:

let enr = EnrBuilder.init()
.withIp(ip)
.withTcpPort(tcpPort)
.withUdpPort(udpPort)
.withKey(key)
.withShards(shards)
.withCapabilities(capabilities)
.build()

So:

• use with instead
• one with one variable
• no Option fields. If a field is not used, we just avoid calling withXXX

This adds extra complexity to the implementation hindering the extensibility. An alternative would be to provide a "sub-builder", but it is not worth the effort at this moment.

Unrelated to this, but this pattern would be interesting for nwaku in general. withFilter(xxx).withStore(xxx)

I already thought about it. There are some corner cases that should be tought thoroughly. It is not an easy win. That's why the WakuNode instantiation is not already following this pattern.

At this moment, there is the nwaku API use cases discussion pending and there are higher priority refactorings pending (e.g. nim-eth <-> nim-libbp2p keys compatibility layer, peer types unification, etc.).

[alrevuelta]

This adds extra complexity to the implementation hindering the extensibility

Perhaps a bit more complex but I wouldn't say it hinders extensibility, its actually the whole point of it. How do you plan to add support for capabilities, shards, extra multiaddrss, etc? I presume adding new proc just like addAddressAndPorts (eg addShard)? If so that kind of what I meant.

[alrevuelta]

should we make this private? seems too low level.

I mean, we dont want people to build it like .withIpAddressAndPorts().addFieldPair()

[LNSD]

should we make this private? seems too low level.

I see no harm in it.

I need to double check Nim module visibility rules. If this proc can be accessed from an external module (e.g., waku_enr) without being publicly exposed, we can think of making it private. Otherwise, it will have to remain public.

I mean, we dont want people to build it like .withIpAddressAndPorts().addFieldPair()

Why not?

like .withIpAddressAndPorts().addFieldPair()

Note that the addFieldPair() proc does not return the builder. What you wrote is not a valid builder extension usage.

This is deliberate.

Parameters validation, in the case of being necessary, should happen within the addXXX extension method and return an EnrResult[void] explaining the validation failure cause.

[LNSD]

I need to double check Nim module visibility rules. If this proc can be accessed from an external module (e.g., waku_enr) without being publicly exposed, we can think of making it private. Otherwise, it will have to remain public.

I can confirm that hiding this method breaks the implementation. It has to be publicly accessible.

[alrevuelta]

Any reason why its on waku/common? Old code was in waku/v2.

[LNSD]

Any reason why its on waku/common?

We are placing in the common module functionality that potentially:

• Could be shared among Waku v1, v2 and the apps.
• Could be shared among different submodules within waku/v2 module.
• Could be upstreamed to the library that they are extending (e.g., this case is extending the ENR functionality of nim-eth module).

Old code was in waku/v2.

There is no such thing as old code here. This is not a code refactoring PR 👀 This is a new addition to the code base that conveniently extends nim-eth functionality by wrapping the Record.init() proc aiming to make it externally extensible.

[jm-clius]

LGTM! It might be possible in future to think of optimising patterns here, but I think this is an improvement and necessary for continuing work on static sharding.

[alrevuelta]

lgtm. thanks for the comments!