refactor(transaction): use ClassInfo in Declare transaction

[ayeletstarkware]

This change is 

[ayeletstarkware]

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion

a discussion (no related file):
Python's PR:
https://reviewable.io/reviews/starkware-industries/starkware/33853

---

[elintul]

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @avi-starkware, @ayeletstarkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/transaction/account_transaction.rs line 560 at r1 (raw file):

            self.calldata_length(),
            self.signature_length(),
        );

Suggestion:

        let actual_cost_builder = ActualCostBuilder::new(
            tx_context,
            self.tx_type(),
            self.calldata_length(),
            self.signature_length(),
        );

---

crates/blockifier/src/transaction/transaction_utils.rs line 58 at r1 (raw file):

}

// TODO(Ayelet,01/03/2024): Consider removing this function.

Why?

Code quote:

Consider removing this function.

---

crates/blockifier/src/transaction/transaction_utils.rs line 60 at r1 (raw file):

// TODO(Ayelet,01/03/2024): Consider removing this function.
pub fn verify_contract_class_version(
    contract_class: &ContractClass,

I think it's better, but why was it needed?

Code quote:

contract_class: &ContractClass,

---

crates/blockifier/src/transaction/transaction_utils.rs line 72 at r1 (raw file):

                    cairo_version: 0,
                })
            }

x2
Does it work?

Suggestion:

            if let TransactionVersion::ZERO | TransactionVersion::ONE = declare_version {
                return Ok(())
            }
            
            Err(TransactionExecutionError::ContractClassVersionMismatch {
                declare_version,
                cairo_version: 0,
            })

---

crates/blockifier/src/transaction/transactions.rs line 190 at r1 (raw file):

            starknet_api::transaction::DeclareTransaction::V0(_)
            | starknet_api::transaction::DeclareTransaction::V1(_) => {
                state.set_contract_class(class_hash, self.class_info.contract_class.clone())?;

Can we use the getter above?

Code quote:

self.class_info.contract_class.clone()

---

crates/blockifier/src/transaction/transactions.rs line 206 at r1 (raw file):

                        state.set_contract_class(
                            class_hash,
                            self.class_info.contract_class.clone(),

Same here.

Code quote:

self.class_info.contract_class.clone(),

---

crates/blockifier/src/transaction/transactions_test.rs line 1095 at r1 (raw file):

        sierra_program_length,
        abi_length: 100,
    };

Consider a test util that does all that (with the arguments you need).

Code quote:

    let sierra_program_length = calculate_sierra_program_length_for_testing(tx_version);
    let class_info = ClassInfo {
        contract_class: empty_contract.get_class(),
        sierra_program_length,
        abi_length: 100,
    };

---

crates/native_blockifier/src/py_declare.rs line 133 at r1 (raw file):

    }?;

    let class_info = ClassInfo::try_from(py_class_info)?;

Pass directly.

Code quote:

ClassInfo::try_from(py_class_info)?

---

crates/native_blockifier/src/py_transaction.rs line 159 at r1 (raw file):

        } else {
            ContractClassV1::try_from_json_string(&py_class_info.raw_contract_class)?.into()
        };

Why not use the existing conversion?

Code quote:

        let contract_class: ContractClass = if py_class_info.sierra_program_length == 0 {
            ContractClassV0::try_from_json_string(&py_class_info.raw_contract_class)?.into()
        } else {
            ContractClassV1::try_from_json_string(&py_class_info.raw_contract_class)?.into()
        };

---

crates/native_blockifier/src/py_transaction.rs line 162 at r1 (raw file):

        let sierra_program_length = py_class_info.sierra_program_length;
        let abi_length = py_class_info.abi_length;

Consider passing directly.

Code quote:

        let sierra_program_length = py_class_info.sierra_program_length;
        let abi_length = py_class_info.abi_length;

---

crates/native_blockifier/src/py_validator.rs line 81 at r1 (raw file):

        // so they are skipped here.
        if let AccountTransaction::DeployAccount(_deploy_account_tx) = account_tx {
            let (_tx_execution_info, _py_bouncer_info) = self.execute(tx, None)?;

Give this a name?

Code quote:

None

[ayeletstarkware]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @avi-starkware, @elintul, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/transaction/account_transaction.rs line 560 at r1 (raw file):

            self.calldata_length(),
            self.signature_length(),
        );

Done.

---

crates/blockifier/src/transaction/transaction_utils.rs line 58 at r1 (raw file):

Previously, elintul (Elin) wrote…

Why?

Gilad didn't like it. I'll consult with him. Removed for now.

---

crates/blockifier/src/transaction/transaction_utils.rs line 60 at r1 (raw file):

Previously, elintul (Elin) wrote…

I think it's better, but why was it needed?

In the DeclareTransaction.create function, the verify_contract_class_version previously requested ownership of contract_class and returned it without modifications. This prevents passing class_info.contract_class to Self{} at the end of the function due to a "partially moved value" issue.

---

crates/blockifier/src/transaction/transaction_utils.rs line 72 at r1 (raw file):

Previously, elintul (Elin) wrote…

x2
Does it work?

Yes.

---

crates/blockifier/src/transaction/transactions.rs line 190 at r1 (raw file):

Previously, elintul (Elin) wrote…

Can we use the getter above?

Done.

---

crates/blockifier/src/transaction/transactions.rs line 206 at r1 (raw file):

Previously, elintul (Elin) wrote…

Same here.

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1095 at r1 (raw file):

Previously, elintul (Elin) wrote…

Consider a test util that does all that (with the arguments you need).

Done.

---

crates/native_blockifier/src/py_declare.rs line 133 at r1 (raw file):

Previously, elintul (Elin) wrote…

Pass directly.

Done

---

crates/native_blockifier/src/py_transaction.rs line 159 at r1 (raw file):

Previously, elintul (Elin) wrote…

Why not use the existing conversion?

The existing conversion relies on TransactionVersion. Since From functions can't accept two inputs directly, this solution distinguishes between versions without explicitly receiving the transaction version as an argument.

---

crates/native_blockifier/src/py_transaction.rs line 162 at r1 (raw file):

Previously, elintul (Elin) wrote…

Consider passing directly.

Done

---

crates/native_blockifier/src/py_validator.rs line 81 at r1 (raw file):

Previously, elintul (Elin) wrote…

Give this a name?

Done

[elintul]

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @ayeletstarkware, @noaov1, and @Yoni-Starkware)

---

crates/native_blockifier/src/py_transaction.rs line 159 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The existing conversion relies on TransactionVersion. Since From functions can't accept two inputs directly, this solution distinguishes between versions without explicitly receiving the transaction version as an argument.

A bit flaky, suggested an additional check above.

---

crates/native_blockifier/src/py_transaction.rs line 132 at r2 (raw file):

        TransactionType::Declare => {
            let non_optional_py_class_info: PyClassInfo = optional_py_class_info
                .expect("A class info must be passed in a Declare transaction.");

Please add an additional check here, that version >= 2 == sierra_length != 0 (in case it does not hold, return an appropriate error).

Code quote:

            let non_optional_py_class_info: PyClassInfo = optional_py_class_info
                .expect("A class info must be passed in a Declare transaction.");

---

crates/native_blockifier/src/py_validator.rs line 81 at r2 (raw file):

        // so they are skipped here.
        if let AccountTransaction::DeployAccount(_deploy_account_tx) = account_tx {
            let empty_class_info_for_deploy_account_tx = None;

Suggestion:

sentinel_class_info

[ayeletstarkware]

Reviewable status: 9 of 13 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @elintul, @noaov1, and @Yoni-Starkware)

---

crates/native_blockifier/src/py_transaction.rs line 159 at r1 (raw file):

Previously, elintul (Elin) wrote…

A bit flaky, suggested an additional check above.

Done.

---

crates/native_blockifier/src/py_transaction.rs line 132 at r2 (raw file):

Previously, elintul (Elin) wrote…

Please add an additional check here, that version >= 2 == sierra_length != 0 (in case it does not hold, return an appropriate error).

Is it okay to add it inside py_declare? We already have version parsing from py_tx there.

[ayeletstarkware]

Reviewable status: 9 of 13 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @elintul, @noaov1, and @Yoni-Starkware)

---

crates/native_blockifier/src/py_transaction.rs line 147 at r3 (raw file):

pub struct PyClassInfo {
    pub raw_contract_class: String,
    pub sierra_program_length: usize,

Had to include it because we use sierra_program_length in the version/length check in py_declare.

[elintul]

Reviewed 2 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

[elintul]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

[elintul]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

[codecov-commenter]

Codecov Report

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

Comparison is base (87bc49e) 69.04% compared to head (f4bac0d) 68.98%.

Files	Patch %	Lines
crates/native_blockifier/src/py_transaction.rs	0.00%	23 Missing ⚠️
...es/blockifier/src/transaction/transaction_utils.rs	33.33%	8 Missing ⚠️
crates/native_blockifier/src/py_validator.rs	0.00%	6 Missing ⚠️
...lockifier/src/transaction/transaction_execution.rs	0.00%	5 Missing ⚠️
crates/blockifier/src/transaction/transactions.rs	50.00%	2 Missing and 3 partials ⚠️
crates/native_blockifier/src/py_declare.rs	0.00%	3 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1456      +/-   ##
================================================
- Coverage         69.04%   68.98%   -0.06%     
================================================
  Files                58       58              
  Lines              7949     7965      +16     
  Branches           7949     7965      +16     
================================================
+ Hits               5488     5495       +7     
- Misses             2005     2013       +8     
- Partials            456      457       +1     

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

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)