Skip to content

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.

@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