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(starknet_batcher): integrate Papyrus state with class manager to Batcher #4314

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

Conversation

elintul
Copy link
Collaborator

@elintul elintul commented Feb 20, 2025

By doing this, cairo-native is reenabled.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented Feb 20, 2025

@elintul elintul force-pushed the elin/class-manager/call-runtime-block-on-in-papyrus-state branch 3 times, most recently from aa18917 to e07189e Compare February 20, 2025 12:20
… Batcher

By doing this, `cairo-native` is reenabled.
@elintul elintul force-pushed the elin/class-manager/call-runtime-block-on-in-papyrus-state branch from e07189e to c660a03 Compare February 20, 2025 12:28
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @giladchase and @yair-starkware)


crates/starknet_batcher/Cargo.toml line 21 at r1 (raw file):

papyrus_storage.workspace = true
serde.workspace = true
starknet-types-core.workspace = true

cargo-machete asked me for some reason.

Code quote:

starknet-types-core.workspace = true

Copy link
Contributor

@yair-starkware yair-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 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @giladchase)


crates/papyrus_state_reader/src/papyrus_state.rs line 91 at r1 (raw file):

        latest_block: BlockNumber,
        contract_class_manager: ContractClassManager,
        class_reader: Option<ClassReader>,

Why is the class reader an option?

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: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @giladchase and @yair-starkware)


crates/papyrus_state_reader/src/papyrus_state.rs line 91 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Why is the class reader an option?

Good Q.

It's None in native_blockifier; there's a comment above.
Reason for this inelegant solution is that this reader is used in both native_blockifier and new Batcher, and in native_blockifier I can't clone any mdbx resources, since it can easily cause a leak when moving from Rust to Python.

Another option was to duplicate this reader, but I chose this easier faster solution.
I have a task on Monday to remove this Option once native_blockifier is deprecated.

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