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

PublicKey de/serialization should support io::{Read,Write} traits. #76

Closed
dongcarl opened this issue Nov 4, 2018 · 8 comments
Closed
Milestone

Comments

@dongcarl
Copy link
Member

dongcarl commented Nov 4, 2018

See conversation here: rust-bitcoin/rust-bitcoin#183 (comment)

@apoelstra
Copy link
Member

I think this is blocked on free capability-less contexts. Labelling 0.12.

@apoelstra apoelstra added this to the 0.12 milestone Nov 4, 2018
@dongcarl
Copy link
Member Author

dongcarl commented Nov 6, 2018

Seems like bitcoin-core/secp256k1#553 is it

@apoelstra
Copy link
Member

Yep. Let's try to get 0.11.4 out and then we can start breaking things :)

@dongcarl
Copy link
Member Author

@apoelstra After talking with @sgeisler it seems that we can also just use the traits provided by serde? Otherwise we can use io::Read to serialize, but I'm not sure if that's what it's meant for.

@apoelstra
Copy link
Member

The serde traits are feature-gated.

@sgeisler
Copy link
Contributor

Otherwise we can use io::Read to serialize, but I'm not sure if that's what it's meant for.

Imo implementing io::Read for keys isn't the way to go since we would have a hard time complying to the contract of that trait: read can be called multiple times with a buffer of arbitrary size and the expected behavior (even though I didn't see it explicitly mentioned in the docs) seems to be to continue reading where the last read stopped.

If we are just talking about serializing then having a

serialize_der<W: io::Write>(writer: &mut W) -> io::Result<()>

function seems to be the most idiomatic/rusty solution.

If we don't want to support some crazy non allocating stream parsing deserialization doesn't seem like such a big problem since

deserialize_der(data: &[u8]) -> DeserializeResult<Self>

would suffice.

The stream parsing supporting alternative could be:

deserialize_der<R: io::Read>(reader: &mut R) -> DeserializeResult<Self>

I can try to implement it if you want.

@apoelstra
Copy link
Member

@sgeisler agreed. I like the idea of serializing through io::Write (and fmt::Write, which we already do through the Display trait). But for deserializing it's fine to just take a fixed byte slice.

@dongcarl
Copy link
Member Author

See #81 for discussion.

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