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

Use google.golang.org/protobuf messages in typed events #11278

Closed
Tracked by #11277
aaronc opened this issue Feb 25, 2022 · 5 comments
Closed
Tracked by #11277

Use google.golang.org/protobuf messages in typed events #11278

aaronc opened this issue Feb 25, 2022 · 5 comments
Labels
C:events C: Proto Proto definition and proto release T:feature-request

Comments

@aaronc
Copy link
Member

aaronc commented Feb 25, 2022

We can do a type switch in TypedEventToEvent to see if we have a new proto.Message. Then we can use the protoreflect.Message.Range method to convert the event to an ABCI event rather than marshaling to JSON in the middle. This should be more efficient than the current method and also allow us to store unquoted strings in attribute values (rather than what happens now with JSON).

Probably ParseTypedEvent should decode to a new proto.Message if one exists.

@aaronc
Copy link
Member Author

aaronc commented Aug 30, 2022

It we do this correctly, it should allow us to resolve any performance issues associated with JSON marshaling for typed events. We may even want to consider deprecating gogo support because of this.

The key thing is being able to iterate over the fields in a deterministic way which protoreflect.Message.Range is not guaranteed to do. Either we can sort fields, or have an extension interface that pulsar exposes with a DeterministicRange method.

@alexanderbez
Copy link
Contributor

Would rather sort the fields rather than rely on Pulsar.

@aaronc
Copy link
Member Author

aaronc commented Aug 30, 2022

Would rather sort the fields rather than rely on Pulsar.

I think we can support both options

@tac0turtle
Copy link
Member

i think if someone is sending events to tendermint they should not assume it doesnt come with a little overhead

@tac0turtle tac0turtle added T:feature-request C: Proto Proto definition and proto release C:events labels Mar 6, 2023
@tac0turtle
Copy link
Member

closing as we decided to deal with one proto type for now in the state machine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:events C: Proto Proto definition and proto release T:feature-request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants