Fix panic in evidence serialization

[thanethomson]

Closes #782

• 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

[codecov-io]

Codecov Report

Merging #798 (8e663f7) into master (6f75a05) will increase coverage by 0.0%.
The diff coverage is 43.7%.

@@          Coverage Diff           @@
##           master    #798   +/-   ##
======================================
  Coverage    55.7%   55.7%           
======================================
  Files         198     198           
  Lines       14063   14068    +5     
  Branches     3696    3692    -4     
======================================
+ Hits         7834    7840    +6     
+ Misses       5897    5896    -1     
  Partials      332     332           

Impacted Files	Coverage Δ
proto/src/serializers/evidence.rs	42.8% <43.7%> (+16.7%)	⬆️

---

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 6f75a05...8e663f7. Read the comment docs.

[romac]

Seems like a good fix!

But I wonder if we actually need this whole custom serializer at all?

Disclaimer: It's 2am here and I am likely missing something, so please bear with me :)

It seems to me that this custom EvidenceVariant is defined the very same way as its counterpart in the generated code.

Custom EvidenceVariant:

 #[derive(Clone, PartialEq, ::serde::Deserialize, ::serde::Serialize)]
 #[serde(tag = "type", content = "value")]
 pub enum EvidenceVariant {
     #[serde(rename = "tendermint/DuplicateVoteEvidence")]
     DuplicateVoteEvidence(crate::tendermint::types::DuplicateVoteEvidence),
     #[serde(rename = "tendermint/LightClientAttackEvidence")]
    LightClientAttackEvidence(crate::tendermint::types::LightClientAttackEvidence),
}

Generated code in proto/src/prost/tendermint.types.rs

    #[derive(Clone, PartialEq, ::prost::Oneof, ::serde::Deserialize, ::serde::Serialize)]
    #[serde(tag = "type", content = "value")]
    pub enum Sum {
        #[prost(message, tag = "1")]
        #[serde(rename = "tendermint/DuplicateVoteEvidence")]
        DuplicateVoteEvidence(super::DuplicateVoteEvidence),
        #[prost(message, tag = "2")]
        #[serde(rename = "tendermint/LightClientAttackEvidence")]
        LightClientAttackEvidence(super::LightClientAttackEvidence),
    }

Could we instead simply remove the #[serde(from = "crate::serializers::evidence::EvidenceVariant", into = "crate::serializers::evidence::EvidenceVariant")] annotation from the generated Evidence type and get the very same behavior wrt. to serialization without having to introduce these weird From instances?

---

Okay, apparently it's not as simple as I though since my branch where I implemented the changes suggested above fails to deserialize the evidence: https://github.com/informalsystems/tendermint-rs/runs/1811124652

It's still not clear to me why this fails, so I would be grateful if someone can enlighten me :)

[romac]

Oh I think I got it: the RPC endpoint actually returns the EvidenceVariant directly rather than wrapped in an Evidence with a sum field. Nevermind then, we likely can't do much different than what we are doing here.

---

Yep, that's it. Confirmed by modifying rpc/tests/support/block_with_evidences.json.

[romac]

I still wonder if there's not another way to work around this discrepancy between the Protobuf definition and the JSON format returned via RPC.

Would it help if we made Evidence into a DomainType, like we do for other datatypes?

[thanethomson]

Yeah the whole reason for this custom serializer is because of how Protobuf wraps its enum types as structs. We generally use the enum type directly (and it's far more ergonomic to do so in Rust), whereas the generated Protobuf/prost types optionally wrap the enum in a struct.

Technically, we should never really encounter an instance where we receive evidence where the sum field is None, because I'd imagine we'd rather receive an empty EvidenceList. I'm still not sure why this is happening on the IBC side.

Would it help if we made Evidence into a DomainType, like we do for other datatypes?

We do actually have a domain type for it in the tendermint crate 😁 This struct in the tendermint-proto crate just performs the JSON de/serialization duties on behalf of the Protobuf struct, because we wanted to keep the JSON serialization out of the tendermint crate.

I have to say, it looks like the dream of a single set of data structures is quite far off. Part of this has to do with the annoying way in which prost generates enum types. Right now we generally have 2 sets of structs (prost and domain types), but in some cases it's unavoidable to also have separate JSON types (a third set).

If we were separating things "properly", we'd have one set of dedicated Protobuf types, one set of JSON types, and one set of domain types. But the sheer amount of manual code and maintenance required for that doesn't really seem justified, especially since so much of it's boilerplate.

---

On a separate note, it looks like the kvstore-test is failing consistently in this build for an unrelated serialization problem: https://github.com/informalsystems/tendermint-rs/pull/798/checks?check_run_id=1809593214 (the consensus state deserialization's failing, but it's not clear why). What makes this more frustrating is how these tests pass just fine on my local machine 😐