tendermint: Change EventAttribute's key and value fields to Vec<u8> for Tendermint v0.34

[penso]

Fixes #1400

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Added entry in .changelog/

[romac]

@penso Great work, thanks so much! I did a few follow-up fixes, notably for the key field, which I believe is not enforced to be a valid base64-encoded UTF-8 string either as evidence by the Protobuf schema (even if it probably always is in practice).

[romac]

Before merging this I would like to get a few more eyes on this, as this is a pretty substantial breaking change and that I am not 100% convinced yet that this works for all uses cases.

@hdevalence @tony-iqlusion @ancazamfir @soareschen If any or all of you could take a look at this PR whenever you have some spare time, it would be much appreciated 💐

[romac]

On my side, I will try to integrate this into Hermes before we merge, and see how that goes.

[penso]

@penso Great work, thanks so much! I did a few follow-up fixes, notably for the key field, which I believe is not enforced to be a valid base64-encoded UTF-8 string either as evidence by the Protobuf schema (even if it probably always is in practice).

Glad it helped, didn't think about the key field but agreed if not enforced we should move that as well.

[hdevalence]

cc @erwanor

[romac]

I wonder if we should get rid of tendermint::abci::EventAttribute altogether and define two dialects of EventAttribute (and all transitive upstream dependencies): one for v0.34 and one for v0.37+. Then one would have to know statically which version of Tendermint/Comet they are dealing with to manipulate EventAttributes or any type which contains one.

[erwanor]

@penso Thanks for you work on this! @romac's suggestion to fold the v034 dialect in its own module, similar to the version based modeling of ABCI messages, makes sense to me.

[soareschen]

I took a quick look, and here are some of my thoughts:

• There should be a trait so that one can work with both variants of the EventAttributes in a uniform way. From what I see, bytes are more general than strings, and we can easily convert a string to bytes using .as_bytes(). So it should be sufficient to provide trait methods to access the raw bytes of the key/value.
• As @romac suggested, it would be good to provide two separate EventAttribute types, which would implement a common trait. That way, user code can still work generically with the two types using the common trait.
• Performance-wise, it may be expensive to perform base64 decoding on all events, regardless of whether the user is interested in the event. Depending on the actual performance, we may want to perform lazy parsing, at least on the value field, so that we can skip parsing events that do not match the key of interest.
  • On the other hand, it may be complicated to implement lazy parsing. So we may want to consider doing this only in future PRs, in case if the performance benefits are worth it.

[penso]

@romac @erwanor would love this merged for me to use to fetch blocks from 0.34 chains. As it's currently failing.

[romac]

In the interest of getting this merged quickly, let's go ahead with the current solution and see if we can improve on it later on based on @soareschen's suggestions.