-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_os_runner: classes provider using class manager #11006
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
starknet_os_runner: classes provider using class manager #11006
Conversation
065855c to
578c249
Compare
f86b54c to
04f96ab
Compare
Yoni-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.
@Yoni-Starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware).
crates/starknet_os_runner/src/classes_provider.rs line 67 at r2 (raw file):
} // Fallback to fetch.
Why not reuse the mechanism of StateReaderAndContractManager?
You get almost everything for free. The casm hash is cached, etc.
Code quote:
// Fallback to fetch.
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 made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware).
crates/starknet_os_runner/src/classes_provider.rs line 67 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Why not reuse the mechanism of
StateReaderAndContractManager?
You get almost everything for free. The casm hash is cached, etc.
I have two problems witht that,
- the RPC state reader returns a dummy compiled class hash
- the RPC state reader compile to cam -> convert to contract_class v1 -> we need to convert to casm again
04f96ab to
56285d5
Compare
578c249 to
f231222
Compare
f231222 to
aa61b12
Compare
56285d5 to
bc7d82e
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 made 1 comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware).
crates/starknet_os_runner/src/classes_provider.rs line 67 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
I have two problems witht that,
- the RPC state reader returns a dummy compiled class hash
- the RPC state reader compile to cam -> convert to contract_class v1 -> we need to convert to casm again
Done
bc7d82e to
e6ce528
Compare
aa61b12 to
3a77b16
Compare
Yoni-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.
@Yoni-Starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware).
crates/starknet_os_runner/Cargo.toml line 10 at r5 (raw file):
[features] cairo_native = ["blockifier/cairo_native"]
Is this necessary?
Code quote:
cairo_native = ["blockifier/cairo_native"]e6ce528 to
864211d
Compare
3a77b16 to
2c77dea
Compare
Yoni-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.
@Yoni-Starkware made 3 comments and resolved 1 discussion.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware).
crates/starknet_os_runner/Cargo.toml line 10 at r5 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Is this necessary?
Oh, nevermind
crates/starknet_os_runner/src/classes_provider.rs line 35 at r5 (raw file):
for &class_hash in executed_class_hashes { let (compiled_class_hash, casm) = self.fetch_class(class_hash)?; compiled_classes.insert(compiled_class_hash, casm);
Add a TODO to parallelize this
Code quote:
for &class_hash in executed_class_hashes {
let (compiled_class_hash, casm) = self.fetch_class(class_hash)?;
compiled_classes.insert(compiled_class_hash, casm);crates/starknet_os_runner/src/classes_provider.rs line 40 at r5 (raw file):
} /// Fetch class by class hash.
Suggestion:
/// Fetches class by class hash.864211d to
818a21b
Compare
Yoni-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.
@Yoni-Starkware made 2 comments.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware).
crates/starknet_os_runner/src/classes_provider.rs line 74 at r5 (raw file):
entry_points_by_type: (&class.entry_points_by_type).into(), } }
And move this below all impl blocks
Suggestion:
/// Converts a `CompiledClassV1` to a `CasmContractClass` for OS execution.
/// Note: Some fields are not preserved in `CompiledClassV1` and are set to default values:
/// - `compiler_version`: Set to empty string
/// - `hints`: Set to empty (OS doesn't use them from this struct for Cairo 1 contracts)
/// - `pythonic_hints`: Set to None
fn compiled_class_v1_to_casm(class: &CompiledClassV1) -> CasmContractClass {
// TODO(Aviv): Consider using dummy prime since it is not used in the OS.
let prime = Felt::prime();
let bytecode: Vec<BigUintAsHex> = class
.program
.iter_data()
.map(|maybe_relocatable| match maybe_relocatable {
MaybeRelocatable::Int(felt) => BigUintAsHex { value: felt.to_biguint() },
_ => panic!("Expected all bytecode elements to be MaybeRelocatable::Int"),
})
.collect();
CasmContractClass {
prime,
compiler_version: String::new(),
bytecode,
bytecode_segment_lengths: Some(class.bytecode_segment_felt_sizes().into()),
hints: Vec::new(),
pythonic_hints: None,
entry_points_by_type: (&class.entry_points_by_type).into(),
}
}crates/starknet_os_runner/src/classes_provider.rs line 90 at r5 (raw file):
RunnableCompiledClass::V1(compiled_class_v1) => { let casm = compiled_class_v1_to_casm(&compiled_class_v1); let compiled_class_hash = self.get_compiled_class_hash(class_hash)?;
I think get_compiled_class_hash_v2 is the one that is already cached in the state-reader-and-class-manager
Code quote:
self.get_compiled_class_hash
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 resolved 2 discussions.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware).
Yoni-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.
@Yoni-Starkware reviewed 1 file.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions.
818a21b to
b32a537
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 made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @Yoni-Starkware).
crates/starknet_os_runner/src/classes_provider.rs line 90 at r5 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
I think
get_compiled_class_hash_v2is the one that is already cached in the state-reader-and-class-manager
right, notice that right now it return dummy value
Yoni-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.
@Yoni-Starkware reviewed 6 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: 6 of 7 files reviewed, all discussions resolved.
b32a537 to
01cc778
Compare
Yoni-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.
@Yoni-Starkware reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware).

No description provided.