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

chore(native_blockifier): convert general config to os config #561

Merged

Conversation

amosStarkware
Copy link
Contributor

@amosStarkware amosStarkware commented Aug 22, 2024

This change is Reviewable

@amosStarkware amosStarkware self-assigned this Aug 22, 2024
@amosStarkware amosStarkware force-pushed the amos/convert_general_config_to_os_config branch from 3c2a46d to 380a783 Compare August 22, 2024 09:22
@amosStarkware amosStarkware changed the title chore:(native_blockifier): convert general config to os config chore(native_blockifier): convert general config to os config Aug 22, 2024
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.40%. Comparing base (df672bc) to head (380a783).
Report is 5 commits behind head on main-v0.13.2.

Files Patch % Lines
crates/native_blockifier/src/py_block_executor.rs 71.42% 2 Missing ⚠️
crates/native_blockifier/src/py_validator.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           main-v0.13.2     #561      +/-   ##
================================================
- Coverage         76.41%   76.40%   -0.01%     
================================================
  Files               314      314              
  Lines             34241    34238       -3     
  Branches          34241    34238       -3     
================================================
- Hits              26164    26161       -3     
  Misses             5800     5800              
  Partials           2277     2277              

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

Copy link
Contributor Author

@amosStarkware amosStarkware 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: 0 of 3 files reviewed, 1 unresolved discussion


crates/native_blockifier/src/py_block_executor.rs line 407 at r1 (raw file):

        path: std::path::PathBuf,
        max_state_diff_size: usize,
    ) -> Self {

Turns out this was passed to PyValidator & PyBlockExecutor, just never used.
validate_max_n_steps was also passed PyGeneralConfig and not used (but it was also passed by 'PyVersionedConstantsOverrides' and used. this duplication was not added by me, BTW)

Suggestion:

    pub invoke_tx_max_n_steps: u32

Copy link
Contributor Author

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

+reviewer:@dorimedini-starkware +reviewer:@Yonatan-Starkware

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yonatan-Starkware)

@amosStarkware amosStarkware requested review from Yoni-Starkware and removed request for Yonatan-Starkware August 25, 2024 12:30
Copy link
Contributor Author

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

-reviewer:@Yonatan-Starkware +reviewer:@Yoni-Starkware

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yoni-Starkware)

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


crates/native_blockifier/src/py_block_executor.rs line 407 at r1 (raw file):

Previously, amosStarkware wrote…

Turns out this was passed to PyValidator & PyBlockExecutor, just never used.
validate_max_n_steps was also passed PyGeneralConfig and not used (but it was also passed by 'PyVersionedConstantsOverrides' and used. this duplication was not added by me, BTW)

I don't see these highlighted lines here, what are you referring to?

Copy link
Contributor Author

@amosStarkware amosStarkware 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 @Yoni-Starkware)


crates/native_blockifier/src/py_block_executor.rs line 407 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I don't see these highlighted lines here, what are you referring to?

to the fields PyGeneralConfig::invoke_tx_max_n_steps, PyGeneralConfig::validate_max_n_steps (which I deleted)

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@amosStarkware amosStarkware merged commit ddc8f51 into main-v0.13.2 Aug 26, 2024
25 of 27 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 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.

2 participants