consensus::State deserialization fixes

[greg-szabo]

Fixes #679.

This should make the consensus::State struct backwards compatible for JSON deserialization. Note that serialization will always use the new format which is not backwards compatible because of a rename from parts to part_set_header in one field.

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Updated CHANGELOG.md

[greg-szabo]

That's interesting. These failures didn't show up in local testing. Checking...

[greg-szabo]

All right, the test problem is my fault, I didn't run the full test suite after changing the total to a string.

[codecov-io]

Codecov Report

Merging #680 (7a1f0c1) into master (fa415e3) will increase coverage by 0.2%.
The diff coverage is 87.1%.

@@           Coverage Diff            @@
##           master    #680     +/-   ##
========================================
+ Coverage    39.2%   39.4%   +0.2%     
========================================
  Files         190     191      +1     
  Lines       12552   12595     +43     
  Branches     3237    3241      +4     
========================================
+ Hits         4927    4974     +47     
+ Misses       7381    7373      -8     
- Partials      244     248      +4     

Impacted Files	Coverage Δ
proto-compiler/src/constants.rs	0.0% <ø> (ø)
proto/src/serializers/from_str.rs	85.7% <ø> (ø)
proto/src/serializers/optional.rs	83.3% <0.0%> (ø)
proto/src/serializers/part_set_header_total.rs	75.0% <75.0%> (ø)
tendermint/src/block/parts.rs	68.5% <100.0%> (ø)
tendermint/src/consensus/state.rs	85.2% <100.0%> (+13.5%)	⬆️
tendermint/src/block/height.rs	67.2% <0.0%> (ø)
tendermint/src/hash.rs	56.9% <0.0%> (+3.2%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa415e3...7a1f0c1. Read the comment docs.

[romac]

Do we really need to define a type + a visitor?

I wonder if this can be expressed instead with just the serialize and deserialize functions, and invoked via a #[serde(with = "crate::serializers::part_set_header_total")] attribute?

[romac]

Instead of a visitor, we would use the deserialize method provided by the Deserialize trait: https://docs.rs/serde/1.0.117/serde/trait.Deserialize.html

eg. to deserialize a u64:

u64::deserialize(deserializer)

[greg-szabo]

Yes, that was the original solution. Unfortunately, the deserializer in the above example is not clonable and I need to be able to derive if the incoming input is an integer or a string.

Here's the outline of the original solution:

let try_string = String::deserialize(deserializer.clone());
if try_string.is_err() {
  u32::deserialize(deserializer)?
} else {
  try_string.unwrap()
}

The deserialize function consumes the deserializer.

So, you need to be able to figure out if the incoming data is an integer or a String before trying to deserialize it. Short of writing our own deserializer, the Visitor seemed the right solution. The "either string or struct" example in the Serde documentation also uses this solution: https://serde.rs/string-or-struct.html

Edit: obviously, I could have removed the if condition and used .or() or similar, this is a quick draft.

[romac]

Right, makes sense! That's an unfortunate limitation/design decision of serde that I wasn't aware of. Thanks for pointing me to the serde doc, had completely forgotten about this example, my bad.

All good then :)

[greg-szabo]

No worries, I'm grateful that you point out possible optimizations!