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

nloadholtes · 2022-10-30T22:54:53Z

Description

This is to address issue #2500 (doing one PR per crate)

For this PR, I have referenced the work done in protocols/dcutr and modeled this work on what was done there. I tried a few other approaches, but am submitting this one because:
- The impact of the changes seems minimal compared to trying to get GossipsubCodec to "inherit" the prost_codec::Codec struct/traits. (Doing that seemed to touch larger number of lines/files, and I never got it to compile which I took as a warning I was going in the wrong direction.)
- This approach seems to work from a test/compiler-messages point of view
- I believe this code looks more like the protocol/dcutr code, and I am biased towards making a code base look consistent with itself.
I am still new to Rust, comments/suggestions/requests/opinions are welcomed 🙂
Locally the tests pass and clippy doesn't complain

Links to any relevant issues

#2500

Open Questions

Change checklist

To be completed after the above questions are answered

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works -- N/A, this is a refactor and existing tests are untouched.
A changelog entry has been made in the appropriate crates

Depends-On: #3081

protocols/gossipsub/src/protocol.rs

thomaseizinger

Looks good so far!

protocols/gossipsub/src/protocol.rs

Cargo.toml

protocols/gossipsub/CHANGELOG.md

protocols/gossipsub/src/protocol.rs

protocols/gossipsub/src/error.rs

…bCodec

…code impl with the Codec.decode() call

…he event of a None.

thomaseizinger

Thanks!

This looks really good already, just a few more minor comments :)

CHANGELOG.md

protocols/gossipsub/CHANGELOG.md

protocols/gossipsub/src/error.rs

protocols/gossipsub/src/protocol.rs

protocols/gossipsub/src/error.rs

thomaseizinger · 2022-11-04T00:48:39Z

No need for force-pushes btw, we squash-merge anyway :)

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

nloadholtes · 2022-11-04T01:41:29Z

Thanks for the heads up about the squashes! There was a merge conflict the other day so I've been super 👀 making sure I'm not drifting too far out of sync. (Of course since that 1 time everything has been smooth sailing 😂 )

thomaseizinger · 2022-11-04T01:48:18Z

Thanks for the heads up about the squashes! There was a merge conflict the other day so I've been super eyes making sure I'm not drifting too far out of sync. (Of course since that 1 time everything has been smooth sailing joy )

You can solve the merge conflicts by just merging latest master in :)

thomaseizinger

I've pushed some final patches, this LGTM!

Thanks for contributing :)

thomaseizinger · 2022-11-04T01:56:43Z

I'll merge once CI is green!

nloadholtes · 2022-11-04T01:58:37Z

protocols/gossipsub/src/error.rs

@@ -22,7 +22,6 @@

 use libp2p_core::identity::error::SigningError;
 use libp2p_core::upgrade::ProtocolError;
-use prost_codec::Error as ProstCodecError;


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)

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 the¹ 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

Many crates by convention just have one error type. ↩

nloadholtes · 2022-11-04T02:01:00Z

Sure thing! Thank you for all your help with this. I'm going to try and tackle another protocol (and hopefully it will be smoother on the PR 😅)

thomaseizinger · 2022-11-04T03:12:48Z

Need to merge #3081 before we can proceed here.

thomaseizinger · 2022-11-04T03:14:14Z

Need to merge #3081 before we can proceed here.

I am using this opportunity to test mergify's "PR dependency" feature: https://docs.mergify.com/actions/queue/#merge-depends-on

thomaseizinger · 2022-11-04T03:15:11Z

Need to merge #3081 before we can proceed here.

I am using this opportunity to test mergify's "PR dependency" feature: docs.mergify.com/actions/queue/#merge-depends-on

Cool! It works like a charm:

@jsx @mxinden @elenaf9 Look at this! :)

nloadholtes commented Oct 30, 2022

View reviewed changes

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

nloadholtes mentioned this pull request Oct 30, 2022

Migrate all protocols to use quick-protobuf-codec #2500

Closed

thomaseizinger reviewed Oct 31, 2022

View reviewed changes

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

nloadholtes commented Nov 1, 2022

View reviewed changes

protocols/gossipsub/src/protocol.rs Show resolved Hide resolved

thomaseizinger reviewed Nov 2, 2022

View reviewed changes

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

nloadholtes force-pushed the nloadholtes/2500-gossipsub-2 branch from a7575aa to 9e2b471 Compare November 2, 2022 14:07

nloadholtes marked this pull request as ready for review November 2, 2022 14:07

nloadholtes force-pushed the nloadholtes/2500-gossipsub-2 branch from 9e2b471 to ab1901b Compare November 2, 2022 14:08

nloadholtes requested a review from thomaseizinger November 2, 2022 14:09

nloadholtes force-pushed the nloadholtes/2500-gossipsub-2 branch from ab1901b to 602e2ab Compare November 2, 2022 15:33

thomaseizinger requested changes Nov 2, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

nloadholtes commented Nov 3, 2022

View reviewed changes

protocols/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Nov 3, 2022

View reviewed changes

protocols/gossipsub/src/error.rs Outdated Show resolved Hide resolved

nloadholtes and others added 15 commits November 3, 2022 16:54

Attempting to graft the prost_codec::Codec into the existing Gossipsu…

0234417

…bCodec

WIP

51753e3

Replacing Error handling with the thiserror crate to be consistent

329a27f

Putting the codec into the constructor

4384869

Fixes for encode, keeping GossipsubCode API untouched

cd85353

Setting codec length the max_length

d73a876

Tests passing, cleanup: remove dead code and unused field. Replace de…

7eef2c8

…code impl with the Codec.decode() call

For consistency with Enocde, return a GossipsubHandlerError

0013e59

CHANGELOG and Cargo version updates

e9370fb

version bumped

4253faa

Error handling change

59c5ac1

Undoing formatting, adding link to PR

e37f4bf

Simplification of error handling, letting it bubble up

9ba3551

Simplification, changing back to allow decode to return Ok(None) in t…

f656b7f

…he event of a None.

Map error like before

c597afb

nloadholtes force-pushed the nloadholtes/2500-gossipsub-2 branch from edae8f3 to c597afb Compare November 3, 2022 20:54

nloadholtes requested a review from thomaseizinger November 3, 2022 20:54

thomaseizinger reviewed Nov 4, 2022

View reviewed changes

nloadholtes and others added 2 commits November 3, 2022 21:28

Update CHANGELOG.md

e38689a

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Simplify the match

165b364

nloadholtes requested a review from thomaseizinger November 4, 2022 01:45

thomaseizinger changed the title ~~Update protocols/gossipsub to use Protobuf Codec pattern~~ protocols/gossipsub: Make use of prost-codec Nov 4, 2022

thomaseizinger added 3 commits November 4, 2022 12:49

Update protocols/gossipsub/CHANGELOG.md

d71ed19

Auto-generate From implementation

98e3bef

Replace use alias with FQN

27984d2

thomaseizinger approved these changes Nov 4, 2022

View reviewed changes

nloadholtes commented Nov 4, 2022

View reviewed changes

thomaseizinger added the send-it label Nov 4, 2022

Merge branch 'master' into nloadholtes/2500-gossipsub-2

7edfe8f

mergify bot merged commit 1ba9e45 into libp2p:master Nov 4, 2022

nloadholtes deleted the nloadholtes/2500-gossipsub-2 branch November 4, 2022 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nloadholtes commented Oct 30, 2022 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger commented Nov 4, 2022

nloadholtes commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger left a comment

thomaseizinger commented Nov 4, 2022

nloadholtes Nov 4, 2022

thomaseizinger Nov 4, 2022

nloadholtes commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

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

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

Conversation

nloadholtes commented Oct 30, 2022 • edited by thomaseizinger Loading

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 4, 2022

nloadholtes commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 4, 2022

nloadholtes Nov 4, 2022

Choose a reason for hiding this comment

thomaseizinger Nov 4, 2022

Choose a reason for hiding this comment

Footnotes

nloadholtes commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

thomaseizinger commented Nov 4, 2022

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

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

nloadholtes commented Oct 30, 2022 •

edited by thomaseizinger

Loading