Make the ipfs transport extensible. #3
Conversation
|
IMO yeah agree the first method is clearer, overall I like the approach. Extending it to apply multiple upgrades I guess has typing issues that make the macro appropriate. Could the |
|
I did try that and I think it introduced more issues with trait bounds. Will give it another go now because I can't remember why I gave up on it. |
|
Ah yeah I gave up on it yesterday because I couldn't figure out how to write the type |
|
I've realised that for kepler we actually need to add upgrades pre-authentication, and there isn't a way to chain upgrades in this way in libp2p. So tomorrow I'll work on a solution to this, which I have a decent idea of. |
|
It seems like the "handshake" might require an implementation of in/outbound upgrades which satisfy the
Is it the case that the "per-connection" logic that gets actioned in an "handshake" way is the upgrade passed in to |
This would allow a user to implement additional checks on the peer id (such as checking this peer is authorized to connect) before establishing a connection.
|
Yep that's pretty much what I've done, except:
I haven't implemented it to be able to use the channel. For our purposes I think we don't need to communicate over the channel, we just need to establish whether the inbound PeerId is authorized to be part of the orbit. I may be misunderstanding how libp2p treats an error raised on the listening side however |
| @@ -61,7 +61,7 @@ pub async fn create_swarm<TIpfsTypes: IpfsTypes>( | |||
| let peer_id = options.peer_id; | |||
|
|
|||
| // Set up an encrypted TCP transport over the Mplex protocol. | |||
| let transport = transport::build_transport(options.keypair.clone())?; | |||
| let transport = transport::TransportBuilder::new(options.keypair.clone())?.build(); | |||
There was a problem hiding this comment.
can we add a transport arg (either builder or TTransport) into this fn and UninitializedIpfs::start so it can be swapped out?
There was a problem hiding this comment.
I've got a version of kepler-staging in my fork where I've consolidated these changes with the behaviour and other changes for kepler. In particular this commit does as you suggest:
cobward@9f2b170
In terms of putting it on this PR, I think we will want to have a commit that breaks open the UninitialisedIpfs, which both the behaviour change and transport change can be based on, or else raise both changes in a single PR.
There was a problem hiding this comment.
It might make sense to do both in a single PR since there is some interdependency (e.g. relay transport requires adding relay to the behaviour), and they are in a similar spirit of making the underlying p2p behaviour extensible.
There was a problem hiding this comment.
if they can be ordered or separated into PRs it would be easier for the reviewer but I see your point w.r.t. the interdependency
I've implemented two methods to do this.
The first is with the
build_transportandbuild_transport_with_upgrademethods. The latter of those creates a ipfs-compatible transport with custom transport layers and a single custom upgrade.The second method is within the
apply_upgradessubmodule. This contains a macro (apply_upgrades) which allows the user to apply any number of upgrades when building the transport.The first method is I think clearer and easier to use, however it doesn't allow more than one upgrade to be applies. The pros and cons are switched for the second method. Thoughts?