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

feature(consensus): add CentralStateDiff object in the aerospike format #2715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yael-Starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Yael-Starkware Yael-Starkware marked this pull request as ready for review December 17, 2024 09:54
Copy link

github-actions bot commented Dec 17, 2024

Artifacts upload workflows:

Copy link
Contributor

@DvirYo-starkware DvirYo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/lib.rs line 5 at r1 (raw file):

//! Implements the consensus context - the interface for consensus to call out to the node.

mod central_communication;

move this module to cende when merged


crates/sequencing/papyrus_consensus_orchestrator/src/central_communication/central_objects_test.rs line 73 at r1 (raw file):

    let python_json: Value = serde_json::from_str(&python_json_string).unwrap();

    assert_eq!(rust_json, python_json,);

we should compare here string representation of the JSON and not the JsonValue object

Copy link
Contributor Author

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/central_communication/central_objects_test.rs line 73 at r1 (raw file):

Previously, DvirYo-starkware wrote…

we should compare here string representation of the JSON and not the JsonValue object

The strings are not the same due to spaces and \n chars.
what is the problem with comparing jsonValue?

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

Successfully merging this pull request may close these issues.

3 participants