protocols/plaintext: Move to stable futures and use unsigned varints #1306
Conversation
The plaintext 2.0 specification requires to use unsigned varints for frame length delimiting instead of fixed 4 byte integer frame length delimiting. This commit aligns the implementation with the specification.
protocols/plaintext/src/lib.rs
Outdated
| use futures::{Sink, Stream}; | ||
| use futures::prelude::*; | ||
| use futures_codec::Framed; | ||
| use libp2p_core::{identity, InboundUpgrade, OutboundUpgrade, UpgradeInfo, upgrade::Negotiated, PeerId, PublicKey}; |
There was a problem hiding this comment.
This line is much longer than 100 characters (cf. .editorconfig)
protocols/plaintext/src/lib.rs
Outdated
| fn map_err(err: io::Error) -> io::Error { | ||
| debug!("error during plaintext handshake {:?}", err); | ||
| io::Error::new(io::ErrorKind::InvalidData, err) | ||
| } | ||
|
|
||
| pub struct PlainTextMiddleware<S> { | ||
| inner: Framed<S, BytesMut>, | ||
| inner: Framed<S, UviBytes<Vec<u8>>>, |
There was a problem hiding this comment.
Why did you decide to move from BytesMut to Vec<u8>?
protocols/plaintext/src/lib.rs
Outdated
| fn poll(&mut self) -> Poll<Option<Self::Item>, Self::Error> { | ||
| self.inner.poll() | ||
| fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
| // TODO: Too much of a hack? (BytesMut -> Vec<u8>) |
There was a problem hiding this comment.
I would stick with BytesMut instead of copying the data.
protocols/plaintext/src/handshake.rs
Outdated
| pub fn handshake<S>(socket: S, config: PlainText2Config) | ||
| -> impl Future<Item = (Framed<S, BytesMut>, Remote), Error = PlainTextError> | ||
| pub async fn handshake<S>(socket: S, config: PlainText2Config) | ||
| -> Result<(Framed<S, unsigned_varint::codec::UviBytes<Vec<u8>>>, Remote), PlainTextError> |
There was a problem hiding this comment.
I would import unsigned_varint::codec::UviBytes and shorten this line. Again, I would keep using BytesMut.
protocols/plaintext/src/handshake.rs
Outdated
| // The handshake messages all start with a variable-length integer indicating the size. | ||
| let mut socket = Framed::new( | ||
| socket, | ||
| unsigned_varint::codec::UviBytes::<Vec<u8>>::default() |
There was a problem hiding this comment.
I think you can write Framed::new(socket, UviBytes::default()) here.
This wasn't too much of a conscious decision, but rather me mechanically comparing the Can you take another look? |
protocols/plaintext/src/lib.rs
Outdated
| { | ||
| type Output = (PeerId, PlainTextOutput<Negotiated<C>>); | ||
| type Error = PlainTextError; | ||
| type Future = Box<dyn Future<Item = Self::Output, Error = Self::Error> + Send>; | ||
| type Future = Pin<Box<dyn Future<Output = Result<Self::Output, Self::Error>> + Send>>; |
There was a problem hiding this comment.
| type Future = Pin<Box<dyn Future<Output = Result<Self::Output, Self::Error>> + Send>>; | |
| type Future = BoxFuture<'static, Result<Self::Output, Self::Error>>; |
(assuming an use futures::future::BoxFuture;)
There was a problem hiding this comment.
Ah, cool. Wasn't aware of this type alias. Thanks.
protocols/plaintext/src/lib.rs
Outdated
| { | ||
| type Output = (PeerId, PlainTextOutput<Negotiated<C>>); | ||
| type Error = PlainTextError; | ||
| type Future = Box<dyn Future<Item = Self::Output, Error = Self::Error> + Send>; | ||
| type Future = Pin<Box<dyn Future<Output = Result<Self::Output, Self::Error>> + Send>>; |
There was a problem hiding this comment.
like above
| type Future = Pin<Box<dyn Future<Output = Result<Self::Output, Self::Error>> + Send>>; | |
| type Future = BoxFuture<'static, Result<Self::Output, Self::Error>>; |
The plaintext 2.0 specification requires to use unsigned varints for
frame length delimiting instead of fixed 4 byte integer frame length
delimiting. This commit aligns the implementation with the
specification.