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

ZMQ PUB/SUB JSON Types #330

Merged
merged 21 commits into from
Nov 21, 2024
Merged

ZMQ PUB/SUB JSON Types #330

merged 21 commits into from
Nov 21, 2024

Conversation

dimalinux
Copy link
Contributor

@dimalinux dimalinux commented Oct 29, 2024

Adds all JSON serialization types needed to implement the ZMQ pub/sub daemon interface or to be a client of the daemon. (Subtask of #199).

There is a separate repo for testing against monerod. That program subscribes to all ZMQ messages, deserializes them into the types in this PR, then reserializes them back to JSON and verifies that the original and re-serialized JSON matches.

I have a few hundred megabytes of output from that program if you are looking for a large number of samples to study. The in-file unit tests are using the original, small sample set:
https://gist.github.com/dimalinux/50cc09956618f4322520246b5ec132dc

When merging, feel free to delete the individual commit message entries to reduce commit message noise.

@github-actions github-actions bot added A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-workspace Changes to a root workspace file or general repo file. A-zmq Related to ZMQ. labels Oct 29, 2024
/// included. `do-not-relay` transactions *are* included. Values are not
/// republished during a re-org.
#[derive(Debug, Default, Clone, Serialize, Deserialize)]
pub struct TxPoolAdd {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same structure as here:

pub enum Transaction {

Sorry for not noticing sooner if it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are similar, but their JSON layout, unfortunately, is completely different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ughh why does monero have 2 JSON tx formats 😭

zmq/types/Cargo.toml Outdated Show resolved Hide resolved
@dimalinux dimalinux marked this pull request as ready for review November 1, 2024 19:25
Copy link
Member

@SyntheticBird45 SyntheticBird45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided a suggestion but you will need to test it to be sure its not rejected by any lint.

Also if you could add your crate (and ZMQ category) to https://github.com/Cuprate/cuprate/blob/main/books/architecture/src/appendix/crates.md with your PR.

When merging main don't forget to replace cuprate-types import with workspace = true instead of path = "../../types". Also define cuprate-zmq-types crate as workspace dependency with the rest of cuprate workspace members

Otherwise LGTM. Good works.

zmq/types/src/json_message_types.rs Outdated Show resolved Hide resolved
zmq/types/src/json_message_types.rs Outdated Show resolved Hide resolved
zmq/types/src/json_message_types.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added A-docs Related to documentation. A-books Related to Cuprate's books. A-book-architecture Related to the Architecture book. labels Nov 2, 2024
Copy link
Member

@Boog900 Boog900 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking a while to review, mainly nits.

zmq/types/Cargo.toml Outdated Show resolved Hide resolved

[dev-dependencies]
serde_json = { workspace = true, features = ["std"] }
assert-json-diff = "2.0.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO I would rather have worse error messages on tests than pull an extra dep in

Copy link
Contributor Author

@dimalinux dimalinux Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled it in as a dev-only dependency, because testing JSON equality is a very non-trivial thing to do. Client readers are not supposed to care about whitespace or the order of fields within an object. If the input object is a map, its field output emit order can even vary between test runs.

In Golang, the feature to test JSON equality is built right into the most popular testing library (testify). assert-json-diff appeared to be the most widely used option for testing JSON-backwards-compatibility in Rust.

I did verify that the library differentiates between empty lists and nil lists, and the other types of backwards compatibility that we care about.

If, after reading this, we still don't want to use the assert-json-diff crate, let me know and I'll find a way to make the tests work without it. We are not using any maps, so it should be possible. The expected values will probably have to be on single, super-long lines, so they won't be human readable. Or I can get the test samples formatted in the same way as serde's pretty print and use multi-line raw strings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seen as it is only a dev dep we can keep it for now, although it might get removed to reduce the number of deps in the future

zmq/types/src/json_message_types.rs Outdated Show resolved Hide resolved
zmq/types/src/json_message_types.rs Outdated Show resolved Hide resolved
@Boog900 Boog900 merged commit 4b925b8 into Cuprate:main Nov 21, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-book-architecture Related to the Architecture book. A-books Related to Cuprate's books. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-docs Related to documentation. A-workspace Changes to a root workspace file or general repo file. A-zmq Related to ZMQ.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants