Adding example for Identify protocol #2689

dadepo · 2022-06-02T19:13:35Z

Description

Added example for the Identify protocol.

Links to any relevant issues

Part of #2684

Change checklist

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
A changelog entry has been made in the appropriate crates

mxinden

Appreciate the help here. Though still a couple of comments.

mxinden · 2022-06-03T15:28:09Z

examples/identify.rs

@@ -0,0 +1,82 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.


Given that this example is focused on libp2p-identify, how about moving it to protocols/identify/examples/? To make sure one can still discover it, it can be mentioned in https://github.com/libp2p/rust-libp2p/blob/master/examples/README.md.

I was not even aware that there are some examples placed together with some of the individual protocols. Is there a reason why not to have all the examples listed in https://github.com/libp2p/rust-libp2p/tree/master/examples/*?

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

No strong opinion on my end. I see /examples as a folder for examples that combine many protocols and protocols/xxx/examples as a folder for examples targeting a single protocol.

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

I see your point.

Maybe @elenaf9 or @thomaseizinger have an opinion here.

I am leaning towards having examples focused on a single protocol in that protocol's folder. Since they are individual crates, imo they should "own" their own examples.

I was not even aware that there are some examples placed together with some of the individual protocols.

We should definitely mention this in the top-level examples/README.md, which is currently not the case.

I am leaning towards having examples focused on a single protocol in that protocol's folder. Since they are individual crates, imo they should "own" their own examples.

I share the same view. These protocols are individual crates and I think we should follow cargo conventions despite this being a large workspace.

I was not even aware that there are some examples placed together with some of the individual protocols.

We should definitely mention this in the top-level examples/README.md, which is currently not the case.

Another solution to this would be to do away with the current meta crate being "special" by being a workspace manifest and a crate at the same time. i.e. we could create a directory meta which could house the libp2p crate. This would also move the top-level examples/ directory which might make it more obvious to users that they will find examples within each crate.

mxinden · 2022-06-03T15:29:13Z

examples/identify.rs

+//! cargo run --example identify
+//! ```
+//! It will print the PeerId and the listening addresses, e.g. `Listening on
+//! "/ip4/0.0.0.0/tcp/24915"`


Suggested change

//! "/ip4/0.0.0.0/tcp/24915"`

//! "/ip6/2001:db8:: /tcp/24915"`

Let's not use a wildcard address here.

Given that we called swarm.listen_on with a ipv4 wildcard address, this can never result in a ipv6 listen address, right? Thus think "/ip4/127.0.0.1/tcp/24915" would make more sense here.

examples/identify.rs

Co-authored-by: Max Inden <[email protected]>

dadepo · 2022-06-03T19:13:48Z

Appreciate the help here. Though still a couple of comments.

Gladly done. I will resolve/respond the comments in a bit.

Quick question, seeing you already approved, do you prefer I create individual PR for each example added?
My initial thought was to include all the added examples in a single PR, hence why I set this PR to draft for now. Plan was to update it to ready PR once all the examples have been added

Let me know which approach the team finds more convenient.

mxinden · 2022-06-08T10:46:58Z

Quick question, seeing you already approved

That was a mistake. Sorry.

do you prefer I create individual PR for each example added?

Yes. I prefer small atomic pull requests. Makes my life as a maintainer a lot easier. Thanks.

mxinden · 2022-06-08T10:48:46Z

examples/identify.rs

@@ -0,0 +1,82 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.


No strong opinion on my end. I see /examples as a folder for examples that combine many protocols and protocols/xxx/examples as a folder for examples targeting a single protocol.

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

I see your point.

Maybe @elenaf9 or @thomaseizinger have an opinion here.

examples/identify.rs

Co-authored-by: Max Inden <[email protected]>

mxinden

🚀 Thank you!

CI failing

mxinden · 2022-06-24T03:30:10Z

CI failures are due to libp2p/if-watch#19.

mxinden · 2022-06-24T04:02:08Z

CI failures are due to mxinden/if-watch#19.

This is fixed now.

To make CI green you would need to:

Run rustfmt

Add libp2p as an explicit dev-dependency to protocols/identify/Cargo.toml similar to

rust-libp2p/protocols/dcutr/Cargo.toml

Line 33 in eb490c0

    
           libp2p = { path = "../..", default-features = false,  features = ["dcutr", "relay", "plaintext", "identify", "tcp-async-io", "ping", "noise", "dns-async-std"] }

elenaf9

Thanks @dadepo! Two comments, otherwise looks good to me.

elenaf9 · 2022-06-25T14:54:25Z

examples/identify.rs

+//! cargo run --example identify
+//! ```
+//! It will print the PeerId and the listening addresses, e.g. `Listening on
+//! "/ip4/0.0.0.0/tcp/24915"`


Given that we called swarm.listen_on with a ipv4 wildcard address, this can never result in a ipv6 listen address, right? Thus think "/ip4/127.0.0.1/tcp/24915" would make more sense here.

elenaf9 · 2022-06-25T14:55:52Z

protocols/identify/examples/identify.rs

+//! The dialing node prints out the `PeerId` of the node it is sending identify info to
+//! The other node prints out the received identify info.


I think not only the dialer, but instead both peers will sent each other identify info once they establish a connection, no?

Added identify example

9d41f5c

mxinden approved these changes Jun 3, 2022

View reviewed changes

dadepo changed the title ~~Added identify example~~ Adding more examples Jun 3, 2022

dadepo and others added 6 commits June 3, 2022 21:09

Update examples/identify.rs

04ca78f

Co-authored-by: Max Inden <[email protected]>

Update examples/identify.rs

1967874

Co-authored-by: Max Inden <[email protected]>

Update examples/identify.rs

0cfa33c

Co-authored-by: Max Inden <[email protected]>

Update examples/identify.rs

83c49a2

Co-authored-by: Max Inden <[email protected]>

Update examples/identify.rs

173e823

Co-authored-by: Max Inden <[email protected]>

Update examples/identify.rs

53360ca

Co-authored-by: Max Inden <[email protected]>

dadepo added 2 commits June 3, 2022 22:25

Start the comment with upper case

c4ae6e4

Merge branch 'master' into extend-examples

3d40130

mxinden reviewed Jun 8, 2022

View reviewed changes

dadepo and others added 2 commits June 23, 2022 18:06

Update examples/identify.rs

f60ad46

Co-authored-by: Max Inden <[email protected]>

Moved identify example to the identify crate

92188ad

dadepo changed the title ~~Adding more examples~~ Adding example for Identify protocol Jun 23, 2022

Merge branch 'master' into extend-examples

bbd783e

dadepo marked this pull request as ready for review June 23, 2022 16:23

dadepo requested review from thomaseizinger, elenaf9 and mxinden June 23, 2022 16:24

mxinden previously approved these changes Jun 24, 2022

View reviewed changes

dadepo added 2 commits June 25, 2022 00:27

Ran rustfmt and added libp2p as an explicit dev-dependency

bd1e117

Merge branch 'master' into extend-examples

dbe73b7

elenaf9 reviewed Jun 25, 2022

View reviewed changes

Updated comments to be more accurate

f2668f3

elenaf9 approved these changes Jun 26, 2022

View reviewed changes

Merge branch 'master' into extend-examples

0d3dcd0

mxinden merged commit 40744be into libp2p:master Jun 27, 2022

dadepo deleted the extend-examples branch June 28, 2022 06:39

elenaf9 mentioned this pull request Jun 28, 2022

protocols/identify: fix dev dependencies required for examples/identify #2737

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding example for Identify protocol #2689

Adding example for Identify protocol #2689

dadepo commented Jun 2, 2022 •

edited

Loading

mxinden left a comment •

edited

Loading

mxinden Jun 3, 2022

dadepo Jun 3, 2022

mxinden Jun 8, 2022

elenaf9 Jun 8, 2022 •

edited

Loading

thomaseizinger Jun 15, 2022

mxinden Jun 3, 2022

elenaf9 Jun 25, 2022

dadepo commented Jun 3, 2022

mxinden commented Jun 8, 2022

mxinden Jun 8, 2022

mxinden left a comment

mxinden commented Jun 24, 2022

mxinden commented Jun 24, 2022

elenaf9 left a comment

elenaf9 Jun 25, 2022

elenaf9 Jun 25, 2022

		@@ -0,0 +1,82 @@
		// Copyright 2018 Parity Technologies (UK) Ltd.

	//! "/ip4/0.0.0.0/tcp/24915"`
	//! "/ip6/2001:db8:: /tcp/24915"`

		//! The dialing node prints out the `PeerId` of the node it is sending identify info to
		//! The other node prints out the received identify info.

Adding example for Identify protocol #2689

Adding example for Identify protocol #2689

Conversation

dadepo commented Jun 2, 2022 • edited Loading

Description

Links to any relevant issues

Change checklist

mxinden left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadepo commented Jun 3, 2022

mxinden commented Jun 8, 2022

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Jun 24, 2022

mxinden commented Jun 24, 2022

elenaf9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadepo commented Jun 2, 2022 •

edited

Loading

mxinden left a comment •

edited

Loading

elenaf9 Jun 8, 2022 •

edited

Loading