-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore(blockifier_reexecution): compile sierra to versioned runnable class #2679
chore(blockifier_reexecution): compile sierra to versioned runnable class #2679
Conversation
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.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @AvivYossef-starkware)
crates/blockifier_reexecution/src/state_reader/test_state_reader.rs
line 138 at r1 (raw file):
StarknetContractClass::Sierra(sierra) => { let (casm, _) = sierra_to_versioned_contract_class_v1(sierra).unwrap(); let runnable_contract_class: RunnableCompiledClass = casm.try_into().unwrap();
personally prefer to avoid let x: X = y.into()
when you can do let x = X::from(y)
non-blocking
Suggestion:
let runnable_contract_class = RunnableCompiledClass::try_from(casm).unwrap();
crates/blockifier_reexecution/src/state_reader/compile.rs
line 79 at r1 (raw file):
pub fn sierra_to_versioned_contract_class_v1( sierra: FlattenedSierraClass, ) -> StateResult<(ContractClass, SierraVersion)> {
doesn't this tuple type have a name you can use?
Code quote:
(ContractClass, SierraVersion)
crates/blockifier_reexecution/src/state_reader/compile.rs
line 100 at r1 (raw file):
.collect::<Vec<u64>>() .try_into() .expect("Expected exactly 3 elements.");
this version-from-sierra logic is duplicated, no? is this avoidable? I realize the type of the sierra contract isn't the same but is there something you can reuse?
non-blocking
Code quote:
let [major, minor, patch] = sierra
.sierra_program
.clone()
.into_iter()
.take(3)
.map(|big_uint_as_hex| u64::try_from(big_uint_as_hex.value).unwrap())
.collect::<Vec<u64>>()
.try_into()
.expect("Expected exactly 3 elements.");
crates/blockifier_reexecution/src/state_reader/offline_state_reader.rs
line 163 at r1 (raw file):
match self.get_contract_class(&class_hash)? { StarknetContractClass::Sierra(sierra) => { let (casm, _) = sierra_to_versioned_contract_class_v1(sierra).unwrap();
why is this line needed...? where is casm
used?
Code quote:
let (casm, _) = sierra_to_versioned_contract_class_v1(sierra).unwrap();
83b7b5a
to
ab56dd1
Compare
484c4f6
to
13950e4
Compare
ab56dd1
to
82253b3
Compare
13950e4
to
deeb020
Compare
82253b3
to
53465de
Compare
deeb020
to
84d38fd
Compare
53465de
to
6e1354f
Compare
84d38fd
to
d1f338f
Compare
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @AvivYossef-starkware)
6e1354f
to
9979a99
Compare
d1f338f
to
bb9d37c
Compare
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.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)
crates/blockifier_reexecution/src/state_reader/compile.rs
line 79 at r1 (raw file):
Previously, dorimedini-starkware wrote…
doesn't this tuple type have a name you can use?
No, I can define a new one but its in use only here
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)
crates/blockifier_reexecution/src/state_reader/compile.rs
line 79 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
No, I can define a new one but its in use only here
nvm then
9979a99
to
b073f78
Compare
bb9d37c
to
4fbece4
Compare
4fbece4
to
98f2e9c
Compare
Artifacts upload workflows: |
No description provided.