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.

Sign up for GitHub

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

protocols/gossipsub: Make use of prost-codec #3070

Merged
merged 21 commits into from
Nov 4, 2022
Merged

protocols/gossipsub: Make use of prost-codec #3070

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0234417
Attempting to graft the prost_codec::Codec into the existing Gossipsu…
nloadholtes Oct 27, 2022
51753e3
WIP
nloadholtes Oct 29, 2022
329a27f
Replacing Error handling with the thiserror crate to be consistent
nloadholtes Oct 30, 2022
4384869
Putting the codec into the constructor
nloadholtes Nov 1, 2022
cd85353
Fixes for encode, keeping GossipsubCode API untouched
nloadholtes Nov 2, 2022
d73a876
Setting codec length the max_length
nloadholtes Nov 2, 2022
7eef2c8
Tests passing, cleanup: remove dead code and unused field. Replace de…
nloadholtes Nov 2, 2022
0013e59
For consistency with Enocde, return a GossipsubHandlerError
nloadholtes Nov 2, 2022
e9370fb
CHANGELOG and Cargo version updates
nloadholtes Nov 2, 2022
4253faa
version bumped
nloadholtes Nov 2, 2022
59c5ac1
Error handling change
nloadholtes Nov 2, 2022
e37f4bf
Undoing formatting, adding link to PR
nloadholtes Nov 3, 2022
9ba3551
Simplification of error handling, letting it bubble up
nloadholtes Nov 3, 2022
f656b7f
Simplification, changing back to allow decode to return Ok(None) in t…
nloadholtes Nov 3, 2022
c597afb
Map error like before
nloadholtes Nov 3, 2022
e38689a
Update CHANGELOG.md
nloadholtes Nov 4, 2022
165b364
Simplify the match
nloadholtes Nov 4, 2022
d71ed19
Update protocols/gossipsub/CHANGELOG.md
thomaseizinger Nov 4, 2022
98e3bef
Auto-generate `From` implementation
thomaseizinger Nov 4, 2022
27984d2
Replace `use` alias with FQN
thomaseizinger Nov 4, 2022
7edfe8f
Merge branch 'master' into nloadholtes/2500-gossipsub-2
jxs Nov 4, 2022
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
3 changes: 1 addition & 2 deletions protocols/gossipsub/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

use libp2p_core::identity::error::SigningError;
use libp2p_core::upgrade::ProtocolError;
use prost_codec::Error as ProstCodecError;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style question: in Python we do that fairly often to disambiguate names. In rust the fully qualified name is preferred? (Just want to make sure my thinking is on the right track)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is more a style-preference. I prefer full-qualified names because:

  • It saves an import line that is otherwise just noise.
  • Common crate names are easy to remember. For example, I know that we are using a crate called prost_codec in this repository. If I come across a reference to prost_codec::Error anywhere in the source-code, my brain just goes "Ah yeah, that must be the1 error type of that crate".
  • If a crate has an Error type, I can pretty much assume that it is the only Error type, otherwise the author would have named them differently (like EncodeError and DecodeError for example).

That is a lot of useful information to extract from a single line of code :)

On the contrary, if types are renamed on import:

  • I have to interrupt my code reading flow if I come across ProstCodecError, go the the use statements and check, what it actually is.
  • Not all programmers will rename it in the same pattern. Meaning I might come across a name that is unfamiliar to me, only to find out it is a type I actually know, just under a different name.

In the end, it is about efficiency in being able to understand code. Especially the last point can be quite annoying :)
When you are hunting a bug f.e. your brain is usually in "spot odd things"-mode and an unfamiliar name is "odd" but in the case of renaming types, it is usually a false-positive.

It is a style thing at the end of the day so YMMV :)

Footnotes

  1. Many crates by convention just have one error type. ↩

use thiserror::Error;

/// Error associated with publishing a gossipsub message.
Expand Down Expand Up @@ -101,7 +100,7 @@ pub enum GossipsubHandlerError {
#[error("Protocol negotiation failed.")]
NegotiationProtocolError(ProtocolError),
#[error("Failed to encode or decode")]
Codec(#[from] ProstCodecError),
Codec(#[from] prost_codec::Error),
}

#[derive(Debug, Clone, Copy)]
Expand Down