Skip to content

Conversation

@nimrod-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

nimrod-starkware commented Jul 1, 2025

@nimrod-starkware nimrod-starkware self-assigned this Jul 1, 2025
@nimrod-starkware nimrod-starkware marked this pull request as ready for review July 1, 2025 06:58
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

py side:
https://reviewable.io/reviews/starkware-industries/starkware/38487

Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on @amosStarkware and @dorimedini-starkware)

@nimrod-starkware nimrod-starkware changed the base branch from nimrod/python_tests/remove_program_input to graphite-base/7675 July 1, 2025 07:43
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware)


crates/starknet_os/src/tests/bls_field.rs line 128 at r1 (raw file):

        );
    }
}

use rstest's #[values] attribute instead of a for loop

Suggestion:

#[rstest]
fn test_felt_to_bigint3(
    #[values(
        0.into(),
        1.into(),

        DEFAULT_PRIME.clone() - 1,
        DEFAULT_PRIME.clone() - 2,
        BASE.clone() - 1,
        BASE.clone(),
        BASE.pow(2_u32) - 1,
        BASE.pow(2_u32),
        DEFAULT_PRIME.clone() / 2
    )] value: BigInt) {
    let entrypoint_runner_config = get_entrypoint_runner_config();
    let explicit_args: [EndpointArg; 1] = [Felt::from(value.clone()).into()];
    let implicit_args = [ImplicitArg::Builtin(BuiltinName::range_check)];

    let split_value = split_bigint3(value.clone()).unwrap();
    let expected_explicit_args = [EndpointArg::Value(ValueArg::Array(split_value.to_vec()))];
    let n_range_checks = if value == DEFAULT_PRIME.clone() - 1 { 0 } else { 6 };
    let expected_implicit_args: [EndpointArg; 1] = [n_range_checks.into()];

    test_cairo_function(
        &entrypoint_runner_config,
        OS_PROGRAM_BYTES,
        "starkware.starknet.core.os.data_availability.bls_field.felt_to_bigint3",
        &explicit_args,
        &implicit_args,
        &expected_explicit_args,
        &expected_implicit_args,
        HashMap::new(),
    );
}
}

crates/starknet_os/src/test_utils/utils.rs line 0 at r1 (raw file):
code here was only moved from the deleted utils module, right?


crates/starknet_os/Cargo.toml line 19 at r1 (raw file):

[dependencies]
apollo_starknet_os_program = { workspace = true, features = ["test_programs"] }

remove - this shouldn't be part of business logic.
if you need it because the testing feature needs it, you can activate the feature from the testing feature directly:

testing = ["blockifier/testing", "starknet_patricia/testing", "apollo_starknet_os_program/test_programs"]

crates/starknet_os/Cargo.toml line 36 at r1 (raw file):

] }
derive_more.workspace = true
ethnum.workspace = true
  1. make it optional - it's not needed in business logic
  2. activate it in the testing feature:
testing = ["blockifier/testing", "starknet_patricia/testing", "ethnum"]

Suggestion:

ethnum = { workspace = true, optional = true }

crates/starknet_os/Cargo.toml line 45 at r1 (raw file):

papyrus_common.workspace = true
paste.workspace = true
rand.workspace = true

same as ethnum (make optional)

Code quote:

rand.workspace = true

crates/starknet_os/src/lib.rs line 11 at r1 (raw file):

pub mod test_utils;
#[cfg(any(test, feature = "testing"))]
pub(crate) mod tests;
  1. why do you need the testing feature here, and not just cfg(test)? are there test utils in these modules?
  2. will you split this module and move the tests into their respective directories? starknet_os/src/tests is a bit too general for aliases and BLS tests, they should be next to their respective hint impl modules

Code quote:

#[cfg(any(test, feature = "testing"))]
pub(crate) mod tests;

@nimrod-starkware nimrod-starkware changed the base branch from graphite-base/7675 to nimrod/python_tests/remove_program_input July 1, 2025 09:57
@nimrod-starkware nimrod-starkware changed the base branch from nimrod/python_tests/remove_program_input to graphite-base/7675 July 1, 2025 10:43
@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from 60073ba to ab9acfb Compare July 1, 2025 10:43
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a 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, 5 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)


crates/starknet_os/Cargo.toml line 19 at r1 (raw file):

Previously, dorimedini-starkware wrote…

remove - this shouldn't be part of business logic.
if you need it because the testing feature needs it, you can activate the feature from the testing feature directly:

testing = ["blockifier/testing", "starknet_patricia/testing", "apollo_starknet_os_program/test_programs"]

Done.


crates/starknet_os/Cargo.toml line 36 at r1 (raw file):

Previously, dorimedini-starkware wrote…
  1. make it optional - it's not needed in business logic
  2. activate it in the testing feature:
testing = ["blockifier/testing", "starknet_patricia/testing", "ethnum"]

Done.


crates/starknet_os/Cargo.toml line 45 at r1 (raw file):

Previously, dorimedini-starkware wrote…

same as ethnum (make optional)

Done.


crates/starknet_os/src/lib.rs line 11 at r1 (raw file):

Previously, dorimedini-starkware wrote…
  1. why do you need the testing feature here, and not just cfg(test)? are there test utils in these modules?
  2. will you split this module and move the tests into their respective directories? starknet_os/src/tests is a bit too general for aliases and BLS tests, they should be next to their respective hint impl modules
  1. Done
  2. Added todos

crates/starknet_os/src/test_utils/utils.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

code here was only moved from the deleted utils module, right?

yes


crates/starknet_os/src/tests/bls_field.rs line 128 at r1 (raw file):

Previously, dorimedini-starkware wrote…

use rstest's #[values] attribute instead of a for loop

Done.

@nimrod-starkware nimrod-starkware changed the base branch from graphite-base/7675 to main-v0.14.0 July 1, 2025 10:43
@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from ab9acfb to d07b00f Compare July 1, 2025 11:21
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @nimrod-starkware)


crates/starknet_os/src/lib.rs line 11 at r1 (raw file):

Previously, nimrod-starkware wrote…
  1. Done
  2. Added todos

ok, now that this module is only cfg(test), you can delete ethnum and rand from [dependencies] (only needed in [dev-dependencies]); + remove them from the testing list.
also, you can remove apollo_starknet_os_program/test_programs

@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from d07b00f to 8c182da Compare July 1, 2025 13:44
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a 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 @amosStarkware and @dorimedini-starkware)


crates/starknet_os/src/lib.rs line 11 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ok, now that this module is only cfg(test), you can delete ethnum and rand from [dependencies] (only needed in [dev-dependencies]); + remove them from the testing list.
also, you can remove apollo_starknet_os_program/test_programs

Done

@github-actions
Copy link

github-actions bot commented Jul 1, 2025

Benchmark movements: No major performance changes detected.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)

@nimrod-starkware nimrod-starkware force-pushed the nimrod/python_tests/run_from_rust branch from 8c182da to 5202af2 Compare July 1, 2025 14:15
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)

@nimrod-starkware nimrod-starkware added this pull request to the merge queue Jul 1, 2025
Merged via the queue into main-v0.14.0 with commit 32d0226 Jul 1, 2025
26 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants