-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier_reexecution: merge rpc_state_reader into state_reader #11717
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
base: main
Are you sure you want to change the base?
blockifier_reexecution: merge rpc_state_reader into state_reader #11717
Conversation
b2066c3 to
12d1764
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AvivYossef-starkware partially reviewed 15 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @lev-starkware, and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 137 at r1 (raw file):
contract_class_mapping_dumper: Arc::new(Mutex::new(None)), } }
I think that is dead code, you can delete it, maybe in a later PR ( or TODO with my name )
Code quote:
impl Default for RpcStateReader {
fn default() -> Self {
let config = get_rpc_state_reader_config();
Self {
config,
block_id: BlockId::Latest,
retry_config: RetryConfig::default(),
chain_id: ChainId::Mainnet,
contract_class_mapping_dumper: Arc::new(Mutex::new(None)),
}
}crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 170 at r1 (raw file):
contract_class_mapping_dumper: Arc::new(Mutex::new(None)), } }
You can delete it, I don't think it is used
Code quote:
/// Creates an RpcStateReader for a specific block number.
/// Used for simple cases without dump mode or retry customization.
pub fn from_number(config: &RpcStateReaderConfig, block_number: BlockNumber) -> Self {
Self {
config: config.clone(),
block_id: BlockId::Number(block_number),
retry_config: RetryConfig::default(),
chain_id: ChainId::Mainnet,
contract_class_mapping_dumper: Arc::new(Mutex::new(None)),
}
}crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 182 at r1 (raw file):
contract_class_mapping_dumper: Arc::new(Mutex::new(None)), } }
loock like it is used only in tests so you can move it to a test file
Code quote:
/// Creates an RpcStateReader for the latest block.
/// Used for simple cases without dump mode or retry customization.
pub fn from_latest(config: &RpcStateReaderConfig) -> Self {
Self {
config: config.clone(),
block_id: BlockId::Latest,
retry_config: RetryConfig::default(),
chain_id: ChainId::Mainnet,
contract_class_mapping_dumper: Arc::new(Mutex::new(None)),
}
}crates/blockifier_reexecution/Cargo.toml line 12 at r2 (raw file):
blockifier_regression_https_testing = [] cairo_native = ["blockifier/cairo_native"] testing = []
why?
Code quote:
testing = []12d1764 to
1c043f2
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AvivYossef-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @lev-starkware, and @noaov1).
1c043f2 to
332092a
Compare
lev-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lev-starkware made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @AvivYossef-starkware, and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 170 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
You can delete it, I don't think it is used
Done.
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 182 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
loock like it is used only in tests so you can move it to a test file
Done.
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AvivYossef-starkware reviewed 6 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader_basic_test.rs line 0 at r4 (raw file):
I think renaming it to mock_rpc_state_reader_test or sone name which contains the word mock would be better
crates/blockifier_reexecution/src/state_reader/rpc_state_reader_test.rs line 4 at r1 (raw file):
/// /// This module contains: /// - Mock-based unit tests that use mockito to test RPC methods in isolation
plz move it to the new test file
Code quote:
/// - Mock-based unit tests that use mockito to test RPC methods in isolation97c8376 to
30596cb
Compare
e4862fe to
8258a76
Compare
30596cb to
014c72f
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AvivYossef-starkware reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware and @noaov1).
8258a76 to
49ee3ea
Compare
014c72f to
b0cf7d9
Compare
lev-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lev-starkware reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware and @noaov1).
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub reviewed 1 file and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware and @noaov1).

No description provided.