search

libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.56k stars 273 forks source link

[Rendezvous] Clarify `UNREGISTER` behaviour and remove `id` field #348

Closed thomaseizinger closed 3 years ago

thomaseizinger commented 3 years ago

All connections in libp2p are authenticated. As such, we don't have to include the PeerId in the Unregister message. We only allow peers to unregister themselves and therefore, this id would always be equal to the one we are learn from the authentication layer. Having to compare those and ensure they are equal is an unnecessary error path.

This also clarifies the absence of an UNREGISTER response.

Resolves #335.

thomaseizinger commented 3 years ago

If we are worried about breaking changes, we could also simply deprecate this field instead or at least make sure the next field that gets added is id 3.

thomaseizinger commented 3 years ago

Yes, please let's make sure the next field starts at 3 e.g. by commenting out the id field and adding a comment.

Done. Also clarified the commit message to provide more context.

mxinden commented 3 years ago

@vyzo can you comment on why you added the id field to the Unregister message in https://github.com/libp2p/specs/commit/af5ba414c285e73b33843214be8c3a4c61ae69c1 ?

vyzo commented 3 years ago

honestly i dont remember ;)

On Fri, Jul 16, 2021, 14:08 Max Inden @.***> wrote:

@vyzo https://github.com/vyzo can you comment on why you added the id field to the Unregister message in af5ba41 https://github.com/libp2p/specs/commit/af5ba414c285e73b33843214be8c3a4c61ae69c1 ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libp2p/specs/pull/348#issuecomment-881367717, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI4SVBCXCIQ7IT2UEUYJTTYAHJNANCNFSM5AFYSRQQ .

mxinden commented 3 years ago

honestly i dont remember ;)

Thanks for the quick reply @vyzo.

Unless there are any objections, I will merge this pull request early next week.

@thomaseizinger would you mind bumping the specification revision number to help other implementors track the change?

thomaseizinger commented 3 years ago

@thomaseizinger would you mind bumping the specification revision number to help other implementors track the change?

Done!