-
Notifications
You must be signed in to change notification settings - Fork 65
feat(apollo_starknet_os_program): add aggregator program + program hash(es) #5937
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 aggregator program + program hash(es) #5937
Conversation
3da610e to
33636e2
Compare
4285720 to
bca7a50
Compare
33636e2 to
b0600fa
Compare
bca7a50 to
d36ae68
Compare
b0600fa to
f79b6de
Compare
d36ae68 to
1cd12cf
Compare
f79b6de to
cf90eea
Compare
3f5e290 to
7cd384d
Compare
2a746c6 to
14f58db
Compare
7cd384d to
6b2700d
Compare
14f58db to
fc32d1d
Compare
6b2700d to
f38164b
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 2 of 6 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)
crates/apollo_starknet_os_program/src/program_hash.rs line 25 at r2 (raw file):
#[derive(Debug, Deserialize, Serialize, PartialEq)] pub struct ProgramHash {
?
Suggestion:
ProgramHashescrates/apollo_starknet_os_program/src/program_hash.rs line 43 at r2 (raw file):
if len > 32 { panic!("Data length exceeds 32 bytes."); }
Consider
Suggestion:
fn pad_to_32_bytes(data: &[u8]) -> Result<[u8; 32], &'static str> { {
let len = data.len();
if len > 32 {
Err("Data length exceeds 32 bytes.")?;
}
let mut padded = [0; 32];crates/apollo_starknet_os_program/src/program_hash.rs line 70 at r2 (raw file):
} else { Ok(Felt::from_bytes_be(&pad_to_32_bytes(&builtin_bytes))) }
Consider sort of this
Suggestion:
Ok(Felt::from_bytes_be(&pad_to_32_bytes(&builtin_bytes).unwrap_or(
Err(ProgramHashError::BuiltinNameTooLong {
builtin: *builtin,
name: builtin.to_str().to_string(),
})?))
crates/apollo_starknet_os_program/src/program_hash_test.rs line 27 at r2 (raw file):
fn test_program_hash() { let AggregatorHash { with_prefix, without_prefix } = compute_aggregator_program_hash().unwrap(); let computed_hash = ProgramHash {
Suggestion:
computed_hashesfc32d1d to
0fdab8f
Compare
f38164b to
5ca04d6
Compare
0fdab8f to
9f1ec6b
Compare
5ca04d6 to
87bc0a2
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: 3 of 7 files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)
crates/apollo_starknet_os_program/src/program_hash.rs line 25 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
?
Done.
crates/apollo_starknet_os_program/src/program_hash.rs line 43 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Consider
this is a test util; so I'll pass
crates/apollo_starknet_os_program/src/program_hash.rs line 70 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Consider sort of this
as this is a test util, I prefer to fail early
crates/apollo_starknet_os_program/src/program_hash_test.rs line 27 at r2 (raw file):
fn test_program_hash() { let AggregatorHash { with_prefix, without_prefix } = compute_aggregator_program_hash().unwrap(); let computed_hash = ProgramHash {
Done.
|
Benchmark movements: No major performance changes detected. |
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 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @nimrod-starkware)
crates/apollo_starknet_os_program/src/program_hash_test.rs line 25 at r3 (raw file):
/// ``` #[test] fn test_program_hash() {
Suggestion:
test_program_hashes9f1ec6b to
c6d8279
Compare
f99e821 to
53d8b0e
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 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)
c6d8279 to
663b3bd
Compare
53d8b0e to
967d6c8
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)
|
Artifacts upload workflows: |
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 r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)
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 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @amosStarkware and @nimrod-starkware)

No description provided.