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(state, blockifier, starknet_api): rename ContractClass to Co… #2296

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Yoni-Starkware
Copy link
Collaborator

…mpiledClass

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 74.19355% with 24 lines in your changes missing coverage. Please review.

Project coverage is 66.96%. Comparing base (e3165c4) to head (f361784).
Report is 609 commits behind head on main.

Files with missing lines Patch % Lines
crates/blockifier/src/execution/contract_class.rs 28.57% 5 Missing ⚠️
...ve_blockifier/src/state_readers/py_state_reader.rs 0.00% 5 Missing ⚠️
crates/papyrus_state_reader/src/papyrus_state.rs 70.00% 0 Missing and 3 partials ⚠️
crates/starknet_gateway/src/rpc_state_reader.rs 50.00% 2 Missing and 1 partial ⚠️
crates/papyrus_execution/src/execution_utils.rs 33.33% 0 Missing and 2 partials ⚠️
.../blockifier/src/blockifier/transaction_executor.rs 0.00% 1 Missing ⚠️
crates/blockifier/src/bouncer.rs 0.00% 0 Missing and 1 partial ⚠️
.../src/execution/deprecated_entry_point_execution.rs 87.50% 0 Missing and 1 partial ⚠️
crates/blockifier/src/execution/entry_point.rs 83.33% 0 Missing and 1 partial ⚠️
.../blockifier/src/transaction/account_transaction.rs 83.33% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2296       +/-   ##
===========================================
+ Coverage   40.10%   66.96%   +26.85%     
===========================================
  Files          26      233      +207     
  Lines        1895    26434    +24539     
  Branches     1895    26434    +24539     
===========================================
+ Hits          760    17701    +16941     
- Misses       1100     7361     +6261     
- Partials       35     1372     +1337     

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

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from 5a75240 to b117387 Compare November 27, 2024 11:30
@avi-starkware
Copy link
Contributor

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

    /// Initialize a compiled contract class for native.
    ///

lets be consistent with the naming - compiled class everywhere

Suggestion:

    /// Initialize a compiled class for native.
    ///

@avi-starkware
Copy link
Contributor

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

}

// The location where the compiled contract is loaded into memory will not

Suggestion:

// The location where the compiled class is loaded into memory will not

@avi-starkware
Copy link
Contributor

crates/blockifier/src/state/cached_state.rs line 177 at r1 (raw file):

    }

    fn get_compiled_contract_class(

WDYT?

Suggestion:

fn get_compiled_class(

@avi-starkware
Copy link
Contributor

crates/blockifier/src/state/state_api.rs line 43 at r1 (raw file):

    /// Returns the compiled contract class of the given class hash.
    fn get_compiled_contract_class(

?

Suggestion:

    /// Returns the compiled class of the given class hash.
    fn get_compiled_class(

@avi-starkware
Copy link
Contributor

crates/blockifier/src/test_utils/dict_state_reader.rs line 18 at r1 (raw file):

    pub address_to_nonce: HashMap<ContractAddress, Nonce>,
    pub address_to_class_hash: HashMap<ContractAddress, ClassHash>,
    pub class_hash_to_class: HashMap<ClassHash, RunnableCompiledClass>,

Suggestion:

   pub class_hash_to_compiled_class: HashMap<ClassHash, RunnableCompiledClass>,

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from b117387 to 73bbdd5 Compare November 27, 2024 11:58
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: 0 of 30 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @meship-starkware)


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

Previously, avi-starkware wrote…

lets be consistent with the naming - compiled class everywhere

Done.


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

}

// The location where the compiled contract is loaded into memory will not

Done.

@avi-starkware
Copy link
Contributor

crates/papyrus_state_reader/src/papyrus_state.rs line 60 at r1 (raw file):

        if class_is_declared {
            let casm_contract_class = self

Suggestion:

            let casm_compiled_class = self

@avi-starkware
Copy link
Contributor

crates/papyrus_state_reader/src/papyrus_state.rs line 72 at r1 (raw file):

        }

        let v0_contract_class = self

Suggestion:

  let v0_compiled_class = self

@avi-starkware
Copy link
Contributor

crates/papyrus_state_reader/src/papyrus_state.rs line 46 at r1 (raw file):

    /// Returns a V1 contract if found, or a V0 contract if a V1 contract is not
    /// found, or an `Error` otherwise.
    fn get_compiled_contract_class_inner(

Suggestion:

    /// Returns a V1 contract if found, or a V0 contract if a V1 contract is not
    /// found, or an `Error` otherwise.
    fn get_compiled_class_inner(

@avi-starkware
Copy link
Contributor

crates/starknet_gateway/src/rpc_state_reader.rs line 147 at r1 (raw file):

    ) -> StateResult<RunnableCompiledClass> {
        let get_compiled_class_params =
            GetCompiledContractClassParams { class_hash, block_id: self.block_id };

Suggestion:

   GetCompiledClassParams { class_hash, block_id: self.block_id };

@avi-starkware
Copy link
Contributor

crates/papyrus_execution/src/state_reader_test.rs line 191 at r3 (raw file):

    // Test that if we try to get a casm and it's missing, that an error is returned and the field
    // `missing_compiled_class` is set to its hash

Suggestion:

    // Test that an error is returned if we try to get a missing casm, and the field
    // `missing_compiled_class` is set to the missing casm's hash.

Copy link
Contributor

@avi-starkware avi-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 29 of 30 files at r1, 24 of 30 files at r3, all commit messages.
Reviewable status: 38 of 44 files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

@avi-starkware
Copy link
Contributor

crates/papyrus_execution/src/state_reader_test.rs line 238 at r3 (raw file):

    assert_eq!(state_reader2.get_compiled_class(class_hash0).unwrap(), blockifier_casm0);
    assert_eq!(state_reader2.get_compiled_class(class_hash2).unwrap(), blockifier_casm1);
    // Test that if we only got the class without the casm then an error is returned.

Suggestion:

    // Test that an error is returned if we only got the class without the casm.

Copy link
Contributor

@avi-starkware avi-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 6 of 30 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from 73bbdd5 to 00bd007 Compare November 27, 2024 12:21
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: 36 of 45 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @meship-starkware)


crates/starknet_gateway/src/rpc_state_reader.rs line 147 at r1 (raw file):

    ) -> StateResult<RunnableCompiledClass> {
        let get_compiled_class_params =
            GetCompiledContractClassParams { class_hash, block_id: self.block_id };

Done.

Copy link
Contributor

@meship-starkware meship-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 14 of 30 files at r1, 22 of 30 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)


crates/blockifier/src/execution/entry_point.rs line 150 at r4 (raw file):

        // Add class hash to the call, that will appear in the output (call info).
        self.class_hash = Some(class_hash);
        let contract_class = state.get_compiled_class(class_hash)?;

Suggestion:

let compiled_class = state.get_compiled_class(class_hash)?;

crates/blockifier/src/execution/entry_point.rs line 409 at r4 (raw file):

) -> ConstructorEntryPointExecutionResult<CallInfo> {
    // Ensure the class is declared (by reading it).
    let contract_class = state.get_compiled_class(ctor_context.class_hash).map_err(|error| {

Suggestion:

let compiled_class = state.get_compiled_class(ctor_context.class_hash).map_err(|error| {

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from 00bd007 to 9133792 Compare November 27, 2024 13:03
Copy link
Contributor

@meship-starkware meship-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 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch 2 times, most recently from 8454eb5 to fa8f088 Compare November 27, 2024 13:39
@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from fa8f088 to a956f69 Compare November 27, 2024 13:42
@Yoni-Starkware Yoni-Starkware force-pushed the yoni/class/rename-to-compile branch from a956f69 to f361784 Compare November 27, 2024 13:43
Copy link
Contributor

@meship-starkware meship-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 12 of 12 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

Copy link
Contributor

@avi-starkware avi-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 6 of 9 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@Yoni-Starkware Yoni-Starkware merged commit cc5c84b into main Nov 27, 2024
23 checks passed
@Yoni-Starkware Yoni-Starkware deleted the yoni/class/rename-to-compile branch November 27, 2024 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 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.

4 participants