Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

refactor(execution, native_blockifier): remove Py objects from Transa… #1427

Conversation

barak-b-starkware
Copy link
Collaborator

@barak-b-starkware barak-b-starkware commented Feb 4, 2024

…ctionExecutor::new


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (586ecbe) 70.45% compared to head (c10f380) 70.38%.

Files Patch % Lines
crates/native_blockifier/src/py_validator.rs 0.00% 9 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs 50.00% 0 Missing and 2 partials ⚠️
crates/native_blockifier/src/py_state_diff.rs 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                               Coverage Diff                               @@
##           barak/block_executor/move_pre_process_block    #1427      +/-   ##
===============================================================================
- Coverage                                        70.45%   70.38%   -0.07%     
===============================================================================
  Files                                               60       60              
  Lines                                             7774     7773       -1     
  Branches                                          7774     7773       -1     
===============================================================================
- Hits                                              5477     5471       -6     
- Misses                                            1866     1871       +5     
  Partials                                           431      431              

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

@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/move_pre_process_block branch from 0c8bc0d to c653a44 Compare February 4, 2024 14:23
@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/new_without_py_objects branch from f06c933 to 18efcca Compare February 4, 2024 14:23
@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/move_pre_process_block branch from c653a44 to 586ecbe Compare February 4, 2024 14:35
@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/new_without_py_objects branch from 18efcca to c10f380 Compare February 4, 2024 14:38
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @Yoni-Starkware)

a discussion (no related file):
This collides with #1387, consider redefining Yael's util, in order to avoid conflicts.


Copy link
Collaborator

@elintul elintul 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 4 files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware and @Yoni-Starkware)

a discussion (no related file):
Python PR?


Yoni-Starkware
Yoni-Starkware previously approved these changes Feb 5, 2024
Copy link
Collaborator

@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.

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware)

@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/move_pre_process_block branch from 586ecbe to d8abfd0 Compare February 5, 2024 10:18
@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/new_without_py_objects branch 2 times, most recently from 327fab2 to 91d6378 Compare February 5, 2024 11:43
Copy link
Collaborator

@elintul elintul 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 r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @barak-b-starkware)


crates/native_blockifier/src/py_block_executor.rs line 335 at r2 (raw file):

}

pub fn into_block_context(

Why is this deleted?

Code quote:

into_block_context

crates/native_blockifier/src/py_state_diff.rs line 167 at r2 (raw file):

            block_timestamp: BlockTimestamp(py_block_info.block_timestamp),
            sequencer_address: ContractAddress::try_from(py_block_info.sequencer_address.0)?,
            gas_prices: GasPrices {

Any chance to make this shorter?
Consider a for loop and shortening each iteration.

@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/new_without_py_objects branch from 91d6378 to 7375293 Compare February 5, 2024 12:23
@barak-b-starkware barak-b-starkware changed the base branch from barak/block_executor/move_pre_process_block to barak/block_executor/transaction_executor_finalize February 5, 2024 12:24
Copy link
Collaborator

@elintul elintul 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: all files reviewed, 4 unresolved discussions (waiting on @barak-b-starkware)

@barak-b-starkware barak-b-starkware force-pushed the barak/block_executor/transaction_executor_finalize branch 6 times, most recently from 6bbb895 to a97ae89 Compare February 6, 2024 06:41
Base automatically changed from barak/block_executor/transaction_executor_finalize to main-v0.13.1 February 6, 2024 06:48
@barak-b-starkware barak-b-starkware dismissed Yoni-Starkware’s stale review February 6, 2024 06:48

The base branch was changed.

gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
… stderr (starkware-libs#1427)

* feat(katana): write katana logs to tempdir and print log file path to stderr

* fix: typo and formatting
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