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

Adding specification of more protocols #137

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ben221199
Copy link

This pull request adds specifications of more protocols than only dnsaddr.

I think this pull request should be merged when I have added more, but now it is already possible to see the changes and talk about improvements.

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.

Very much appreciated! Thanks for the help here!

To prevent any of the discussions on one protocol to block merging another, I would suggest to use separate pull requests for each. That is not a strong opinion though.

@ben221199
Copy link
Author

I think that adding the basis using this pull request is fine. We can add more information for a specific protocol using other pull requests.

@mxinden Also, can you tell me what the binary format of dns, dns4, dns6 and dnsaddr is? Is it already defined? If not, I would suggest the FQDN format with a length-byte for every label.

Also, I have made some other pull requests on this and on other repositories. Hope someone can take a look at those too.

@mxinden
Copy link
Member

mxinden commented Aug 23, 2022

@mxinden Also, can you tell me what the binary format of dns, dns4, dns6 and dnsaddr is? Is it already defined? If not, I would suggest the FQDN format with a length-byte for every label.

Hope I understand your question correctly @ben221199. As far as I know it is just the utf-8 byte representation.

https://github.com/multiformats/rust-multiaddr/blob/911b18a51fafdee44a445dc491192f85430fbbbf/src/protocol.rs#L393-L416

@ben221199
Copy link
Author

w.write_all(encode::usize(bytes.len(), &mut encode::usize_buffer()))?;

This code above will prefix the data with an unsigned byte, yes.

Are the implementations in this organization authoritive at the moment, or is it still possible to alter the binary format at this time?

@ben221199 ben221199 marked this pull request as draft August 23, 2022 15:51
@ben221199
Copy link
Author

Okay, I think I added enough protocols for now. I think that, except for maybe some slight changes, this pull request can be merged.

@ben221199 ben221199 marked this pull request as ready for review August 23, 2022 17:13
@mxinden
Copy link
Member

mxinden commented Aug 26, 2022

Are the implementations in this organization authoritive at the moment, or is it still possible to alter the binary format at this time?

They are used in many production systems, thus I would say yes.

protocols/DNS4.md Outdated Show resolved Hide resolved
protocols/P2P.md Outdated Show resolved Hide resolved
@ben221199
Copy link
Author

Above suggestions are applied.

@ben221199 ben221199 requested a review from mxinden August 26, 2022 09:51
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.

2 participants