Skip to content

feat: identify #286

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

Jump to bottom
Merged
merged 11 commits into from
Feb 20, 2023
Merged

feat: identify #286

merged 11 commits into from
Feb 20, 2023

Conversation

salmad3
Copy link
Member

@salmad3 salmad3 commented Jan 9, 2023 •
edited
Loading

Context

Adds identify content to protocols section.

Latest preview

Fleek preview unavailable as PR not against master.

@salmad3 salmad3 added the ready for review PR is ready for review label Jan 9, 2023
marten-seemann
marten-seemann reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

We should also mention that there's a Identify Delta that go-libp2p shipped with for some time, and that we recently deprecated because we found that it's not particularly useful, but introduced a lot of complexity.

@salmad3 salmad3 added the P1 High label Jan 26, 2023
marten-seemann
marten-seemann reviewed Jan 31, 2023
View reviewed changes
content/concepts/introduction/protocols/identify.md Outdated
remote node and the remote node responds with an `Identify` message containing
its metadata and closes the stream.

<!-- ADD Diagram -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need one, this should be really straightforward.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had sketched simple diagrams, one for identity and identify/push.

@salmad3 salmad3 added change request PR requires changes. and removed ready for review PR is ready for review labels Jan 31, 2023
salmad3 and others added 2 commits February 1, 2023 08:34
@salmad3 @marten-seemann
Apply suggestions from code review
746de91 
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@salmad3
edits
cdc2aee
@salmad3 salmad3 added ready for review PR is ready for review and removed change request PR requires changes. labels Feb 1, 2023
mxinden
mxinden approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Pushed a couple of adjustments. This is good to go from my end.

@p-shahi p-shahi merged commit 9f5367a into refactor/intro Feb 20, 2023
@p-shahi p-shahi deleted the protocols/identify branch February 20, 2023 16:11
p-shahi pushed a commit that referenced this pull request Mar 1, 2023
@salmad3 @p-shahi
feat: identify (#286)
94572fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

@p-shahi p-shahi p-shahi approved these changes

@MarcoPolo MarcoPolo Awaiting requested review from MarcoPolo

@marten-seemann marten-seemann Awaiting requested review from marten-seemann

Assignees
No one assigned
Labels
P1 High ready for review PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@salmad3 @marten-seemann @mxinden @p-shahi