Skip to content
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

feat: properly encapsulate quick_protobuf::Error #3894

Merged
merged 6 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 2 additions & 15 deletions core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ mod proto {

/// Multi-address re-export.
pub use multiaddr;
use std::fmt;
use std::fmt::Formatter;
pub type Negotiated<T> = multistream_select::Negotiated<T>;

#[deprecated(since = "0.39.0", note = "Depend on `libp2p-identity` instead.")]
Expand Down Expand Up @@ -130,16 +128,5 @@ pub use transport::Transport;
pub use upgrade::{InboundUpgrade, OutboundUpgrade, UpgradeError, UpgradeInfo};

#[derive(Debug, thiserror::Error)]
pub struct DecodeError(String);

impl From<quick_protobuf::Error> for DecodeError {
fn from(e: quick_protobuf::Error) -> Self {
Self(e.to_string())
}
}

impl fmt::Display for DecodeError {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0)
}
}
#[error(transparent)]
pub struct DecodeError(quick_protobuf::Error);
Comment on lines -133 to +132
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. Isn't the constructor of DecodeError the same as a From<quick_protobuf::Error>? Asked in a different way, changing DecodeError(quic_protobuf::Error) to e.g. DecodeError(SomeNewType) a breaking change? (Where SomeNewType might as well be quic_protobuf just as a new version.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if the field is crate-private, you can't construct it from outside this crate. Trait impls are global and because From can be imported and used by anyone, anyone can call this implemented and thus depend on the underlying quick_protobuf version.

It is a very nasty situation.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for explaining!

3 changes: 1 addition & 2 deletions core/src/peer_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ impl PeerRecord {
let (payload, signing_key) =
envelope.payload_and_signing_key(String::from(DOMAIN_SEP), PAYLOAD_TYPE.as_bytes())?;
let mut reader = BytesReader::from_bytes(payload);
let record =
proto::PeerRecord::from_reader(&mut reader, payload).map_err(DecodeError::from)?;
let record = proto::PeerRecord::from_reader(&mut reader, payload).map_err(DecodeError)?;

let peer_id = PeerId::from_bytes(&record.peer_id)?;

Expand Down
3 changes: 1 addition & 2 deletions core/src/signed_envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ impl SignedEnvelope {
use quick_protobuf::MessageRead;

let mut reader = BytesReader::from_bytes(bytes);
let envelope =
proto::Envelope::from_reader(&mut reader, bytes).map_err(DecodeError::from)?;
let envelope = proto::Envelope::from_reader(&mut reader, bytes).map_err(DecodeError)?;

Ok(Self {
key: PublicKey::try_decode_protobuf(&envelope.public_key)?,
Expand Down
5 changes: 3 additions & 2 deletions transports/noise/src/io/handshake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ mod proto {

use crate::io::{framed::NoiseFramed, Output};
use crate::protocol::{KeypairIdentity, STATIC_KEY_DOMAIN};
use crate::Error;
use crate::{DecodeError, Error};
use bytes::Bytes;
use futures::prelude::*;
use libp2p_identity as identity;
Expand Down Expand Up @@ -140,7 +140,8 @@ where
{
let msg = recv(state).await?;
let mut reader = BytesReader::from_bytes(&msg[..]);
let pb = proto::NoiseHandshakePayload::from_reader(&mut reader, &msg[..])?;
let pb =
proto::NoiseHandshakePayload::from_reader(&mut reader, &msg[..]).map_err(DecodeError)?;

state.id_remote_pubkey = Some(identity::PublicKey::try_decode_protobuf(&pb.identity_key)?);

Expand Down
27 changes: 4 additions & 23 deletions transports/noise/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ use libp2p_core::{InboundUpgrade, OutboundUpgrade, UpgradeInfo};
use libp2p_identity as identity;
use libp2p_identity::PeerId;
use snow::params::NoiseParams;
use std::fmt;
use std::fmt::Formatter;
use std::pin::Pin;

/// The configuration for the noise handshake.
Expand Down Expand Up @@ -211,29 +209,12 @@ pub enum Error {
BadSignature,
#[error("Authentication failed")]
AuthenticationFailed,
#[error(transparent)]
InvalidPayload(DecodeError),
#[error("failed to decode protobuf ")]
InvalidPayload(#[from] DecodeError),
#[error(transparent)]
SigningError(#[from] libp2p_identity::SigningError),
}

#[derive(Debug, thiserror::Error)]
pub struct DecodeError(String);

impl fmt::Display for DecodeError {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0)
}
}

impl From<quick_protobuf::Error> for DecodeError {
fn from(e: quick_protobuf::Error) -> Self {
Self(e.to_string())
}
}

impl From<quick_protobuf::Error> for Error {
fn from(e: quick_protobuf::Error) -> Self {
Error::InvalidPayload(e.into())
}
}
#[error(transparent)]
pub struct DecodeError(quick_protobuf::Error);
8 changes: 4 additions & 4 deletions transports/plaintext/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub enum PlainTextError {
}

#[derive(Debug)]
pub struct DecodeError(quick_protobuf::Error);
pub struct DecodeError(pub(crate) quick_protobuf::Error);

impl fmt::Display for DecodeError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Expand Down Expand Up @@ -87,9 +87,9 @@ impl From<IoError> for PlainTextError {
}
}

impl From<quick_protobuf::Error> for PlainTextError {
fn from(err: quick_protobuf::Error) -> PlainTextError {
PlainTextError::InvalidPayload(DecodeError(err))
impl From<DecodeError> for PlainTextError {
fn from(err: DecodeError) -> PlainTextError {
PlainTextError::InvalidPayload(err)
}
}

Expand Down
4 changes: 2 additions & 2 deletions transports/plaintext/src/handshake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.

use crate::error::PlainTextError;
use crate::error::{DecodeError, PlainTextError};
use crate::proto::Exchange;
use crate::PlainText2Config;

Expand Down Expand Up @@ -74,7 +74,7 @@ impl HandshakeContext<Local> {
exchange_bytes: BytesMut,
) -> Result<HandshakeContext<Remote>, PlainTextError> {
let mut reader = BytesReader::from_bytes(&exchange_bytes);
let prop = Exchange::from_reader(&mut reader, &exchange_bytes)?;
let prop = Exchange::from_reader(&mut reader, &exchange_bytes).map_err(DecodeError)?;

let public_key = PublicKey::try_decode_protobuf(&prop.pubkey.unwrap_or_default())?;
let peer_id = PeerId::from_bytes(&prop.id.unwrap_or_default())?;
Expand Down