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

fix(blockifier): native can have CairoSteps mode #2272

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

Yoni-Starkware
Copy link
Collaborator

No description provided.

@Yoni-Starkware Yoni-Starkware self-assigned this Nov 25, 2024
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented Nov 25, 2024

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/sierra-gas/fix-tracked-resource branch from 7df6ab8 to 438aebc Compare November 25, 2024 14:36
@Yoni-Starkware Yoni-Starkware changed the title fix(blockifier): Native can have CairoSteps mode fix(blockifier): native can have CairoSteps mode Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.64%. Comparing base (e3165c4) to head (438aebc).
Report is 578 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2272       +/-   ##
===========================================
+ Coverage   40.10%   68.64%   +28.54%     
===========================================
  Files          26      108       +82     
  Lines        1895    13896    +12001     
  Branches     1895    13896    +12001     
===========================================
+ Hits          760     9539     +8779     
- Misses       1100     3948     +2848     
- Partials       35      409      +374     

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

Copy link
Contributor

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


crates/blockifier/src/execution/contract_class.rs line 142 at r1 (raw file):

                Self::V1(contract_class) => contract_class.tracked_resource(min_sierra_version),
                #[cfg(feature = "cairo_native")]
                Self::V1Native(contract_class) => {

What makes contracts be marked as Native in the first place if we have Sierra code for everything and it seems from your change it's unrelated to the Sierra version?

Code quote:

Self::V1Native(contract_class)

Copy link
Collaborator Author

@Yoni-Starkware Yoni-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 @dorimedini-starkware and @TzahiTaub)


crates/blockifier/src/execution/contract_class.rs line 142 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

What makes contracts be marked as Native in the first place if we have Sierra code for everything and it seems from your change it's unrelated to the Sierra version?

It depends on the implementation of the state. It's not a mark, it's a different object.
We, for example, are going to hold Native classes only (state-level), but run native (and not casm) according to your config (execution-level).

Copy link
Collaborator Author

@Yoni-Starkware Yoni-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 @dorimedini-starkware and @TzahiTaub)


crates/blockifier/src/execution/contract_class.rs line 142 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It depends on the implementation of the state. It's not a mark, it's a different object.
We, for example, are going to hold Native classes only (state-level), but run native (and not casm) according to your config (execution-level).

I.e., run SierraGas according to your config, which will enable native.

@Yoni-Starkware Yoni-Starkware merged commit abf5736 into main Nov 26, 2024
13 of 16 checks passed
@Yoni-Starkware Yoni-Starkware deleted the yoni/sierra-gas/fix-tracked-resource branch November 26, 2024 06:43
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
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