Deserialization of abci::Event fails

[penso]

Deserialization of abci::Event fails

If you look at https://github.com/cometbft/cometbft/blob/main/api/cometbft/abci/v1/types.pb.go#L3012C82-L3012C91 you'll notice omitempty for Type. Trying to find previous similar issues it looks like the same as explained by @ebuchman in #197

Block 12791634 for DYDX has the following block_results:

    "finalize_block_events": [
      {
        "attributes": [
          {
            "key": "",
            "value": "\n0/dydxprotocol.clob.MsgProposedOperationsResponse",
            "index": false
          }
        ]
      }
    ]

It therefor doesn't have any type, deserialization fails.

[hdevalence]

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.

[hdevalence]

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).

[penso]

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.

[hdevalence]

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.

[penso]

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.

[romac]

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.