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

feat(blockifier): integrate Casm read from class manager client #3983

Conversation

elintul
Copy link
Collaborator

@elintul elintul commented Feb 5, 2025

Chose this simpler option to implement, instead of making Papyrus state generic by CR: CasmReader trait, since it would require cloning mdbx resources, which is flaky working with PyO3.

After native_blockifier is deprecated, the client should become non-Option and Papyrus state (in the new Batcher) should always be initialized with a concrete instance.

@reviewable-StarkWare
Copy link

This change is Reviewable

@elintul elintul force-pushed the elin/class-manager/papyrus-state/read-from-class-manager-client branch 5 times, most recently from 261626c to 17af2c7 Compare February 5, 2025 19:37
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.

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @elintul, and @meship-starkware)


crates/papyrus_state_reader/src/papyrus_state.rs line 54 at r2 (raw file):

            contract_class_manager,
            // TODO(Elin): integrate class manager client.
            class_reader: None,

Out of curiosity- where do you set this to Some?

Code quote:

            class_reader: None,

Copy link
Contributor

@meship-starkware meship-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 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @elintul)

Copy link
Collaborator Author

@elintul elintul 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, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1)


crates/papyrus_state_reader/src/papyrus_state.rs line 54 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Out of curiosity- where do you set this to Some?

Good Q (I guess you looked it up); nowhere yet. Shahak's team is still helping me integrating the class manager.
It will be set to Some(class_manager_client) when the new Batcher's state reader is initialized. :)

@elintul elintul force-pushed the elin/class-manager/papyrus-state/read-from-class-manager-client branch from 17af2c7 to ad7dd2a Compare February 8, 2025 14:40
@elintul elintul requested review from avivg-starkware and removed request for AvivYossef-starkware February 8, 2025 14:40
Chose this simpler option to implement, instead of making Papyrus state
generic by `CR: CasmReader` trait, since it would require cloning `mdbx`
resources, which is flaky working with PyO3.

After `native_blockifier` is deprecated, the client should become
non-`Option` and Papyrus state (in the new Batcher) should always be initialized
with a concrete instance.
@elintul elintul force-pushed the elin/class-manager/papyrus-state/read-from-class-manager-client branch from ad7dd2a to 4d6c655 Compare February 8, 2025 14:44
Copy link
Collaborator Author

@elintul elintul left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@elintul elintul added this pull request to the merge queue Feb 8, 2025
Merged via the queue into main with commit fb193d2 Feb 8, 2025
22 checks passed
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.

4 participants