Fix LeafVersion serde

[sanket1729]

The default implementation maps to visit_u64. The current implementation
does not roundtrip with many deserializers, including serde_json. See
the failing test in the second commit

[sanket1729]

This still fails on secp256k1::Parity type because of the same reason. I think we should also fix it there to use visit_u64 so that we users can use serde_json. Even though we are testing the structures with serde_test, I think it should be considered a bug if the given serde_json test fails.

Anyways, input from people using serde is appreciated. My intention is to fix #1327.

[apoelstra]

Heh, I'm surprised we're using serde_test anywhere because IME it's completely useless.

[apoelstra]

Having said this, okay, I'm fine deserializing u64s like this but IMO it's a bug in serde(_json) that you cannot actually roundtrip u8s by using serialize_u8 and deserialize_u8.

[Kixunil]

This looks like robustness principle on the surface but actually isn't so concept ACK.

IMO it's a bug in serde(_json)

Yeah, looks like one.

[apoelstra]

Looks like the CI failure is an actual code breakage of some sort.

[Kixunil]

tACK a400757

Checked that the test is failing before the fix.

[tcharding]

Heh, I'm surprised we're using serde_test anywhere because IME it's completely useless.

You meant "IMO", right? Can you expand on this? I ask because we use it in a few places and I added tests using it thinking it was useful, I'm interested to know if I was wrong?

[tcharding]

ACK a400757

[apoelstra]

You meant "IMO", right? Can you expand on this? I ask because we use it in a few places and I added tests using it thinking it was useful, I'm interested to know if I was wrong?

I meant "IME". This is one such case. The other I remember had to do with all of the types in rust-secp that wrap bytestrings. It is trivial to create things that serde_test will round-trip but actual de/serializers will not.

I went to search the git history for the removal of these tests but I actually it looks like we just left them in...presumably to avoid a json dependency, and because (hopefully) downstream users have round-trip tests of their own.

[tcharding]

lol, as my kids are fond of telling me "I'm such a boomer" I had to look up the definition of IME (In My Experience) :)

We have a serde_json dev dependency already so should it be on my todo-at-some-stage list to remove the serde_test usage and favour roundtrip tests with serde_json?

[sanket1729]

I think serde_test is the recommended way of testing. serde_json just has a weird bug around requiring everything as either u64 or i64.

[apoelstra]

@sanket1729 yes, serde_json has a bug around u64 and i64. But I have also run into trouble with its handling of byteslices (or at least, serde_test okayed something which did not round trip with serde_json).

Maybe I'm just upset about some old long-fixed bug. But it made me distrust serde_test.