Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserialization of abci::Event fails #1415

Open
penso opened this issue Apr 27, 2024 · 6 comments · May be fixed by #1416
Open

Deserialization of abci::Event fails #1415

penso opened this issue Apr 27, 2024 · 6 comments · May be fixed by #1416
Labels
bug Something isn't working

Comments

@penso
Copy link
Contributor

penso commented Apr 27, 2024

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.

@penso penso added the bug Something isn't working label Apr 27, 2024
@penso penso linked a pull request Apr 27, 2024 that will close this issue
5 tasks
@hdevalence
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

penso commented Apr 27, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

penso commented Apr 27, 2024

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
Copy link
Member

romac commented May 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants