-
Notifications
You must be signed in to change notification settings - Fork 69
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
Update networking section #333
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grandpa and authority-discovery are missing, but I guess it's intentional for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed Appendix E. Comments are in a0c8160
Appendix e reviewed by @drskalman. After @lamafab goes over the changes, @FlorianFranzen will review the result again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work on consolidating all these different sources and getting rid of all these outdated chapters and appendices.
I would just like to ask you to make it a bit more clear how the protocol identifiers in the sections "Connection establishment" and "Protocols and Substreams" are related, including defining what "After the protocol is negotiated" means and which substreams uses which type of substream protocols.
It might also be useful to mention that protobuffer encodes each fields as a key value pair using the provided id, etc, but it is hardly a must. People can always read the official documentation.
@FlorianFranzen The "protocol identifiers" section was removed from the spec, since it's not directly relevant. This spec is exclusively about Polkadot and those identifiers are essentially just prefixes on substreams. I did add a note in the "Protocols and Substreams" section about it, however. Each substream now mentions the substream type (Request-Response or Notification substream). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thank you for getting these last things fixed. LGTM!
943e86b
to
d457f39
Compare
…tion. mention scale and protobuf situation
d457f39
to
0b782a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I am sure that was not fun to rebase. Just a few smaller things left to fix.
[Early Review] Update networking section
[Early Review] Update networking section
Fixes #234.
Things to consider: