refactor: streamline class info object to match contract class

[ArniStarkware]

This change is 

[ArniStarkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• refactor: streamline class info object to match contract class #427 👈
• chore: streamline class info constructor #1215
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ArniStarkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 51.85185% with 13 lines in your changes missing coverage. Please review.

Please upload report for BASE (arni/snapi/contract_class_refacrtor/class_info_new_method@85481c0). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/papyrus_execution/src/lib.rs	25.00%	4 Missing and 2 partials ⚠️
crates/native_blockifier/src/py_transaction.rs	0.00%	5 Missing ⚠️
crates/blockifier/src/execution/contract_class.rs	85.71%	2 Missing ⚠️

Additional details and impacted files

@@                                     Coverage Diff                                      @@
##             arni/snapi/contract_class_refacrtor/class_info_new_method     #427   +/-   ##
============================================================================================
  Coverage                                                             ?   62.70%           
============================================================================================
  Files                                                                ?      146           
  Lines                                                                ?    17999           
  Branches                                                             ?    17999           
============================================================================================
  Hits                                                                 ?    11287           
  Misses                                                               ?     5880           
  Partials                                                             ?      832           

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

[yair-starkware]

Reviewed 2 of 5 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/blockifier/src/execution/contract_class.rs line 49 at r3 (raw file):

/// We wrap the actual class in an Arc to avoid cloning the program when cloning the class.
// Note: when deserializing from a SN API class JSON string, the ABI field is ignored
// by serde, since it is not required for execution.

What is this referring to?

Code quote:

/// Represents a runnable Starknet contract class (meaning, the program is runnable by the VM).
/// We wrap the actual class in an Arc to avoid cloning the program when cloning the class.
// Note: when deserializing from a SN API class JSON string, the ABI field is ignored
// by serde, since it is not required for execution.

---

crates/gateway/src/compilation.rs line 3 at r3 (raw file):

use std::sync::Arc;

use blockifier::execution::contract_class::{ClassInfo, ContractClassV1};

Consider using as BlockifierContractClass

Code quote:

ContractClassV1

---

crates/papyrus_execution/src/lib.rs line 760 at r3 (raw file):

            )?);
            let class_info = ClassInfo::new(&class_v0, DEPRECATED_CONTRACT_SIERRA_SIZE, abi_length)
                .map_err(|err| ExecutionError::BadDeclareTransaction {

Is this error still in use?

Code quote:

ExecutionError::BadDeclareTransaction

[ArniStarkware]

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @yair-starkware)

---

crates/blockifier/src/execution/contract_class.rs line 49 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

What is this referring to?

The original intent was to document ContractClass:
https://reviewable.io/reviews/starkware-libs/blockifier/452

Done.

---

crates/gateway/src/compilation.rs line 3 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

Consider using as BlockifierContractClass

Done.

---

crates/papyrus_execution/src/lib.rs line 760 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

Is this error still in use?

No! 🥳
Removed.

[yair-starkware]

Reviewed 2 of 3 files at r4, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @ArniStarkware)

---

crates/blockifier/src/execution/contract_class.rs line 49 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The original intent was to document ContractClass:
https://reviewable.io/reviews/starkware-libs/blockifier/452

Done.

I don't understand how the comment matches the type (where is the Arc?)

[ArniStarkware]

crates/blockifier/src/execution/contract_class.rs line 49 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

I don't understand how the comment matches the type (where is the Arc?)

You are correct.

This issue is now handled in #491. I think these are unrelated issues, and I hope the prep PR will be merged long before this one.

[yair-starkware]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ArniStarkware]

crates/blockifier/src/execution/contract_class.rs line 502 at r5 (raw file):

    V0 { contract_class: ContractClassV0, abi_length: usize },
    V1 { contract_class: ContractClassV1, sierra_program_length: usize, abi_length: usize },
}

Is this cleaner, in your opinion?

Suggestion:

pub enum ClassInfo {
    V0 (ClassInfoV0),
    V1 (ClassInfoV1),
}

pub struct ClassInfoV0 {contract_class: ContractClassV0, abi_length: usize};
pub struct ClassInfoV1 {contract_class: ContractClassV1, sierra_program_length: usize, abi_length: usize };

[yair-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

---

crates/blockifier/src/execution/contract_class.rs line 502 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Is this cleaner, in your opinion?

No

[yair-starkware]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[yair-starkware]

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[yair-starkware]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ArniStarkware]

crates/blockifier/src/execution/contract_class.rs line 565 at r10 (raw file):

    V0 { contract_class: ContractClassV0, abi_length: usize },
    V1 { contract_class: ContractClassV1, sierra_program_length: usize, abi_length: usize },
}

This is the main innovation of this PR.
This, and deleting the combersom new class method.

Code quote:

pub enum ClassInfo {
    V0 { contract_class: ContractClassV0, abi_length: usize },
    V1 { contract_class: ContractClassV1, sierra_program_length: usize, abi_length: usize },
}

[noaov1]

Reviewed 1 of 1 files at r11.
Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @yair-starkware)

---

crates/blockifier/src/execution/contract_class.rs line 565 at r10 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This is the main innovation of this PR.
This, and deleting the combersom new class method.

Using two separate enums (ContractClass and ClassInfo) that mirror each other's structure can be redundant and can make the code harder to maintain, as any changes to ContractClass would need to be reflected in ClassInfo

[ArniStarkware]

crates/blockifier/src/execution/contract_class.rs line 565 at r10 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Using two separate enums (ContractClass and ClassInfo) that mirror each other's structure can be redundant and can make the code harder to maintain, as any changes to ContractClass would need to be reflected in ClassInfo

I split this PR into this one and #1215. PTAL.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.382 ms 34.494 ms 34.647 ms]
change: [-5.9984% -3.5090% -1.3443%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
7 (7.00%) high mild
6 (6.00%) high severe

[ArniStarkware]

No longer relevant. It becomes harder to streamline now that there are three variants of contract class.