Skip to content
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

refactor(starknet_integration_tests): add sierra contract class to dummy state #2467

Conversation

AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

AvivYossef-starkware commented Dec 5, 2024

@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review December 5, 2024 09:48
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from 833b635 to 13e4acf Compare December 5, 2024 09:52
@AvivYossef-starkware AvivYossef-starkware changed the title refactor(starknet_integration_test): add sierra contract class to dummy state refactor(starknet_integration_tests): add sierra contract class to dummy state Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.94%. Comparing base (e3165c4) to head (511eafb).
Report is 739 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
+ Coverage   40.10%   48.94%   +8.84%     
==========================================
  Files          26      290     +264     
  Lines        1895    33112   +31217     
  Branches     1895    33112   +31217     
==========================================
+ Hits          760    16208   +15448     
- Misses       1100    15803   +14703     
- Partials       35     1101    +1066     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @Yoni-Starkware)


a discussion (no related file):
please add reviewer(s) from mempool team


crates/starknet_integration_tests/src/state_reader.rs line 171 at r1 (raw file):

        }
    }
    sierra_contract_classes

use filter_map

Suggestion:

    contract_classes_to_retrieve
        .iter()
        .filter_map(|contract| {
            if contract.cairo_version() == CairoVersion::Cairo1 {
                Some((contract.class_hash(), contract.sierra()))
            } else {
                None
            }
        })
        .collect()

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/get_sn_api_sierra_from_feature_contract branch from 7045fbb to cff63ab Compare December 5, 2024 10:53
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch 2 times, most recently from 566536c to 110372e Compare December 5, 2024 11:04
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


a discussion (no related file):

Previously, dorimedini-starkware wrote…

please add reviewer(s) from mempool team

I spoke with Dan before opening the PR, and he said it wasn’t necessary.


crates/starknet_integration_tests/src/state_reader.rs line 171 at r1 (raw file):

Previously, dorimedini-starkware wrote…

use filter_map

Done.

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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @Yoni-Starkware)


crates/starknet_integration_tests/src/state_reader.rs line 171 at r2 (raw file):

            } else {
                None
            }

sorry, missed this earlier.
please use match, we currently have another version variant

Suggestion:

            match contract.cairo_version() {
                CairoVersion::Cairo0 => None,
                CairoVersion::Cairo1 => Some((contract.class_hash(), contract.sierra())),
                #[cfg(feature = "cairo_native")]
                CairoVersion::Native => Some((contract.class_hash(), contract.sierra())),                
            }

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/get_sn_api_sierra_from_feature_contract branch from cff63ab to 3efd03b Compare December 5, 2024 13:05
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from 110372e to f9b3362 Compare December 5, 2024 13:05
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.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/get_sn_api_sierra_from_feature_contract branch from 3efd03b to e4ce2e7 Compare December 5, 2024 14:02
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from f9b3362 to b383741 Compare December 5, 2024 14:02
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 1 of 3 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/starknet_integration_tests/src/state_reader.rs line 171 at r2 (raw file):

Previously, dorimedini-starkware wrote…

sorry, missed this earlier.
please use match, we currently have another version variant

Done.

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/get_sn_api_sierra_from_feature_contract branch from e4ce2e7 to e85b600 Compare December 5, 2024 14:31
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from b383741 to f5062e4 Compare December 5, 2024 14:31
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 r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils.rs line 103 at r5 (raw file):

    pub fn is_cairo0(&self) -> bool {
        matches!(self, Self::Cairo0)
    }

here, you do have the feature, so use full match

Suggestion:

    pub fn is_cairo0(&self) -> bool {
        match self {
            Self::Cairo0 => true,
            Self::Cairo1 => false,
            #[cfg(feature = "cairo_native")]
            Self::Native => false,
        }
    }

crates/starknet_integration_tests/src/state_reader.rs line 169 at r5 (raw file):

    contract_classes_to_retrieve
        .filter(|contract| !contract.cairo_version().is_cairo0())
        .map(|contract| (contract.class_hash(), contract.sierra()))

non-blocking, but filter+map should probably just be filter_map

Suggestion:

        .filter_map(|contract| if !contract.cairo_version().is_cairo0() {
            Some((contract.class_hash(), contract.sierra()))
        } else { None })

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/get_sn_api_sierra_from_feature_contract branch from e85b600 to cb2c53d Compare December 8, 2024 09:17
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from f5062e4 to d25e648 Compare December 8, 2024 09:17
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils.rs line 103 at r5 (raw file):

Previously, dorimedini-starkware wrote…

here, you do have the feature, so use full match

Done.


crates/starknet_integration_tests/src/state_reader.rs line 169 at r5 (raw file):

Previously, dorimedini-starkware wrote…

non-blocking, but filter+map should probably just be filter_map

Don’t you think this makes things clearer? I believe that using filter_map is a good option when the closure in the map returns an Option, as it clearly indicates what is being filtered. However, that isn’t the case for us. The logic behind the filter and the logic of the map are unrelated.

@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/get_sn_api_sierra_from_feature_contract to graphite-base/2467 December 8, 2024 09:35
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from d25e648 to 24c6353 Compare December 8, 2024 09:36
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/2467 to main December 8, 2024 09:36
@AvivYossef-starkware AvivYossef-starkware force-pushed the 12-05-refactor_starknet_integration_test_add_sierra_contract_class_to_dummy_state branch from 24c6353 to 511eafb Compare December 8, 2024 09:36
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 r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@AvivYossef-starkware AvivYossef-starkware merged commit 8f210dc into main Dec 8, 2024
13 checks passed
Copy link
Contributor Author

Merge activity

  • Dec 8, 6:07 AM EST: A user merged this pull request with Graphite.

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.

3 participants