Add a TypedEvent conversion trait for ABCI events.

[hdevalence]

Downstream users of ABCI need to encode data in ABCI Events. Different consumers of these events may have different requirements on the event structure. For instance, the Go IBC relayer expects to see an event with a hex-encoded representation of a client state.

To ensure that there's a single source of truth for these kinds of encodings, it would be useful for downstream users of the abci::Event type to be able to write custom domain types for particular event types. This PR adds an abci::TypedEvent trait that captures the conversion relationships between an Event and a domain type. It also defines a contract about how the conversion between raw events and domain types should handle index information, which is nondeterministic: if T: TypedEvent, then:

• T::try_from(e) == Ok(t) for all t: T, e: Event where Event::from(t).eq_ignoring_index(e) == true.
• Event::from(T::try_from(e).unwrap()).eq_ignoring_index(e) == true for all e: Event where T::try_from(e) returns Ok(_).

This allows downstream code to separate concerns between the event data itself (in T), and instance-specific configuration of indexing of the event's attributes (e.g., by structuring the application so that outbound events are passed through a filter that adjusts the index fields based on configuration data).

Finally, I also removed an erroneous Serialize implementation on Event. As explained in the commit, this is a bugfix, not a breaking change, and I believe it should be included in an 0.30.1 release rather than an 0.31.0 release. (If other developers feel differently, our preference would be for it to be pulled out, so that we can start using TypedEvent on the 0.30.x series). EDIT: Removed as it broke other code in the repo; I still think it is a bug.

[codecov-commenter]

Codecov Report

Merging #1288 (4310585) into main (2067103) will increase coverage by 0.1%.
The diff coverage is 85.4%.

❗ Current head 4310585 differs from pull request most recent head 639bf6a. Consider uploading reports for the commit 639bf6a to get more accurate results

@@           Coverage Diff           @@
##            main   #1288     +/-   ##
=======================================
+ Coverage   64.1%   64.2%   +0.1%     
=======================================
  Files        271     271             
  Lines      24328   24450    +122     
=======================================
+ Hits       15596   15717    +121     
- Misses      8732    8733      +1     

Impacted Files	Coverage Δ
tendermint/src/abci/event.rs	61.6% <85.4%> (+59.5%)	⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[romac]

Sounds good! While we're at it, can we also derive Hash on Event and EventAttribute and add a hash_ignoring_index method to both structs?

---

Can you also please expand a bit on this part of your comment? Or perhaps open another issue/PR for this, with the rationale behind the change?

Finally, I also removed an erroneous Serialize implementation on Event. As explained in the commit, this is a bugfix, not a breaking change, and I believe it should be included in an 0.30.1 release rather than an 0.31.0 release. (If other developers feel differently, our preference would be for it to be pulled out, so that we can start using TypedEvent on the 0.30.x series).

[romac]

Let's also merge main in, which should fix the docs build failure.

[hdevalence]

Sounds good! While we're at it, can we also derive Hash on Event and EventAttribute and add a hash_ignoring_index method to both structs?

Sure, sounds good to me. I'll merge this after making those changes.

Can you also please expand a bit on this part of your comment? Or perhaps open another issue/PR for this, with the rationale behind the change?

Finally, I also removed an erroneous Serialize implementation on Event. As explained in the commit, this is a bugfix, not a breaking change, and I believe it should be included in an 0.30.1 release rather than an 0.31.0 release. (If other developers feel differently, our preference would be for it to be pulled out, so that we can start using TypedEvent on the 0.30.x series).

Sorry about that, the description was in a commit that I dropped because it did cause some minor breakage. Basically, there's a stray derive(Serialize) on Event, which declares an incorrect serialization format, incorrect because (a) it misses a derive(Deserialize), so it's not usable, and (b) because it's derived from the domain type, it won't be compatible with anything else in the ecosystem. But it's a moot point because I dropped the commit. It could be good to do a review pass for Serialize derives; IMO it's a pretty dangerous pattern compared to the domain/serialization type pattern as with the proto infrastructure (where serialization is done on a dedicated facade type) because it secretly defines an additional external interface with long-lasting implications based off of an accident of the internal API.

[romac]

@hdevalence Thanks for the explanation! Now tracked in #1299