-
Notifications
You must be signed in to change notification settings - Fork 65
feat(apollo_starknet_os_program): add test contracts, add dumper in CLI #5946
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
feat(apollo_starknet_os_program): add test contracts, add dumper in CLI #5946
Conversation
b5385b4 to
622d12c
Compare
995428a to
3d599de
Compare
622d12c to
a136d27
Compare
3d599de to
0f5874b
Compare
a136d27 to
32cb04d
Compare
0f5874b to
69ea9d5
Compare
32cb04d to
aff8f88
Compare
69ea9d5 to
3198f53
Compare
aff8f88 to
e1bcc4f
Compare
3198f53 to
26f58f6
Compare
e1bcc4f to
192afa4
Compare
26f58f6 to
d655d2d
Compare
192afa4 to
08b00d4
Compare
d655d2d to
70cd54e
Compare
08b00d4 to
3adf43f
Compare
70cd54e to
d80cbe1
Compare
3adf43f to
a395b96
Compare
d80cbe1 to
beff0f1
Compare
a395b96 to
16eb004
Compare
beff0f1 to
612f5d4
Compare
16eb004 to
3dd6a3e
Compare
612f5d4 to
19a68f0
Compare
3dd6a3e to
87dbe9e
Compare
19a68f0 to
d3a1ffa
Compare
7cb661d to
9eba377
Compare
42908ca to
e05feb2
Compare
9eba377 to
0ae3e91
Compare
e05feb2 to
49c095f
Compare
0ae3e91 to
0e13d04
Compare
49c095f to
34f9837
Compare
0e13d04 to
8558852
Compare
34f9837 to
cb73d6f
Compare
8558852 to
9f22c4b
Compare
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.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)
crates/apollo_starknet_os_program/build/compile_program.rs line 43 at r2 (raw file):
task_set.spawn(compile_and_output_program( out_dir, "starkware/starknet/core/os/state/aliases_test.cairo",
Const. And later on - an enum similar to the blockifier's FeatureContract.
Code quote:
"starkware/starknet/core/os/state/aliases_test.cairo",crates/apollo_starknet_os_program/src/lib.rs line 13 at r2 (raw file):
pub mod program_hash; #[cfg(feature = "test_programs")] pub mod test_programs;
Is this needed?
Code quote:
#[cfg(feature = "test_programs")]
pub mod test_programs;crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 119 at r2 (raw file):
let bytes = match program { ProgramToDump::Aggregator => AGGREGATOR_PROGRAM_BYTES, ProgramToDump::AliasesTest => ALIASES_TEST_BYTES,
Possibly condition, see the comment below.
Code quote:
ProgramToDump::AliasesTest => ALIASES_TEST_BYTES,crates/starknet_committer_and_os_cli/src/os_cli/run_os_cli.rs line 31 at r2 (raw file):
pub enum ProgramToDump { Aggregator, AliasesTest,
Shoudn't this be under #[cfg(feature = "test_programs")] as well?
If you only want to be able to recompile with this feature, but dump without it - maybe change the name of the feature. Otherwise, add the condition.
dorimedini-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, 4 unresolved discussions (waiting on @TzahiTaub)
crates/apollo_starknet_os_program/build/compile_program.rs line 43 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Const. And later on - an enum similar to the blockifier's FeatureContract.
see comment in previous PR - same question (this is still the build script)
crates/apollo_starknet_os_program/src/lib.rs line 13 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Is this needed?
the CLI crate includes an option to dump the aliases_test.cairo compiled program, which I will later use to remove the cairo code from the main repo
crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 119 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Possibly condition, see the comment below.
see below
crates/starknet_committer_and_os_cli/src/os_cli/run_os_cli.rs line 31 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Shoudn't this be under
#[cfg(feature = "test_programs")]as well?
If you only want to be able to recompile with this feature, but dump without it - maybe change the name of the feature. Otherwise, add the condition.
the CLI crate activates the test_programs feature of the OS program crate,
but no reason for the CLI crate to have this feature - it's "always on"
cb73d6f to
db02f56
Compare
9f22c4b to
03b37e8
Compare
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/apollo_starknet_os_program/build/main.rs line 18 at r2 (raw file):
let mut task_set = tokio::task::JoinSet::new(); #[cfg(feature = "test_programs")]
I'm not sure how much the recompile-if of build scripts can be configured, but if you know how to bind the test_program feature with the test contracts (i.e., that if only a test contract was changed and we don't use the test_program feature - we won't run this scripts) - please do.
Code quote:
#[cfg(feature = "test_programs")]
dorimedini-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 @TzahiTaub)
crates/apollo_starknet_os_program/build/main.rs line 18 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
I'm not sure how much the recompile-if of build scripts can be configured, but if you know how to bind the
test_programfeature with the test contracts (i.e., that if only a test contract was changed and we don't use thetest_programfeature - we won't run this scripts) - please do.
IDK how to do that, and I think it's overkill - if you make a change to the test contract, you should be using the feature, or you are not testing your changes
03b37e8 to
6161f7d
Compare
Merge activity
|
dorimedini-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.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

No description provided.