-
Notifications
You must be signed in to change notification settings - Fork 43
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 to v0.55 of rust-libp2p #209
Conversation
Signed-off-by: Dave Grantham <[email protected]>
@galargh any idea why I can't add a reviewer to this PR? I looked at github-mgmt but couldn't figure out what in there would allow me to do that. |
@DougAnderson444 could you give this a review for me? |
Signed-off-by: Dave Grantham <[email protected]>
@galargh nvrmnd, it auto-added the rust-libp2p maintainers |
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.
Hi Dave, overall looks good to me, left one comment
/// | ||
/// > **Note**: Prepends a variable-length prefix indicate the length of the message. This is | ||
/// > compatible with what [`read_length_prefixed`] expects. | ||
pub async fn write_length_prefixed( |
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.
It seems this functions don't need to be copy pasted, you can use quick-protobuf-codec
see libp2p/rust-libp2p#4788 (comment)
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.
ah thanks! I'll fix it.
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.
So I took a look at the quick-protobuf-codec example and it assumes that the messages are protobuf generated structs with a specific interface. I think leaving the manual varuint-length-prefixed-messages code in here is better. It amounts to a re-implementation of the quick-protobuf-codec without the protobufs.
In the future we may want to add to the misc/
folder an implementation of a "length-prefixed-codec" that has a public Trait HasLength (or whatever) that defines a single fn get_length() -> usize
function. Then the codec can have generic parameters that have the trait bounds of HasLength + AsRef<[u8]>.
Now that I think about it, it should probably use something like a SHA3 sponge construction where the protocol and the message type are mixed in as associated data and then the length is mixed and then then length is written followed by a MAC then the data is mixed in and written followed by another MAC to give the messages some authentication data.
Anyway, I think we should leave this as-is and just land it.
Thanks for the review.
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.
thanks for checking that Dave
Signed-off-by: Dave Grantham <[email protected]>
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.
LGTM
/// | ||
/// > **Note**: Prepends a variable-length prefix indicate the length of the message. This is | ||
/// > compatible with what [`read_length_prefixed`] expects. | ||
pub async fn write_length_prefixed( |
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.
thanks for checking that Dave
Updates the universal connectivity app to v0.55 of rust-libp2p.
Signed-off-by: Dave Grantham [email protected]