Skip to content

feat(starknet_batcher): state reader get compiled classes from class manager#3676

Merged
noamsp-starkware merged 1 commit intomainfrom
noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager
Jan 27, 2025
Merged

feat(starknet_batcher): state reader get compiled classes from class manager#3676
noamsp-starkware merged 1 commit intomainfrom
noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager

Conversation

@noamsp-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

noamsp-starkware commented Jan 26, 2025

@github-actions
Copy link

github-actions bot commented Jan 26, 2025

@noamsp-starkware noamsp-starkware marked this pull request as ready for review January 26, 2025 12:51
Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

@alonh5 or someone else from your team, please review this :)

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware)


crates/papyrus_state_reader/src/papyrus_state_wrapper.rs line 15 at r1 (raw file):

use crate::papyrus_state::PapyrusReader;
pub struct PapyrusReaderWrapper {

Rename to PapyrusReaderWithClassManager


crates/papyrus_state_reader/src/lib.rs line 2 at r1 (raw file):

pub mod papyrus_state;
pub mod papyrus_state_wrapper;

Move this file to the batcher

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager branch 2 times, most recently from 6152f9b to cae6c3b Compare January 26, 2025 14:33
Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware)


crates/starknet_batcher/src/papyrus_state_wrapper.rs line 15 at r2 (raw file):

use starknet_types_core::felt::Felt;
pub struct PapyrusReaderWithClassManager {
    papyrus_reader: PapyrusReader,

Change this into a generic StateReader and in the tests use MockStateReader. Rename the struct to ReaderWithClassManager and rename the file accordingly

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager branch from cae6c3b to 653d6e9 Compare January 26, 2025 15:05
Copy link
Contributor Author

@noamsp-starkware noamsp-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: 4 of 11 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)


crates/starknet_batcher/src/papyrus_state_wrapper.rs line 15 at r2 (raw file):

Previously, ShahakShama wrote…

Change this into a generic StateReader and in the tests use MockStateReader. Rename the struct to ReaderWithClassManager and rename the file accordingly

Done.

Copy link
Collaborator

@ShahakShama ShahakShama 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 r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware)


crates/blockifier/src/state/state_api.rs line 24 at r3 (raw file):

/// The `self` argument is mutable for flexibility during reads (for example, caching reads),
/// and to allow for the `State` trait below to also be considered a `StateReader`.
#[cfg_attr(feature = "testing", automock)]

@dorimedini-starkware please approve this change


crates/starknet_batcher/src/reader_with_class_manager_test.rs line 14 at r3 (raw file):

#[tokio::test]
async fn test_get_compiled_class() {

In a different PR, create a test for each of the other methods to make sure that you pass the call to the inner state reader

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 @noamsp-starkware and @ShahakShama)


crates/blockifier/src/state/state_api.rs line 24 at r3 (raw file):

Previously, ShahakShama wrote…

@dorimedini-starkware please approve this change

  1. what is automock?
  2. consider using mockall::automock so you don't need the extra gated use at the top
  3. if you gate code behind feature = "testing" and not test, then that means your crate tests do not use this and it is only a test util for external use (we will be disallowing crates depending on self with the testing feature activated - we want no dependency cycles). If it is also used for internal testing please use #[cfg_attr(any(test, feature = "testing"), automock)]

Copy link
Collaborator

@ShahakShama ShahakShama 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 @noamsp-starkware)


crates/blockifier/src/state/state_api.rs line 24 at r3 (raw file):

Previously, dorimedini-starkware wrote…
  1. what is automock?
  2. consider using mockall::automock so you don't need the extra gated use at the top
  3. if you gate code behind feature = "testing" and not test, then that means your crate tests do not use this and it is only a test util for external use (we will be disallowing crates depending on self with the testing feature activated - we want no dependency cycles). If it is also used for internal testing please use #[cfg_attr(any(test, feature = "testing"), automock)]
  1. It creates a struct called Mock* where * is name of trait that for each function in the trait annotated as foo it has a function called expect_foo that tells what to return when someone calls foo
  2. Good idea
  3. I agree. @noamsp-starkware please make this change

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager branch from 653d6e9 to 8c85c60 Compare January 27, 2025 10:21
@noamsp-starkware noamsp-starkware changed the base branch from noam.s/feat_starknet_gateway_state_reader_get_compiled_classes_from_class_manager to main January 27, 2025 10:21
Copy link
Contributor Author

@noamsp-starkware noamsp-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: 5 of 12 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @ShahakShama)


crates/starknet_batcher/src/reader_with_class_manager_test.rs line 14 at r3 (raw file):

Previously, ShahakShama wrote…

In a different PR, create a test for each of the other methods to make sure that you pass the call to the inner state reader

Added a TODO for this.

Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@noamsp-starkware noamsp-starkware added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit ee9ac0f Jan 27, 2025
19 of 35 checks passed
@noamsp-starkware noamsp-starkware deleted the noam.s/feat_starknet_batcher_state_reader_get_compiled_classes_from_class_manager branch January 27, 2025 11:14
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2025
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