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

test: append block with large declared contract #2054

Merged

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Jul 8, 2024

This change is Reviewable

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.

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


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

    /// Wrapper with pub(crate) visibility for internal (in crate) testing.
    #[cfg(test)]
    #[pyo3(signature = (concurrency_config, general_config, path, max_state_diff_size))]

Is this necessary?

Code quote:

#[pyo3(signature = (concurrency_config, general_config, path, max_state_diff_size))]

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

    #[cfg(test)]
    #[pyo3(signature = (concurrency_config, general_config, path, max_state_diff_size))]
    #[staticmethod]

Same. What does it mean?

Code quote:

#[staticmethod]

Copy link
Collaborator Author

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


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

Previously, Yoni-Starkware (Yoni) wrote…

Is this necessary?

nope, fixed


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

Previously, Yoni-Starkware (Yoni) wrote…

Same. What does it mean?

yup (pyo3 expects the first arg to be of some wrapper type to Self without this)

error[E0277]: the trait bound `PyConcurrencyConfig: TryFrom<&PyCell<PyBlockExecutor>>` is not satisfied
   --> crates/native_blockifier/src/py_block_executor.rs:375:29
    |
375 |         concurrency_config: PyConcurrencyConfig,
    |                             ^^^^^^^^^^^^^^^^^^^ the trait `From<&PyCell<PyBlockExecutor>>` is not implemented for `PyConcurrencyConfig`, which is required by `PyConcurrencyConfig: TryFrom<&PyCell<PyBlockExecutor>>`

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.

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)


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

Previously, dorimedini-starkware wrote…

yup (pyo3 expects the first arg to be of some wrapper type to Self without this)

error[E0277]: the trait bound `PyConcurrencyConfig: TryFrom<&PyCell<PyBlockExecutor>>` is not satisfied
   --> crates/native_blockifier/src/py_block_executor.rs:375:29
    |
375 |         concurrency_config: PyConcurrencyConfig,
    |                             ^^^^^^^^^^^^^^^^^^^ the trait `From<&PyCell<PyBlockExecutor>>` is not implemented for `PyConcurrencyConfig`, which is required by `PyConcurrencyConfig: TryFrom<&PyCell<PyBlockExecutor>>`

Move this to the second impl PyBlockExecutor block

Copy link
Collaborator Author

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


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

Previously, Yoni-Starkware (Yoni) wrote…

Move this to the second impl PyBlockExecutor block

Done.

Signed-off-by: Dori Medini <dori@starkware.co>
Copy link
Collaborator Author

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


crates/native_blockifier/src/state_readers/papyrus_state_test.rs line 126 at r3 (raw file):

            HashMap::from([(PyFelt::from(1_u8), serde_json::to_string(&dep_casm).unwrap())]),
        )
        .unwrap();

why is this done twice? @Yoni-Starkware

Code quote:

    block_executor
        .append_block(
            0,
            None,
            Default::default(),
            Default::default(),
            Default::default(),
            HashMap::from([(PyFelt::from(1_u8), serde_json::to_string(&dep_casm).unwrap())]),
        )
        .unwrap();

    block_executor
        .append_block(
            1,
            Some(PyFelt(Felt::ZERO)),
            PyBlockInfo { block_number: 1, ..PyBlockInfo::default() },
            Default::default(),
            Default::default(),
            HashMap::from([(PyFelt::from(1_u8), serde_json::to_string(&dep_casm).unwrap())]),
        )
        .unwrap();

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.

Reviewed 1 of 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


crates/native_blockifier/src/state_readers/papyrus_state_test.rs line 126 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why is this done twice? @Yoni-Starkware

We read the class in the second append.
The bug was in that read

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:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

@dorimedini-starkware dorimedini-starkware merged commit c954da7 into main-v0.13.2 Jul 9, 2024
11 checks passed
@dorimedini-starkware dorimedini-starkware deleted the dori/large-contract-papyrus-test branch July 9, 2024 07:26
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