-
Notifications
You must be signed in to change notification settings - Fork 55
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
New builders PoC #646
New builders PoC #646
Conversation
One of the reasons of having the builders in the first place was to separate object creation and initialization to avoid tight coupling, by moving it back into the transport module/class, we're back to square one. I like the overall idea of having dedicated factory methods handling object creation and I like the potential syntax of |
Ok, so I did a V2. To recap the goals: Avoid centralization of buildingCentralization, as it's done now, as a few issues:
Impossible to get a half-built objectYou should only get the usable switch once it's fully setup Avoid tight couplingModules shouldn't rely on the switch, and the switch should rely on as-little modules as possible. With that in mind, the new system: How it's usedCreating a switch: proc createSwitch(ma: MultiAddress, rng: ref BrHmacDrbgContext): Switch =
var switchBuilder = SwitchBuilder.new()
switchBuilder.with(PeerInfo, addrs=[ma])
switchBuilder.with(MultistreamSelect)
switchBuilder.with(Identify.new())
switchBuilder.with(Noise, rng=rng)
switchBuilder.with(Mplex)
switchBuilder.with(ConnManager)
let tcpTransport = switchBuilder.with(TcpTransport)
switchBuilder.with(Dialer)
return switchBuilder.build() We mount each module one by one. We have multiple possibilities to mount a module: we can give only it's type, or it's type + some parameters that will be forwarded, or give an existing instance. Creating a module: proc new*(
p: typedesc[PeerInfo],
rng = newRng(),
addrs: openarray[MultiAddress] = []): PeerInfo
let randomPrivateKey = PrivateKey.random(rng[])
PeerInfo.new(randomPrivateKey.get(), addrs)
proc setup*[Ctx](c: Ctx,
peerInfo: PeerInfo) =
c.peerInfo = peerInfo The A few other setups, to see what they look like: proc setup*[Ctx](c: Ctx, i: Identify) {.raises: [Defect, LPError].} =
i.peerInfo = c.peerInfo
c.identify = i
c.mount(i)
########
proc setup*[Ctx](c: Ctx, tcpTransport: TcpTransport) =
# The upgrade is now created per-transport. I think this make sense
# because Quic will use a different upgrader, for instance (no need for mplex)
let upgrade = MuxedUpgrade.new(c.identify, c.muxers, c.secureManagers, c.connManager, c.ms)
tcpTransport.upgrader = upgrade
c.transports &= tcpTransport With this new system, each module is completely independent, and can do his setup as he likes. The SwitchBuilderThe SwitchBuilder adds security to this switch building phase. First of all, to call var switchBuilder = SwitchBuilder.new()
switchBuilder.with(Identify)
## Fails with: With(Identify): PeerInfo should be setup first [Defect]
var switchBuilder = SwitchBuilder.new()
[... setup everything ...]
switchBuilder.with(TcpTransport)
switchBuilder.with(Dialer)
switchBuilder.with(WsTransport)
## Fails with: With(WsTransport): can't be done after With(Dialer) [Defect] The SwitchBuilder remains completely generic, and will only base theses errors on the read/write of the different modules. The main drawback of this entire system is that users have to be careful about the order they Right now I only did the setup for the minimum amount of modules, before being sure that we want to go with this. |
Closing in favor of #701 which is miles better |
#551 introduced Builders, which allows to build the switch like this:
My main gripe with it is that the
builders.nim
file must import & handle the setup logic of every module of libp2p. Beside being fairly centralized, this also forces everyone to compile every module, even if they don't use them.To avoid this issue with WS, I had to create a generic builder:
But now, websocket is a second class citizen, because it's harder to create than Tcp.
Other example, to implement PushIdentify, which requires some wiring up in the switch, I need to add it again in the
builders.nim
This PR is an attempt to create a new builder system which alleviates some of the issues. Example build with it:
Each
mount
is now handled in each module directly, ex intcptransport.nim
:(The generic is a ugly hack to avoid circular imports in some cases)
The biggest downside of this technique is that you must mount everything manually, and in the right order (eg, mount transports last because they need the upgrade). But imo, it's easy & clear enough to add errors when you miss some mandatory protocols
Creating this PR to get some opinions. There is a lot of cleanup potential, this
mount
(which we can rename) could replace most constructors, theLPProtocol
init
method, etcObviously, big breaking change, which is why I would like to squeeze it before V1 if you like it