Add Protobuf typed/defined Penumbra ABCI Events #3492
Conversation
crates/proto/src/event.rs
Outdated
| // NOTE: Clarify if the serde derivation is pbjson/prost aware. Or, more specifically, if Penumbra generated serialization code for protobuf defs are more coherent than the | ||
| // 3-4 different ways the cosmo-sdk chooses to serializes and deserialize its protobuf data in the reference code. | ||
| // TODO: Would it be preferred to return an empty(?) or error coded abci::Event? | ||
| let event_json = serde_json::to_vec(&self.encode_to_vec()).expect("ProtoEvent constrained values should be JSON serializeable."); |
There was a problem hiding this comment.
Yeah, I made a mistake specifying the original issue. We use pbjson_build to ensure that all of the Rust proto types generated from .proto files have Serialize/Deserialize impls that produce ProtoJSON output.
So we should change the trait bound to be ProtoEvent: Message + Name + Serialize + Deserialize + Sized and use the Serialize/Deserialize impls directly, rather than doing encode_to_vec.
There was a problem hiding this comment.
this is clarifying. at some point I realized that I didn't know if there was a cost/benefit to relying on those traits being transitively resolved because of Message.
There was a problem hiding this comment.
Technically, they're not -- that's why we need to add them as extra bounds. It just happens to be the case (because of our build configuration) that every Message impl we generate also has a matching Serialize/Deserialize implementation.
In terms of the API, it would be the caller's responsibility to ensure that they only pass Message instances that were generated with pbjson_build-generated Serialize impls. But we don't need to worry about this too much because we do that in our build configs.
crates/proto/src/event.rs
Outdated
| // TODO: Would it be preferred to return an empty(?) or error coded abci::Event? | ||
| let event_json = serde_json::to_vec(&self.encode_to_vec()).expect("ProtoEvent constrained values should be JSON serializeable."); | ||
|
|
||
| let attribute_kv : HashMap<String, String> = serde_json::from_slice(&event_json).expect("A serde derived Vec<u8> should be deserializeable."); |
There was a problem hiding this comment.
I think this might be easier if instead of using serde_json::to_vec above, the serialization to JSON was done with serde_json::value::to_value https://docs.rs/serde_json/latest/serde_json/value/fn.to_value.html
In other words, we would serialize the Rust type to a JSON AST, then walk through the first-level leaves and generate an appropriate k/v pair for each leaf (using the field name as the key, and fully serializing the JSON AST as the value).
There was a problem hiding this comment.
completely missed that as a feature of the crate, thanks!
crates/proto/src/event.rs
Outdated
| let json = serde_json::to_value(attributes).expect("HashMap of String, serde_json::Value should be serializeable."); | ||
|
|
||
| // NOTE: Same thing here as into_event(), check what the equivalent to the jsonpb Unmarshaler is. | ||
| let json_bytes = serde_json::to_vec(&attributes_kv).expect("Hashmap of EventAttribute Keys and values should be serializeable."); | ||
| // let decoded_event = Self::decode(serde_json::from_slice(&json_bytes).expect("Serde derived serialized bytes should be deserializeable.")); | ||
| // Hitting a lifetime error that requires further restriction on Self: Deserialize that conflicts with the current supertraits defined above. | ||
| // return Ok(serde_json::from_value(json)?) |
There was a problem hiding this comment.
Ok, @hdevalence, all I'm left is figuring out how to get the final bit of equivalent unmarshalling/marshalling code in from_event method so I can do the actual test data/string munging of results. These lines are the rough equivalent to the final lines of the go impl here.
With the final bit twiddling, do I just want serde_json to give me back a value of Result<Self> like I have commented out above? If so, I'm hitting a Trait restriction compile error directly on Self that required an additional lifetime that had me re-writing the Trait's signature to the point I started leaking out the parameterized type variable into the StateWriterTrait trait.
I tried massaging things around but I want to make sure I'm not going on another snipe hunt again.
There was a problem hiding this comment.
What about using https://docs.rs/serde/latest/serde/de/trait.DeserializeOwned.html ?
…f with generated code
… using serde_json::{to/from_value}
- use DeserializeOwned to avoid lifetime bounds - use the fully qualified proto name for the event type, to avoid conflicts - add a hardcoded test vector (generated with the current code and manually inspected) - run rustfmt
This adds the EventOutput case, which exposed some issues with the (de)serialization logic since it has nested objects.
hdevalence
force-pushed
the
ejmg/3460-protobuf-abci-events
branch
from
83536d5 to
145253e
Compare
hdevalence
had a problem deploying
to
smoke-test
GitHub Actions
Error
hdevalence
temporarily deployed
to
smoke-test
GitHub Actions
Inactive
|
This looks great! I added a few commits to carry it over the finish line. In particular:
I had to make some changes to the (de)serialization to make it work with the In the original issue there was a suggestion to replace all of the existing events with proto-events. I don't think we should do that before merging this, it could be a separate issue. The events are not consensus-critical, so we can change them later (though it's slightly annoying to have different event formats in the same database or otherwise to re-execute all the blocks, etc) |
PR for #3460.