refactor(blockifier): change return type of invoke_tx deploy_account_tx to account_transaction

[avivg-starkware]

…tx to account_transaction

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.65%. Comparing base (e3165c4) to head (ca4a66d).
Report is 293 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1643       +/-   ##
===========================================
+ Coverage   40.10%   67.65%   +27.54%     
===========================================
  Files          26      102       +76     
  Lines        1895    13868    +11973     
  Branches     1895    13868    +11973     
===========================================
+ Hits          760     9382     +8622     
- Misses       1100     4085     +2985     
- Partials       35      401      +366     

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

[github-actions]

Artifacts upload triggered. View details here

[noaov1]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 238 at r1 (raw file):

    // Prepare transactions
    let account_tx_1 = deploy_account_tx(

Please revert (more informative)
(in the entire file)

Code quote:

 let account_tx_1 = deploy_account_tx(

---

crates/blockifier/src/test_utils/invoke.rs line 30 at r1 (raw file):

        false => AccountTransaction::Invoke(InvokeTransaction::new(invoke_tx, tx_hash)),
    }
}

Suggestion:

    let invoke_tx = match only_query {
        true => InvokeTransaction::new_for_query(invoke_tx, tx_hash),
        false => InvokeTransaction::new(invoke_tx, tx_hash),
    };
    AccountTransaction::Invoke(invoke_tx
}

---

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

            Self::DeployAccount(tx) => tx.constructor_calldata(),
            Self::Invoke(tx) => tx.calldata(),
        }

Please revert. See my comment below.

Code quote:

    pub fn calldata_length(&self) -> usize {
        self.calldata().0.len()
    }

    pub fn calldata(&self) -> Calldata {
        match self {
            Self::Declare(_tx) => calldata![],
            Self::DeployAccount(tx) => tx.constructor_calldata(),
            Self::Invoke(tx) => tx.calldata(),
        }

---

crates/blockifier/src/transaction/test_utils.rs line 141 at r1 (raw file):

) -> (AccountTransaction, ContractAddress) {
    // Deploy an account contract.
    let account_tx = deploy_account_tx(deploy_tx_args, nonce_manager);

Please revert the name

Code quote:

 let account_tx = deploy_account_tx(deploy_tx_args, nonce_manager);

---

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

    // transaction.
    let calldata = Calldata(Arc::clone(&account_tx.calldata().0)); //Aviv how to access?
    let calldata_length = account_tx.calldata_length();

Suggestion:

    let calldata = create_trivial_calldata(test_contract_address);
    let account_tx = invoke_tx(invoke_tx_args! {
        sender_address: account_contract_address,
        calldata
        resource_bounds: default_l1_resource_bounds,
    });

    // Extract invoke transaction fields for testing, as it is consumed when creating an account
    // transaction.
    let calldata_length = account_tx.calldata_length();

[github-actions]

Artifacts upload triggered. View details here

[avivg-starkware]

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @noaov1)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 238 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please revert (more informative)
(in the entire file)

Done.

---

crates/blockifier/src/test_utils/invoke.rs line 30 at r1 (raw file):

        false => AccountTransaction::Invoke(InvokeTransaction::new(invoke_tx, tx_hash)),
    }
}

Done

---

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

Previously, noaov1 (Noa Oved) wrote…

Please revert. See my comment below.

Done.

---

crates/blockifier/src/transaction/test_utils.rs line 141 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please revert the name

Done.

---

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

    // transaction.
    let calldata = Calldata(Arc::clone(&account_tx.calldata().0)); //Aviv how to access?
    let calldata_length = account_tx.calldata_length();

Partially done, please see my fix; As 'calldata' is used twice, it doesn't work without a clone.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewed 1 of 7 files at r1, 2 of 5 files at r3.
Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 235 at r4 (raw file):

            Self::DeployAccount(tx) => tx.constructor_calldata(),
            Self::Invoke(tx) => tx.calldata(),
        };

If we are already here...

Suggestion:

        let calldata = match self {
            Self::Declare(_tx) => return 0,
            Self::DeployAccount(tx) => tx.constructor_calldata(),
            Self::Invoke(tx) => tx.calldata(),
        };

---

crates/blockifier/src/transaction/account_transactions_test.rs line 212 at r4 (raw file):

    let result = account_tx.execute(state, &block_context, enforce_fee, true);
    // Execution should fail if the fee is enforced because the account doesn't have sufficient
    // balance.

Suggestion:

    let deploy_account_tx = deploy_account_tx(
        deploy_account_tx_args! {
            class_hash: account.get_class_hash(),
            max_fee: Fee(if zero_bounds { 0 } else { MAX_FEE.0 }),
            resource_bounds: match gas_bounds_mode {
                GasVectorComputationMode::NoL2Gas => l1_resource_bounds(
                    (if zero_bounds { 0 } else { DEFAULT_L1_GAS_AMOUNT.0 }).into(),
                    DEFAULT_STRK_L1_GAS_PRICE.into()
                ),
                GasVectorComputationMode::All => create_all_resource_bounds(
                    (if zero_bounds { 0 } else { DEFAULT_L1_GAS_AMOUNT.0 }).into(),
                    DEFAULT_STRK_L1_GAS_PRICE.into(),
                    (if zero_bounds { 0 } else { DEFAULT_L2_GAS_MAX_AMOUNT.0 }).into(),
                    DEFAULT_STRK_L2_GAS_PRICE.into(),
                    (if zero_bounds { 0 } else { DEFAULT_L1_DATA_GAS_MAX_AMOUNT.0 }).into(),
                    DEFAULT_STRK_L1_DATA_GAS_PRICE.into(),
                ),
            },
            version,
        },
        &mut NonceManager::default(),
    );

    let enforce_fee = deploy_account_tx.create_tx_info().enforce_fee();
    assert_ne!(zero_bounds, enforce_fee);
    let result = deploy_account_tx.execute(state, &block_context, enforce_fee, true);
    // Execution should fail if the fee is enforced because the account doesn't have sufficient
    // balance.

[ArniStarkware]

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

[ArniStarkware]

crates/blockifier/src/transaction/transactions_test.rs line 1721 at r6 (raw file):

    // Build expected validate call info.
    // TODO (AvivG) Temp implementation until AccountTransaction refactor PR.

Suggestion:

    // TODO(AvivG): When the AccountTransaction refactor is complete, simplify the creation of `validate_calldata` by doing 1.2.3...

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

Reviewed 2 of 4 files at r6.
Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware, @meship-starkware, and @noaov1)

[avivg-starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1721 at r6 (raw file):

    // Build expected validate call info.
    // TODO (AvivG) Temp implementation until AccountTransaction refactor PR.

Done.

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 3 of 4 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[ArniStarkware]

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

[meship-starkware]

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

[avivg-starkware]

• refactor(blockifier): impl .into() from api executable tx to blockifier AccountTransaction #1868
• refactor(blockifier): change return type of invoke_tx deploy_account_tx to account_transaction #1643 👈
• main

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

Join @avivg-starkware and the rest of your teammates on Graphite

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[ArniStarkware]

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

---

crates/blockifier/src/test_utils/deploy_account.rs line 16 at r10 (raw file):

    );

    // TODO(AvivG): use the "new" method.

Delete the todo. It will be addressed in future PR.
Please link in this thread which PR addresses this issue.

(Non-blocking, this todo can be deleted later).

Code quote:

    // TODO(AvivG): use the "new" method.

[meship-starkware]

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

[avivg-starkware]

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

---

crates/blockifier/src/test_utils/deploy_account.rs line 16 at r10 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Delete the todo. It will be addressed in future PR.
Please link in this thread which PR addresses this issue.

(Non-blocking, this todo can be deleted later).

Thanks! the PR is #1688
A short explanation: after this PR, using "new" is longer needed here as we're no longer manually setting only_query to false, but rather this is done in ".into()"