-
Notifications
You must be signed in to change notification settings - Fork 65
feat(starknet_committer_and_os_cli): add command to dump the OS compiled program to file #5876
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
47f7df8 to
b2bea45
Compare
83d6363 to
1dbb071
Compare
b2bea45 to
1c55dd5
Compare
1dbb071 to
d9d9e5f
Compare
1c55dd5 to
c438e17
Compare
d9d9e5f to
8688f92
Compare
c438e17 to
c57c217
Compare
8688f92 to
4061106
Compare
c57c217 to
a939bdc
Compare
4061106 to
97825d7
Compare
a939bdc to
4ed33f4
Compare
97825d7 to
2e6df53
Compare
4ed33f4 to
68b8a50
Compare
a00ac52 to
53fefdc
Compare
2ef9756 to
04364be
Compare
53fefdc to
b977413
Compare
04364be to
5106d5a
Compare
b977413 to
629c819
Compare
5106d5a to
7e34630
Compare
629c819 to
073ef22
Compare
7e34630 to
62086e7
Compare
073ef22 to
f3c678e
Compare
62086e7 to
f26c3f2
Compare
f3c678e to
bcb8b83
Compare
f26c3f2 to
f1e2c5e
Compare
bcb8b83 to
1dd0d61
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 1 of 7 files at r4, 2 of 6 files at r5, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)
.github/workflows/committer_and_os_cli_push.yml line 82 at r7 (raw file):
- name: Build CLI binary run: ./build_native_in_docker.sh rustup toolchain install && cargo build -p starknet_committer_and_os_cli -r --bin starknet_committer_and_os_cli --target-dir CLI_TARGET
Is this included in the install_builld_tools script?
Code quote:
rustup toolchain installcrates/starknet_committer_and_os_cli/src/os_cli/run_os_cli.rs line 48 at r7 (raw file):
#[clap(long, short = 'o', default_value = "stdout")] output_path: String, },
Looks like it needs to follow the same structure as the above (if this is changed in a future PR it's OK).
Code quote:
DumpProgramHash {
/// File path to output.
#[clap(long, short = 'o', default_value = "stdout")]
output_path: String,
},crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 115 at r7 (raw file):
}; // Dumping the `Program` struct won't work - it is not deserializable via cairo-lang's Program // class. JSONify the raw bytes instead.
The first Program is the starknet_api struct (for Cairo 0)? Isn't this struct and the Python class compatible?
Code quote:
// Dumping the `Program` struct won't work - it is not deserializable via cairo-lang's Program
// class. JSONify the raw bytes instead.
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, 3 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)
.github/workflows/committer_and_os_cli_push.yml line 82 at r7 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Is this included in the
install_builld_toolsscript?
in the bootstrap script
crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 115 at r7 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
The first
Programis the starknet_api struct (for Cairo 0)? Isn't this struct and the Python class compatible?
it's the VM struct, and no, it is not.
python->VM works, not the other way around
crates/starknet_committer_and_os_cli/src/os_cli/run_os_cli.rs line 48 at r7 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Looks like it needs to follow the same structure as the above (if this is changed in a future PR it's OK).
WDYM? what is missing?
we don't need a program param when dumping the program hash; we get the entire dict (OS + aggregator)
f1e2c5e to
12ebfab
Compare
1dd0d61 to
3ff8c06
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, 3 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @nimrod-starkware)
crates/starknet_committer_and_os_cli/src/os_cli/run_os_cli.rs line 48 at r7 (raw file):
Previously, dorimedini-starkware wrote…
WDYM? what is missing?
we don't need aprogramparam when dumping the program hash; we get the entire dict (OS + aggregator)
OK, switch to DumpProgramHashes instead. The two commands look equivalent, but the hashes are actually all programs' hashes in one file, and the "program" is for a specific program.
crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 116 at r7 (raw file):
// Dumping the `Program` struct won't work - it is not deserializable via cairo-lang's Program // class. JSONify the raw bytes instead. let os_program_json = serde_json::from_slice::<serde_json::Value>(bytes)
As this can be "any" program
Suggestion:
program_jsonSigned-off-by: Dori Medini <[email protected]>
3ff8c06 to
67bf78b
Compare
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, 2 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)
crates/starknet_committer_and_os_cli/src/os_cli/commands.rs line 116 at r7 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
As this can be "any" program
Done.
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 2 of 2 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)
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 2 of 2 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)

No description provided.