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

add identify/delta to identify spec #176

Closed
wants to merge 3 commits into from
Closed

Conversation

yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented Jun 20, 2019

This updates the spec to track the changes @raulk is adding in libp2p/go-libp2p#659, which adds an identify/delta variant that only sends delta updates for protocol additions / removals (and maybe later, addresses). I'll keep this PR parked until the dust settles, but I figured I'd get a jump on it now.

@raulk am I correct that identify/push now always sends full updates?

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for addressing this so quickly ❤️

identify/README.md Outdated Show resolved Hide resolved
identify/README.md Outdated Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

@jacobheun
Copy link
Contributor

Does this really need to be a separate protocol? I'm not seeing the need here aside from the concern about older peers breaking, in which case I feel like this should just become /ipfs/id/push/2.0.0. Adding delta is ultimately going to result in push no longer being used.

@raulk
Copy link
Member

raulk commented Jun 21, 2019

Does this really need to be a separate protocol?

Librarian note: conversation continued in libp2p/go-libp2p#659 (comment) and onwards.

|------------------|-----------------------|------------------------------------------------|
| `identify` | `/ipfs/id/1.0.0` | Respond to queries for identifying info. |
| `identify/push` | `/ipfs/id/push/1.0.0` | Send info proactively to inform about changes. |
| `identify/delta` | `/p2p/id/delta/1.0.0` | Send partial updates to inform about changes. |
Copy link
Member

Choose a reason for hiding this comment

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

Is this not just "identify push 1.1.0"?

(either way works for me, this just feels like identify push with an extra feature)

@Stebalien
Copy link
Member

Stebalien commented Mar 3, 2020

This protocol is strange.

  • Why are we reusing the protobuf from identify while also saying that the new "delta" field is exclusive with the rest of the fields?
  • Why doesn't this support updating addresses? Is the goal to use identify push for that and make this address only?

@raulk or @vyzo?

@Stebalien
Copy link
Member

Why doesn't this support updating addresses? Is the goal to use identify push for that and make this address only?

Nvm. This part won't matter once we get signed peer addresses anyways.

yusefnapora and others added 3 commits March 2, 2020 20:55
typo & whitespace
thanks raul :)

Co-Authored-By: Raúl Kripalani <raul.kripalani@gmail.com>
@Stebalien
Copy link
Member

@aschmahmann
Copy link
Contributor

@raulk @Stebalien what's the state of this PR?

@mxinden
Copy link
Member

mxinden commented Jan 16, 2023

Given that this effort has stalled and given that go-libp2p removed support for identify delta (libp2p/go-libp2p#1975) I am closing here.

Please comment in case you disagree with the direction.

@mxinden mxinden closed this Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants