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

dot/network: check if peer supports protocol before sending message #1571

Closed
noot opened this issue May 10, 2021 · 4 comments · Fixed by #1617
Closed

dot/network: check if peer supports protocol before sending message #1571

noot opened this issue May 10, 2021 · 4 comments · Fixed by #1617
Assignees

Comments

@noot
Copy link
Contributor

noot commented May 10, 2021

Task summary

  • currently we don't check if a peer supports a given protocol before attempting to open a stream with them/send a message over that stream
  • this results in an error
  • we should check before doing this that the peer supports the given protocol
@danigomez
Copy link
Contributor

danigomez commented May 26, 2021

Hi!,
For this issue each time that an attempt to open a NewStream (Like in

stream, err := h.h.NewStream(h.ctx, p, pid)
) is done an error should be returned right away if the protocol is not supported wihout attempting a connection to the peer??
I think that at least a connection to that peer should be tried in order to know if a protocol is supported or not, right??

@noot
Copy link
Contributor Author

noot commented May 31, 2021

@danigomez hey, sorry for the late reply. at the point NewStream is called there is already a connection established. this issue is specifically for the sendData function in notifications.go as this is the only function where outbound data is sent. essentially we want to check supported protocols before opening a stream with the remote peer in that function

@noot
Copy link
Contributor Author

noot commented May 31, 2021

also @danigomez we have a team member scheduled to work on this, so if you have already started on this please let us know ASAP

@danigomez
Copy link
Contributor

@noot no prob!. Let me know if there is any pending issue that i can work on. Thanks!

@noot noot closed this as completed in #1617 Jun 4, 2021
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 a pull request may close this issue.

4 participants