Closed penso closed 2 months ago
@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).
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 💐
On my side, I will try to integrate this into Hermes before we merge, and see how that goes.
@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.
cc @erwanor
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.
@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.
I took a quick look, and here are some of my thoughts:
EventAttribute
s 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.EventAttribute
types, which would implement a common trait. That way, user code can still work generically with the two types using the common trait.@romac @erwanor would love this merged for me to use to fetch blocks from 0.34 chains. As it's currently failing.
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.
Fixes https://github.com/informalsystems/tendermint-rs/issues/1400
.changelog/