chore(blockifier): impl traits Executable & TxInfoCreator for api_executabe_txs

[avivg-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 185 lines in your changes missing coverage. Please review.

Project coverage is 75.57%. Comparing base (e3165c4) to head (758f49b).
Report is 286 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/transaction/transactions.rs	0.00%	182 Missing ⚠️
crates/starknet_api/src/executable_transaction.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1279       +/-   ##
===========================================
+ Coverage   40.10%   75.57%   +35.47%     
===========================================
  Files          26      102       +76     
  Lines        1895    13872    +11977     
  Branches     1895    13872    +11977     
===========================================
+ Hits          760    10484     +9724     
- Misses       1100     2932     +1832     
- Partials       35      456      +421     

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

[meship-starkware]

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

---

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

                    // In the past, we allowed redeclaration of Cairo 0 contracts since there was
                    // no class commitment (so no need to check if the class is already declared).
                    state.set_contract_class(class_hash, contract_class(self)?)?;

Is this semi-column intentional? Also, it seems like all the functions are returning None upon success, so in which case are we returning CallInfo? Is it for future use?

---

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

                    max_fee: tx.max_fee,
                })
            }

Do they need to be in different cases because of the compiled class hash? It seems to me like it should work.

Suggestion:

            starknet_api::transaction::DeclareTransaction::V0(tx)
            | starknet_api::transaction::DeclareTransaction::V1(tx) | 
            starknet_api::transaction::DeclareTransaction::V2(tx)  => {
                TransactionInfo::Deprecated(DeprecatedTransactionInfo {
                    common_fields,
                    max_fee: tx.max_fee,
                })
            }

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Is this semi-column intentional? Also, it seems like all the functions are returning None upon success, so in which case are we returning CallInfo? Is it for future use?

Never mind, I saw that it is part of this function API. Only the semicolon part is relevant

[meship-starkware]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[meship-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @noaov1)

[noaov1]

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

---

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

    DeployAccountTransaction as StarknetApiDeployAccountTx,
    InvokeTransaction as StarknetApiInvokeTx,
};

Suggestion:

use starknet_api::executable_transaction::{
    DeclareTransaction as ExecutableDeclareTransaction,
    DeployAccountTransaction as ExecutableDeployAccountTx,
    InvokeTransaction as ExecutableInvokeTx,
};

---

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

            nonce: self.nonce(),
            sender_address: self.sender_address(),
            only_query: false, //QUESTION(AvivG): should get only_query from some source?

That is an interesting question. We can set it here to false and than override it by the account transaction that called this method.

Code quote:

            only_query: false, //QUESTION(AvivG): should get only_query from some source?

[avivg-starkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

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

    DeployAccountTransaction as StarknetApiDeployAccountTx,
    InvokeTransaction as StarknetApiInvokeTx,
};

for consistency I've changed all to end with 'Tx' instead of 'Transaction'. Is there a reason you think 'ExecutableDeclareTransaction' should be different? or to have them all end with 'Transaction'?

---

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

Previously, noaov1 (Noa Oved) wrote…

That is an interesting question. We can set it here to false and than override it by the account transaction that called this method.

OK thank you. I left a temporary comment for myself to be removed before merging.

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Do they need to be in different cases because of the compiled class hash? It seems to me like it should work.

'tx' in the first two is of type 'DeclareTransactionV0V1' and in the latter it is 'DeclareTransactionV2', because it is used to extract max_fee, it must be separated

[github-actions]

Artifacts upload triggered. View details here

[noaov1]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)

---

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

Previously, avivg-starkware wrote…

for consistency I've changed all to end with 'Tx' instead of 'Transaction'. Is there a reason you think 'ExecutableDeclareTransaction' should be different? or to have them all end with 'Transaction'?

No, I accidentally wrote it differently.

[meship-starkware]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.642 ms 29.689 ms 29.740 ms]
change: [-2.2144% -1.9735% -1.7229%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

[ArniStarkware]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 715 at r5 (raw file):

    ) -> TransactionExecutionResult<Option<CallInfo>> {
        let class_hash = self.class_hash();
        let ctor_context = ConstructorContext {

Avoid abbreviations.

Suggestion:

let constructor_context = ConstructorContext {

---

crates/blockifier/src/transaction/transactions.rs line 852 at r5 (raw file):

    class_hash: ClassHash,
    compiled_class_hash: Option<CompiledClassHash>,
) -> TransactionExecutionResult<()> {

Document.

Suggestion:

/// Tries declaring a contract class i.e. setting the contract class at the state at the given class hash.
fn try_declare<S: State>(
    tx: &ExecutableDeclareTx,
    state: &mut S,
    class_hash: ClassHash,
    compiled_class_hash: Option<CompiledClassHash>,
) -> TransactionExecutionResult<()> {

---

crates/blockifier/src/transaction/transactions.rs line 852 at r5 (raw file):

    class_hash: ClassHash,
    compiled_class_hash: Option<CompiledClassHash>,
) -> TransactionExecutionResult<()> {

see that for the Declare struct in this file - try_declare is a method. There is code duplication here. Add a todo to delete it there (Or, can you please link the PR where the code duplication is resolved?)

Also, please do the same for other code duplications around this PR.

Code quote:

fn try_declare<S: State>(
    tx: &ExecutableDeclareTx,
    state: &mut S,
    class_hash: ClassHash,
    compiled_class_hash: Option<CompiledClassHash>,
) -> TransactionExecutionResult<()> {

---

crates/blockifier/src/transaction/transactions.rs line 872 at r5 (raw file):

fn contract_class(tx: &ExecutableDeclareTx) -> ContractClass {
    tx.class_info.contract_class.clone()
}

Can't this be a getter method of ExecutableDeclareTx? (i.e. update the code in Starknet API).

Code quote:

fn contract_class(tx: &ExecutableDeclareTx) -> ContractClass {
    tx.class_info.contract_class.clone()
}

---

crates/starknet_api/src/executable_transaction.rs line 232 at r5 (raw file):

        (signature, TransactionSignature),
        (version, TransactionVersion)
    );

TL;DR" move to top of impl block.

Align the order of calls with other impl blocks.

For DeployAccountTransaction and InvokeTransaction it is:

1. implement_inner_tx_getter_calls,
2. pub fn tx(&self) (no need to add this function if it is never used).
3. create
4. from_rpc_tx (Irrelevant for declare - long story).

Code quote:

    implement_inner_tx_getter_calls!(
        (class_hash, ClassHash),
        (nonce, Nonce),
        (sender_address, ContractAddress),
        (signature, TransactionSignature),
        (version, TransactionVersion)
    );

[avivg-starkware]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 715 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Avoid abbreviations.

Done.

---

crates/blockifier/src/transaction/transactions.rs line 852 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Document.

Done.

---

crates/blockifier/src/transaction/transactions.rs line 852 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

see that for the Declare struct in this file - try_declare is a method. There is code duplication here. Add a todo to delete it there (Or, can you please link the PR where the code duplication is resolved?)

Also, please do the same for other code duplications around this PR.

Yes, it is removed together with "DeclareTransaction" entire implementation in the following PR:
#1831

---

crates/blockifier/src/transaction/transactions.rs line 872 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Can't this be a getter method of ExecutableDeclareTx? (i.e. update the code in Starknet API).

It can. See change

---

crates/starknet_api/src/executable_transaction.rs line 232 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

TL;DR" move to top of impl block.

Align the order of calls with other impl blocks.

For DeployAccountTransaction and InvokeTransaction it is:

1. implement_inner_tx_getter_calls,
2. pub fn tx(&self) (no need to add this function if it is never used).
3. create
4. from_rpc_tx (Irrelevant for declare - long story).

Done.

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

crates/blockifier/src/transaction/transactions.rs line 858 at r6 (raw file):

        Err(StateError::UndeclaredClassHash(_)) => {
            // Class is undeclared; declare it.
            state.set_contract_class(class_hash, tx.contract_class().try_into()?)?;

Note to self. This is where the magic happens!
Nice.

Code quote:

contract_class().try_into()

[ArniStarkware]

Reviewed 1 of 2 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 848 at r6 (raw file):

/// Tries declaring a contract class i.e. setting the contract class at the state at the given class
/// hash.

ChatGPT suggests:

Suggestion:

/// Attempts to declare a contract class by setting the contract class in the state with the specified class hash.

[avivg-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 848 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

ChatGPT suggests:

Done.

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

[ArniStarkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @noaov1)

[meship-starkware]

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

[noaov1]

Reviewed 1 of 2 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[meship-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

[meship-starkware]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)