-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Consolidate libp2p abstractions and core types #602
Comments
When this lands, can we have a small summary of breaking changes or upgrade steps? It should be fine to break things now that everything is go-modded. It's worse that people stay depending on deprecated repos just because they work and missed this consolidation. |
I think this is a great idea, and it will make it so much simpler to approach go-libp2p and explain the core functionality. I'll start going through tomorrow and see if there's anything useful from the donor READMEs that didn't carry over, and also make sure the godoc comments still make sense. I'm also going to need you to tell me what you used to make that sweet diagram... |
We have a problem the way the function aliases are implemented:
I propose to do the move of functions by preserving the old functions and changing the body to simply call the new |
So to be clear, the aliasing of function should be done like this: func OldModuleFunc(args ...) {
return core.NewModuleFunc(args ...)
} |
@vyzo +1 makes sense. Definitely more compiler optimisation-friendly. Just to clarify, we'll be moving
|
Yeah, let's do it gradually. As long as there is no performance regression, there is no impetus for immediate big bang. |
@vyzo I've now updated all donor modules to alias deprecated package funcs by proxying the call over to |
I'd also like to consider moving some of the core types to the root package. While it'll clutter the namespace, it may make it easier for users (one import). Core types include:
IMO, we might want to include that as well.
Don't include them. We're planning on switching to a third-party event logging framework anyways. For completeness, other paths considered includes:
|
Hm, not sure about this. Moving just some types creates irregularity. When finding a type, developers would have to think: "is this type core enough to be at the root?" Actually, the question is different: "did the maintainers of go-libp2p consider this type core enough at the time to put it at the root?" Unless we can all agree on a heuristic that's so natural and obvious that it's intuitive (not just to us, but to newcomers who are clueless about libp2p), I'm more inclined to adopt a clean-cut, normalised structure. That said, we can introduce type aliases at the root for better ergonomics. |
Let's try that. My main concern is that there's nothing at the root so it's kind of hard to explore the API. |
We appear to have positive consensus on the change. Merge bus shall depart soon! 🚌 Please get your approvals or final judgement calls flowing! Since this change comprises ~20 PRs, let's consolidate review approval on the go-libp2p-core PR: libp2p/go-libp2p-core#1 I've tested the changes against go-ipfs (@Stebalien) and ipfs-cluster (@hsanjuan) with no changes, but with Compilation succeeded, and so did a runtime smoke test ( It appears that backwards-compatibility is preserved. If we discover the opposite at a later time, our usage of gomod has us covered. |
How about a |
The refactor is now complete! 🎉 All affected modules have been released with minor version bumps. Check out the new core abstractions at: https://github.com/libp2p/go-libp2p-core/ It includes shiny new godocs for all types as well ;-) |
... into
go-libp2p-core
! 🎉We have positive consensus about consolidating the abstractions and core types of go-libp2p in a single repository. The rationale is clear:
Alternatives considered
There were two reasonable approaches, with their pros, cons, and notes:
Pull the abstractions and core types into
go-libp2p
under a dedicated package:skel
(skeleton),abstract
, or similar. Move out the constructor, host implementations and protocols.Place abstractions in a new module, e.g.
go-libp2p-core
.go-libp2p
, thus that module continues serving as a true entrypoint for development.Selected approach
I started with option 1, and after consultation with @Stebalien, decided to migrate to option 2. Hence, option 2 is what I'm proposing to adopt.
Here is a diagram of the proposed refactor, to ease digestion:
WARNING: This diagram is now outdated, but it was accurate when the PR was filed.
Proposed changes
All of these PRs are work in progress (despite the PRs not signalling so) and subject to change based on feedback.
This is a sizeable architectural change that will bring long-awaited improvements to our development workflow and experience, but we have to get it right. Consensus is the decision process I am invoking.
** recipients:**
go-libp2p-core: libp2p/go-libp2p-core#1
go-libp2p-testing: libp2p/go-libp2p-testing#1
donors:
libp2p/go-conn-security#12
libp2p/go-flow-metrics#6libp2p/go-libp2p-discovery#24
libp2p/go-libp2p-host#29
libp2p/go-libp2p-interface-connmgr#16
libp2p/go-libp2p-interface-pnet#12
libp2p/go-libp2p-net#44
libp2p/go-libp2p-peer#48
libp2p/go-libp2p-peerstore#69
libp2p/go-libp2p-pnet#27
libp2p/go-libp2p-protocol#8
libp2p/go-libp2p-routing#43
libp2p/go-libp2p-transport#49
libp2p/go-stream-muxer#28
libp2p/go-libp2p-crypto#60
libp2p/go-testutil#24 (test helpers and suites)
modules requiring backwards-compatible changes:
#601
libp2p/go-libp2p-blankhost#20
multiformats/go-multistream#37
Summary of change in these modules: The
Host
interface was referencing thego-multistream.MultistreamMuxer
struct directly. I extracted an interfaceprotocol.Switch
out of it, which in turn embedsprotocol.Router
(methods to attach handlers) and aprotocol.Negotiator
(methods to negotiate protocols).Host
now references the new interface, and backwards-compatible changes were required in those modules.Testing the changes for backwards compatibility
Unfortunately CI won't work because the go-libp2p-peerstore dependency is affected by this roadblock in the go toolchain: golang/go#31491
To test building these changes locally:
(TODO: run all tests)
Notes
(will keep enhancing this section with notes that come off my mind)
moved
as a package alias anddeprecated.go
as a filename. No strong opinion here, but I think it intuitively screams the signal that you should not use this.TODO
Open points:
The text was updated successfully, but these errors were encountered: