Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

change interfaces for the security-in-multiaddr change #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Sep 21, 2021

Changes to the Transport interface

Protocols() now returns a []ma.Protocol, not a slice of untyped ints. Note that this also changes the interpretation if more than one element is returned: So far, returning multiple protocols here meant that the Transport supports multiple protocols at the same time. Now it means that the Transports supports these protocols on top of each other.
Use case: We will have one TCP Transport that handles TLS and one TCP Transport that handles Noise handshakes (the handshake protocol will be taking from the multiaddr, and not negotiated using multistream). These transports will then return [ TCP, TLS ] and [ TCP, Noise ], respectively.

Changes to the SecureTransport interface

This PR adds Protocol method, returning the handshake protocol this SecureTransport uses. Examples: plaintextv2, TLS, Noise.

@github-actions
Copy link

gocompat says:

Branch 'master' set up to track remote branch 'master' from 'origin'.
"github.com/libp2p/go-libp2p-core/sec".SecureTransport InterfaceChanged
"github.com/libp2p/go-libp2p-core/transport".Transport InterfaceChanged

@@ -28,28 +29,36 @@ const ID = "/plaintext/2.0.0"
// No authentication of the remote identity is performed.
type Transport struct {
id peer.ID
key ci.PrivKey
key crypto.PrivKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we use a different import alias is to differentiate from the Go SDK crypto. I don't have strong opinions either way, just noting it.

// Protocol returns the set of protocols handled by this transport.
//
// Protocols returns the set of protocols handled by this transport.
// If protocols A and B are returned, this means that this transport supports running B on top of A.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a semantic change, or was the previous interpretation supposed to be stacked too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, I just read the PR description. I think we will find this interface limiting, and suggest something more sophisticated. [][]protocol.ID could be an option, but we might want to go with a struct for more developer friendliness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s correct, it is a semantic change (as I’ve noted in the PR description). None of our transports ever returned more than one value here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read your 2nd comment after posting my reply. We could define a ProtocolStack as []Protocol, and then return a []ProtocolStack here.

One example where returning multiple protocols might be useful is when we mint a new code point for a new QUIC version. On the other hand, one might argue that if two code points are warranted, the differences between the two transports are probably large enough to just initializing two objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be more self-descriptive, yes. Not sure if ProtocolStack is the right term -- since in the past we've talked about libp2p being a factory of stacks with different characteristics and API shapes/semantics (e.g. message orientation). So that might get overloaded in the future. Perhaps ProtocolSeq?

Copy link
Member

@raulk raulk Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the term "stack" to me denotes some kind of behaviour associated with that type, versus choosing a keyword for a type with the very limited purpose of enabling AND and OR combinations of protocols IDs.

@BigLep BigLep requested a review from a team October 24, 2021 04:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants