chore(blockifier): a simple code cleanup in signature_length()

[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
• chore(blockifier): a simple code cleanup in signature_length() #2108 👈 (View in Graphite)
• main

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

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.16%. Comparing base (e3165c4) to head (8b701d7).
Report is 594 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2108       +/-   ##
===========================================
+ Coverage   40.10%   69.16%   +29.05%     
===========================================
  Files          26      108       +82     
  Lines        1895    13866    +11971     
  Branches     1895    13866    +11971     
===========================================
+ Hits          760     9590     +8830     
- Misses       1100     3867     +2767     
- Partials       35      409      +374     

☔ 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

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

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

            Transaction::Declare(tx) => tx.signature(),
            Transaction::DeployAccount(tx) => tx.signature(),
            Transaction::Invoke(tx) => tx.signature(),

Are we cloning the signature here?

Code quote:

        let signature = match &self.tx {
            Transaction::Declare(tx) => tx.signature(),
            Transaction::DeployAccount(tx) => tx.signature(),
            Transaction::Invoke(tx) => tx.signature(),

[avivg-starkware]

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

Previously, Yoni-Starkware (Yoni) wrote…

Are we cloning the signature here?

yes,
before the change by using the following macro.
and After with a slight change:

• In chore(blockifier): remove dupl tip() and resource_bounds() in AccountTx #2101, the macro is copied to starknet_api::executable_transaction (implementing other getters)
• and in chore(blockifier): slight diff impl for AccountTransaction getters #2102, the macro is removed from blockifier::account_transaction, and implemented for 'signature' as well

both through the blockifier and the Api, 'signature' is currently cloned.

with the current implementation it is actually cloned twice, once by the transaction (tx field) and another by the executable transaction.
I can try and change this but this might mean separating to another macro, as it might not be the case for the other implementations of the macro. Would you like me to try and check?

Code snippet (i):

implement_account_tx_inner_getters!(
        (signature, TransactionSignature),
        ...

Code snippet (ii):

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(),
            }
        })*
    };
}

[Yoni-Starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

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

Previously, avivg-starkware wrote…

yes,
before the change by using the following macro.
and After with a slight change:

• In chore(blockifier): remove dupl tip() and resource_bounds() in AccountTx #2101, the macro is copied to starknet_api::executable_transaction (implementing other getters)
• and in chore(blockifier): slight diff impl for AccountTransaction getters #2102, the macro is removed from blockifier::account_transaction, and implemented for 'signature' as well

both through the blockifier and the Api, 'signature' is currently cloned.

with the current implementation it is actually cloned twice, once by the transaction (tx field) and another by the executable transaction.
I can try and change this but this might mean separating to another macro, as it might not be the case for the other implementations of the macro. Would you like me to try and check?

Maybe we should have one additional macro that returns a reference.

[Yoni-Starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Maybe we should have one additional macro that returns a reference.

Let's talk

[avivg-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Let's talk

Added a TODO

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

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