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

Erroneous Serialize implementation on abci::Event #1299

Closed
romac opened this issue Apr 17, 2023 · 1 comment
Closed

Erroneous Serialize implementation on abci::Event #1299

romac opened this issue Apr 17, 2023 · 1 comment

Comments

@romac
Copy link
Member

romac commented Apr 17, 2023

[…] Basically, there's a stray derive(Serialize) on Event, which declares an incorrect serialization format, incorrect because (a) it misses a derive(Deserialize), so it's not usable, and (b) because it's derived from the domain type, it won't be compatible with anything else in the ecosystem. But it's a moot point because I dropped the commit. It could be good to do a review pass for Serialize derives; IMO it's a pretty dangerous pattern compared to the domain/serialization type pattern as with the proto infrastructure (where serialization is done on a dedicated facade type) because it secretly defines an additional external interface with long-lasting implications based off of an accident of the internal API.

Originally posted by @hdevalence in #1288 (comment)

@mzabaluev
Copy link
Contributor

This has been fixed by restoring the Deserialize derive and making this serialization format the official as per JSON-RPC starting from 0.37. We don't explicitly care about other serializers, but the derives on the domain types provide the most straightforward data model for serde, so it should work reasonably well everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants