-
Notifications
You must be signed in to change notification settings - Fork 207
Add_all_cairo_stwo_layout #1957
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
Conversation
|
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1957 +/- ##
==========================================
+ Coverage 96.46% 96.53% +0.06%
==========================================
Files 103 103
Lines 42507 42586 +79
==========================================
+ Hits 41005 41109 +104
+ Misses 1502 1477 -25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bc25090 to
6182e1f
Compare
OmriEshhar1
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.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @yuvalsw)
vm/src/types/instance_definitions/builtins_instance_def.rs line 401 at r1 (raw file):
assert!(builtins.poseidon.is_some()); }
add get_builtins_all_cairo_stwo() test?
vm/src/types/layout.rs line 470 at r1 (raw file):
); }
add get_all_cairo_stwo_instance() test?
YairVaknin-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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/types/layout.rs line 470 at r1 (raw file):
Previously, OmriEshhar1 wrote…
add get_all_cairo_stwo_instance() test?
yeah, they are ready. will post later. I pushed without any tests to see where CI fails and then add all at once.
JulianGCalderon
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.
Hi @YairVaknin-starkware! The codecov workflow seems to be failing. Could you add some unit tests? You can use an an example these tests:
cairo-vm/vm/src/types/instance_definitions/builtins_instance_def.rs
Lines 402 to 413 in 6182e1f
#[test] fn get_builtins_all_solidity() { let builtins = BuiltinsInstanceDef::all_solidity(); assert!(builtins.output); assert!(builtins.pedersen.is_some()); assert!(builtins.range_check.is_some()); assert!(builtins.ecdsa.is_some()); assert!(builtins.bitwise.is_some()); assert!(builtins.ec_op.is_some()); assert!(builtins.keccak.is_none()); assert!(builtins.poseidon.is_none()); } cairo-vm/vm/src/types/layout.rs
Lines 472 to 483 in 6182e1f
fn get_all_solidity_instance() { let layout = CairoLayout::all_solidity_instance(); let builtins = BuiltinsInstanceDef::all_solidity(); assert_eq!(layout.name, LayoutName::all_solidity); assert_eq!(layout.rc_units, 8); assert_eq!(layout.builtins, builtins); assert_eq!(layout.public_memory_fraction, 8); assert_eq!( layout.diluted_pool_instance_def, Some(DilutedPoolInstanceDef::default()) ); }
YairVaknin-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.
yeah, they are ready. will post later. I pushed without any tests to see where CI fails and then add all at once.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
6182e1f to
1b8e606
Compare
YairVaknin-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.
Done @JulianGCalderon
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)
vm/src/types/layout.rs line 470 at r1 (raw file):
Previously, YairVaknin-starkware wrote…
yeah, they are ready. will post later. I pushed without any tests to see where CI fails and then add all at once.
Done
vm/src/types/instance_definitions/builtins_instance_def.rs line 401 at r1 (raw file):
Previously, OmriEshhar1 wrote…
add get_builtins_all_cairo_stwo() test?
Done.
1b8e606 to
d2ab417
Compare
JulianGCalderon
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.
LGTM
OmriEshhar1
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.
Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @yuvalsw)
d39f68c to
bdce4f0
Compare
ad19ba2 to
5ec3568
Compare
OmriEshhar1
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.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @yuvalsw)
5ec3568 to
5c8f96f
Compare
yuvalsw
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.
Reviewed 1 of 5 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, and @pefontana)
vm/src/types/instance_definitions/builtins_instance_def.rs line 182 at r7 (raw file):
} pub(crate) fn all_cairo_stwo() -> BuiltinsInstanceDef {
I am not familiar enough with the requirements of the ratios (in relation to one another in a single layout) - does it make sense to just take an existing layout and change some builtins to none?
vm/src/types/instance_definitions/builtins_instance_def.rs line 399 at r7 (raw file):
assert!(builtins.ec_op.is_some()); assert!(builtins.keccak.is_some()); assert!(builtins.poseidon.is_some());
consider adding here the mod_builitins check as in in the all_cairo_stwo test below
vm/src/vm/runners/cairo_runner.rs line 3402 at r7 (raw file):
#[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn run_empty() {
rename to run_empty_all_cairo
5c8f96f to
34d3496
Compare
YairVaknin-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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @yuvalsw)
vm/src/types/instance_definitions/builtins_instance_def.rs line 182 at r7 (raw file):
Previously, yuvalsw wrote…
I am not familiar enough with the requirements of the ratios (in relation to one another in a single layout) - does it make sense to just take an existing layout and change some builtins to none?
Yeah, since it's not a layout used in a stone proof later. We only care about the pointer init of the builtins we support.
yuvalsw
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.
Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
TITLE
Adding
all_cairo_stwolayoutDescription
Adding a layout to the vm that includes all builtins supported by stwo.
Checklist
This change is