Integrate the native LibP2P implementation #632
Conversation
|
Probably worth using this for #122 at this point, rather than implementing it twice. |
| template libp2pProtocol*(name: string, version: int) {.pragma.} | ||
| # TODO: This exists only as a compatibility layer between the daemon | ||
| # APIs and the native LibP2P ones. It won't be necessary once the | ||
| # daemon is removed. |
There was a problem hiding this comment.
or we can move some of it to the libp2p library - I believe there was interest in maintaining both daemon and non-daemon versions (though generally I'd prefer we drop the daemon api completely and focus our limited resources on making the native version excellent)
There was a problem hiding this comment.
Supporting both also means keeping a significantly more complex build setup and build dependency documentation, due to the daemon being in Go. I'd prefer to drop all that when feasible.
There was a problem hiding this comment.
I'm very eager to drop the daemon as soon as possible, because it's holding back the API in some sense (the daemon is not providing enough events to which you can hook up and it cannot be interrogated regarding the supported protocols of another peer).
To do this, we need to be more confident that we have working interop with all other clients though. Also, with the current state of nim-libp2p, we are back to the star topology that we managed to escape with the daemon. Discovery V5 should solve this.
There was a problem hiding this comment.
Keeping the Go daemon wrapper helps with interoperability tests, unless you want to write half of them in Go.
There was a problem hiding this comment.
Having the daemon wrapper as an optional module is perfectly fine. I was referring to the limitations imposed from trying to maintain the same facade API in front of the native implementation and the daemon.
| if not stream.closed: | ||
| await close(stream) | ||
|
|
||
| include eth/p2p/p2p_backends_helpers |
There was a problem hiding this comment.
any way we can avoid the includes? they are difficult for tooling to analyze
There was a problem hiding this comment.
as an alternative, let's at least start calling them something like .inc.nim so they're clearly marked / can be excluded
There was a problem hiding this comment.
The purpose of these includes is to simulate "generic code" without introducing actual generics. In the included file, types such as P2PStream will be different depending on the backend file that uses it. If all the code was generic instead, this would have lead to a more baroque structure and slightly worse compilation error messages, so I think the current trade-off is worthwhile.
Local network sim (and testnet1) will now use the native libp2p by default.