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

Consider adding support for serde #47

Closed
VictorKoenders opened this issue Oct 16, 2019 · 3 comments
Closed

Consider adding support for serde #47

VictorKoenders opened this issue Oct 16, 2019 · 3 comments
Assignees

Comments

@VictorKoenders
Copy link

It would be nice if the PublicKey would derive serde::Serialize and serde::Deserialize. Serde is the most used serialization library for Rust, and is presumably used a lot in network communication.

Serde support would change the following code:

enum NetworkPacket {
    SendPublicKey([u8; 32])
}

// sender
let key: PublicKey = ...;
let bytes = bincode::serialize(&NetworkPacket::SendPublicKey(key.as_bytes().clone())).unwrap();
// receiver
let buffer: &[u8] = ..;
let packet: NetworkPacket = bincode::deserialize(buffer).unwrap();
match packet {
    NetworkPacket::SendPublicKey(key) => {
        let key: PublicKey = key.into();
    }
}

To:

enum NetworkPacket {
    SendPublicKey(PublicKey)
}

// sender
let key: PublicKey = ...;
let bytes = bincode::serialize(&NetworkPacket::SendPublicKey(key)).unwrap();
// receiver
let buffer: &[u8] = ..;
let packet: NetworkPacket = bincode::deserialize(buffer).unwrap();
match packet {
    NetworkPacket::SendPublicKey(key) => {
        // key is already of type `PublicKey`
    }
}

Which is slightly nicer to work with.

This could also be implemented for StaticSecret, as this is intended to be able to be saved and loaded.

I'd suggest adding a feature called serde, which enables/disables the serde dependency and is disabled by default. When this feature is enabled, PublicKey implements serde::Serialize and serde::Deserialize.

I can implement this if you want me to.

@hdevalence
Copy link
Contributor

I think this would be a great feature, but the serde support in the underlying library will probably change soon, because it was implemented suboptimally/incorrectly: dalek-cryptography/curve25519-dalek#265

I have been very busy with a work project for the last few weeks, but I am hoping to close it out this Friday, so we could possibly revisit this issue next week?

@hdevalence hdevalence self-assigned this Oct 16, 2019
@VictorKoenders
Copy link
Author

This is just a minor thing I thought could improve. Don't worry about implementing this if you have things to do in real life, that obviously has priority. I'll look at the curve change and after that's implemented I'll revisit this issue.

@hdevalence hdevalence removed their assignment Oct 24, 2019
@DebugSteven DebugSteven self-assigned this Oct 24, 2019
@hdevalence
Copy link
Contributor

Closed by #48 :)

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

No branches or pull requests

3 participants