chore(blockifier): remove dupl tip() and resource_bounds() in AccountTx

[avivg-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[avivg-starkware]

• chore(blockifier): slight diff impl for AccountTransaction getters #2102
• chore(blockifier): use macro in AccounTx::{sender_address(), tx_hash()} #2109
• chore(blockifier): remove dupl tip() and resource_bounds() in AccountTx #2101 👈 (View in Graphite)
• chore(blockifier): a simple code cleanup in signature_length() #2108
• main

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.43%. Comparing base (e3165c4) to head (855ade9).
Report is 596 commits behind head on main.

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2101       +/-   ##
===========================================
+ Coverage   40.10%   74.43%   +34.33%     
===========================================
  Files          26      129      +103     
  Lines        1895    14904    +13009     
  Branches     1895    14904    +13009     
===========================================
+ Hits          760    11094    +10334     
- Misses       1100     3327     +2227     
- Partials       35      483      +448     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it 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]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.838 ms 35.313 ms 35.877 ms]
change: [+1.3293% +2.7762% +4.4491%] (p = 0.00 < 0.05)
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
1 (1.00%) high mild
9 (9.00%) high severe

[avivg-starkware]

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

---

crates/starknet_api/src/executable_transaction.rs line 113 at r1 (raw file):

                _ => None,
            },
        }

looks like this is already being handled as part of the implementation of tx.tx.resource_bounds() (same for tip)

Code snippet (i):

implement_v3_tx_getters!(
        (resource_bounds, ValidResourceBounds),

Code snippet (ii):

macro_rules! implement_v3_tx_getters {
    ($(($field:ident, $field_type:ty)),*) => {
        $(pub fn $field(&self) -> $field_type {
            match self {
                Self::V3(tx) => tx.$field.clone(),
                _ => panic!("{:?} do not support the field {}; they are only available for V3 transactions.", self.version(), stringify!($field)),
            }
        })*
    };
}

[Yoni-Starkware]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware)

---

crates/starknet_api/src/executable_transaction.rs line 113 at r1 (raw file):

Previously, avivg-starkware wrote…

looks like this is already being handled as part of the implementation of tx.tx.resource_bounds() (same for tip)

What does it mean?

---

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

        })*
    };
}

Is this still needed?

Code quote:

macro_rules! implement_account_tx_inner_getters {
    ($(($field:ident, $field_type:ty)),*) => {
        $(pub fn $field(&self) -> $field_type {
            match &self.tx {
                // TODO(AvivG): Consider moving some of the logic to the Transaction enum.
                Transaction::Declare(tx) => tx.tx.$field().clone(),
                Transaction::DeployAccount(tx) => tx.tx.$field().clone(),
                Transaction::Invoke(tx) => tx.tx.$field().clone(),
            }
        })*
    };
}

[avivg-starkware]

crates/starknet_api/src/executable_transaction.rs line 113 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

What does it mean?

in both funcs above we're making sure that the field 'tx' of the enum AccountTransaction is of version v3.
This 'tx' is either
starknet_api::transaction::InvokeTransaction or
starknet_api::transaction::DeployAccountTransaction or
starknet_api::transaction::DeclareTransaction

However, for each of those, there is a func resource_bounds() which implements the above macro, and thus panicking if version != v3

there was a duplication, as the blockifier's AccountTransaction implementation didn't actually use the api's AccountTransaction's implementation of those funcs, but rather accessed 'directly' the implementation of the starknet_api::transaction.

With that said, the key difference between the 2 implementations I see are: the use of panic vs except (w Option) and clone() vs passing a ref.

Perhaps better to leave things as they were?

[avivg-starkware]

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

Previously, Yoni-Starkware (Yoni) wrote…

Is this still needed?

Yes as all the below funcs are not implemented by 'Transaction' (= starknet_api::executable_transaction::AccountTransaction)

Code snippet:

        (signature, TransactionSignature),
        (nonce, Nonce),
        (nonce_data_availability_mode, DataAvailabilityMode),
        (fee_data_availability_mode, DataAvailabilityMode),
        (paymaster_data, PaymasterData)

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

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

---

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

Previously, avivg-starkware wrote…

Yes as all the below funcs are not implemented by 'Transaction' (= starknet_api::executable_transaction::AccountTransaction)

Can you use implement_tx_getter_calls! instead of implement_account_tx_inner_getters! now?

---

crates/starknet_api/src/executable_transaction.rs line 113 at r1 (raw file):

Previously, avivg-starkware wrote…

in both funcs above we're making sure that the field 'tx' of the enum AccountTransaction is of version v3.
This 'tx' is either
starknet_api::transaction::InvokeTransaction or
starknet_api::transaction::DeployAccountTransaction or
starknet_api::transaction::DeclareTransaction

However, for each of those, there is a func resource_bounds() which implements the above macro, and thus panicking if version != v3

there was a duplication, as the blockifier's AccountTransaction implementation didn't actually use the api's AccountTransaction's implementation of those funcs, but rather accessed 'directly' the implementation of the starknet_api::transaction.

With that said, the key difference between the 2 implementations I see are: the use of panic vs except (w Option) and clone() vs passing a ref.

Perhaps better to leave things as they were?

Thanks!

[Yoni-Starkware]

Reviewable status: 2 of 3 files reviewed, all discussions resolved

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Can you use implement_tx_getter_calls! instead of implement_account_tx_inner_getters! now?

Oh you are doing it in a later PR, so LGTM as long as that future PR gets in :)

[Yoni-Starkware]

Reviewable status: 2 of 3 files reviewed, all discussions resolved

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Oh you are doing it in a later PR, so LGTM as long as that future PR gets in :)

(Should have been part of this PR)

[avivg-starkware]

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

Previously, Yoni-Starkware (Yoni) wrote…

(Should have been part of this PR)

I see, will know for the next :)

[Yoni-Starkware]

Reviewed 1 of 3 files at r1.
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 workflows:

• Started at 2024-11-27T09:41:29Z
• Started at 2024-11-27T09:41:46Z
• Started at 2024-11-27T09:41:47Z
• Started at 2024-11-27T09:42:17Z
• Started at 2024-11-27T09:42:21Z
• Started at 2024-11-27T09:58:53Z
• Started at 2024-11-27T10:18:35Z
• Started at 2024-11-27T10:23:04Z
• Started at 2024-11-27T10:51:13Z
• Started at 2024-11-27T10:51:15Z

[Yoni-Starkware]

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