Skip to content

Conversation

@einat-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

einat-starkware commented Jan 4, 2026

@einat-starkware einat-starkware marked this pull request as ready for review January 4, 2026 10:00
@einat-starkware einat-starkware requested a review from noaov1 January 4, 2026 10:02
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch 3 times, most recently from e5e04ce to da8ef68 Compare January 5, 2026 09:36
@einat-starkware einat-starkware force-pushed the einat/proof_manager/add_to_integration_test branch from 90d9641 to 52e19d6 Compare January 5, 2026 09:36
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from ee2edf4 to 7995c4f Compare January 5, 2026 14:42
@einat-starkware einat-starkware force-pushed the einat/proof_manager/add_to_integration_test branch from 2b100d1 to 2b06b3d Compare January 5, 2026 14:42
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

@noaov1 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @einat-starkware).

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

@noaov1 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @einat-starkware).


crates/apollo_proof_manager/src/proof_storage.rs line 20 at r1 (raw file):

    fn set_proof(&self, proof_facts: ProofFacts, proof: Proof) -> Result<(), Self::Error>;
    fn get_proof(&self, proof_facts: ProofFacts) -> Result<Option<Proof>, Self::Error>;
    fn contains_proof(&self, proof_facts: ProofFacts) -> Result<bool, Self::Error>;

Do you need an actual copy of the proof facts?

Suggestion:

    fn set_proof(&self, proof_facts: &ProofFacts, proof: Proof) -> Result<(), Self::Error>;
    fn get_proof(&self, proof_facts: &ProofFacts) -> Result<Option<Proof>, Self::Error>;
    fn contains_proof(&self, proof_facts: &ProofFacts) -> Result<bool, Self::Error>;

@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from 7995c4f to 1a5aa6f Compare January 6, 2026 08:27
Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

@einat-starkware made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1).


crates/apollo_proof_manager/src/proof_storage.rs line 20 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Do you need an actual copy of the proof facts?

You're right, I don't need it :), updated to use a reference

@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from 1a5aa6f to f225f91 Compare January 6, 2026 08:55
Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

@einat-starkware made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1).


crates/apollo_proof_manager/src/proof_storage.rs line 20 at r1 (raw file):

Previously, einat-starkware wrote…

You're right, I don't need it :), updated to use a reference

Never mind, I actually do need the proof_facts themselves because for reasons relating to the serde and async requirements in the client, I would need to clone in the client anyways and all the interaction with the proof manager happens through the client so I would be cloning anyways, so there's no benefit to using a reference here. Regardless, the proof facts is wrapped in Arc so the benefit is negligible anyways to avoiding the clone

@einat-starkware einat-starkware force-pushed the einat/proof_manager/add_to_integration_test branch from 2b06b3d to ddec95e Compare January 7, 2026 07:07
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from f225f91 to 178943a Compare January 7, 2026 07:07
@einat-starkware einat-starkware changed the base branch from einat/proof_manager/add_to_integration_test to graphite-base/11382 January 10, 2026 22:04
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from 178943a to 135fe74 Compare January 11, 2026 08:20
@einat-starkware einat-starkware changed the base branch from graphite-base/11382 to einat/proof_manager/add_to_integration_test January 11, 2026 08:20
Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

@einat-starkware resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @noaov1).

@graphite-app graphite-app bot changed the base branch from einat/proof_manager/add_to_integration_test to graphite-base/11382 January 11, 2026 11:26
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from 135fe74 to bfca5a8 Compare January 11, 2026 11:54
@einat-starkware einat-starkware changed the base branch from graphite-base/11382 to main January 11, 2026 11:54
@einat-starkware einat-starkware force-pushed the einat/proof_manager/calc_hash_in_proof_storage branch from bfca5a8 to 3db1952 Compare January 11, 2026 11:55
@graphite-app
Copy link

graphite-app bot commented Jan 11, 2026

Merge activity

  • Jan 11, 11:55 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

@einat-starkware reviewed 3 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @einat-starkware).

@einat-starkware einat-starkware added this pull request to the merge queue Jan 11, 2026
Merged via the queue into main with commit 893785c Jan 11, 2026
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants