search

w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
306 stars 92 forks source link

PublicKey incompatible with serde_json #66

Closed ghost closed 3 years ago

ghost commented 3 years ago

During testing I encountered an error when trying to deserialize a PublicKey from a serde_json::Value

Error:

thread 'main' panicked at 'unable to deserialize PublicKey: Error("invalid type: sequence, expected A Ristretto Schnorr public key represented as a 32-byte Ristretto compressed point", line: 0, column: 0)', src/main.rs:11:40

example code can be found here.

I think this can be fixed in multiple ways, either by implementing visit_seq or by making use of serde_bytes (even tho it will probably mean that it'll be necessary to wrap with Bytes during serialization / deserialization manually instead of using the attributes offered)

burdges commented 3 years ago

Can you explain what fails in https://github.com/w3f/schnorrkel/blob/master/src/serdey.rs please? A priori I'd expect visit_seq should fail when invoked upon a simple byte array because that's what visit_bytes does, no?

Appears ed25519-dalek used serde_bytes without using Bytes anyplace.

ghost commented 3 years ago

I don't understand why the issue was closed...? There's nothing failing in serdey per se, it's just that not all format specialise sequence of bytes as just bytes, json being one of those... This poses an issue with the current implementation during deserialization, with the reported error, where a sequence is encountered but serdey doesn't handle it

burdges commented 3 years ago

I just pressed the wrong button due to it being late..

Do we need a serde_bytes feature like ed25519-dalek added? If so, we're basically replaying https://github.com/dalek-cryptography/ed25519-dalek/commit/69eccda4449b564aff57a776c6a0ed51fce01123 yes? I'll accept a PR for that of course.

ghost commented 3 years ago

There's a problem in implementing the deserialization part, namely a BytesBuf becomes somewhat necessary if you want sequence support since Bytes doesn't implement visit_sequence, probably since the size is now known so you'd need to allocate. I'm probably gonna impl with Bytes and if alloc is provided as feature then use ByteBuf instead

burdges commented 3 years ago

I think a serde_bytes feature flag sounds good, mostly because ed25519-dalek did so.