Closed penso closed 4 weeks ago
Probably Event's serialize impls should run through the proto type (using Serde's try_from/into attributes) and the proto type should have a ProtoJSON compatible Serde impl via pbjson_build.
Example of how this works for a random Penumbra data structure: the CompactBlock
rust type serializes through the proto type:
https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/compact-block/src/compact_block.rs#L20-L21
The corresponding proto type has a generated Serde impl that handles this correctly:
https://github.com/penumbra-zone/penumbra/blob/main/crates/proto/src/gen/penumbra.core.component.compact_block.v1.serde.rs
That code is generated using pbjson_build
:
https://github.com/penumbra-zone/penumbra/blob/main/tools/proto-compiler/src/main.rs#L135-L147
There's a bit more machinery here but the upshot is that this fixes the problem in every case. (In Penumbra, we never derive Serde formats from the Rust representation, all our data formats are specified only in protos, with no custom "shaping" of the format, to avoid these kinds of inconsistencies).
The proto file at https://github.com/cometbft/cometbft/blob/main/proto/cometbft/abci/v1/types.proto#L467 shows the type
field isn't optional, the generated Rust struct from the proto file wouldn't be optional either and same bug would occur. You'd need to add the same fix serde(default)
for this specific field when generating the Rust struct from the proto file.
The fix would be for the Go implementation to not have omitempty
.
The proto file at https://github.com/cometbft/cometbft/blob/main/proto/cometbft/abci/v1/types.proto#L467 shows the type field isn't optional
No, it doesn't, it says
string type = 1;
All proto3 fields are optional. A message without a field 1
(in binary protobuf encoding) or a type
field (in ProtoJSON) is decoded with type = ""
since the empty string is the default value for the Protobuf string scalar type. Additionally, a value of an Event
message where type = ""
should be encoded without the type field.
Ah nice, wasn't aware all fields were optional. I agree as much as possible should be automated from proto files when possible, my internal code uses default Serde Serialize
I definitely need to move to pbjson
as well for it.
I agree with @hdevalence that we should eventually move all serialization to ProtoJSON, but that's a much larger piece of work that I would rather do all at once in a dedicated PR (and perhaps only do in the upcoming cometbft-rs
library. So for now, let's go with the default
annotation.
Deserialization of
abci::Event
failsIf you look at https://github.com/cometbft/cometbft/blob/main/api/cometbft/abci/v1/types.pb.go#L3012C82-L3012C91 you'll notice
omitempty
forType
. Trying to find previous similar issues it looks like the same as explained by @ebuchman in https://github.com/informalsystems/tendermint-rs/issues/197Block 12791634 for DYDX has the following block_results:
It therefor doesn't have any
type
, deserialization fails.