fix: fix the deprecated L1_Handler transaction hash computation

[Yael-Starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[codecov]

Codecov Report

Merging #1132 (7223101) into main (b9aecf5) will increase coverage by 0.02%.
The diff coverage is 88.00%.

@@            Coverage Diff             @@
##             main    #1132      +/-   ##
==========================================
+ Coverage   74.19%   74.22%   +0.02%     
==========================================
  Files          73       73              
  Lines        6642     6657      +15     
  Branches     6642     6657      +15     
==========================================
+ Hits         4928     4941      +13     
  Misses        873      873              
- Partials      841      843       +2     

Files Changed	Coverage Δ
crates/papyrus_common/src/transaction_hash.rs	93.67% <88.00%> (-0.45%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[yoavGrs]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @ShahakShama and @Yael-Starkware)

---

crates/papyrus_common/src/transaction_hash.rs line 273 at r1 (raw file):

            L1HandlerVersions::Deprecated,
        )?,
    ];

Consider

Suggestion:

    Ok(vec![
        get_common_l1_handler_transaction_hash(
            transaction,
            chain_id,
            L1HandlerVersions::PreL1Handler,
        )?,
        get_common_l1_handler_transaction_hash(
            transaction,
            chain_id,
            L1HandlerVersions::Deprecated,
        )?,
    ])

---

crates/papyrus_common/src/transaction_hash.rs line 278 at r1 (raw file):

#[derive(PartialEq)]
enum L1HandlerVersions {

Please move the definition up. (maybe right before the first use?)

---

crates/papyrus_common/src/transaction_hash.rs line 284 at r1 (raw file):

}

fn get_common_l1_handler_transaction_hash(

This function is hard to track and will be harder with more hash versions.

Consider one of the following:

1. Splitting the PreL1Handler to a function that doesn't call 'get_common_l1_handler_transaction_hash'.
2. Determine the chain_if conditions by < instead of =.

---

crates/papyrus_common/src/transaction_hash_test.rs line 35 at r1 (raw file):

#[derive(Deserialize, Serialize)]
struct TransactionWithHash {

Rename it?

[ShahakShama]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Yael-Starkware and @yoavGrs)

---

crates/papyrus_common/src/transaction_hash.rs line 279 at r1 (raw file):

#[derive(PartialEq)]
enum L1HandlerVersions {
    PreL1Handler,

Consider AsInvoke instead of PreL1Handler

---

crates/papyrus_common/src/transaction_hash.rs line 280 at r1 (raw file):

enum L1HandlerVersions {
    PreL1Handler,
    Deprecated,

Deprecated is now not a good name. find another name for it

---

crates/papyrus_common/src/transaction_hash.rs line 284 at r1 (raw file):

Previously, yoavGrs wrote…

This function is hard to track and will be harder with more hash versions.

Consider one of the following:

1. Splitting the PreL1Handler to a function that doesn't call 'get_common_l1_handler_transaction_hash'.
2. Determine the chain_if conditions by < instead of =.

3. Whenever there's a branch, stop the chain and store it in a mid variable and do a match to determine what's the next value you should chain

---

crates/papyrus_common/src/transaction_hash_test.rs line 35 at r1 (raw file):

Previously, yoavGrs wrote…

Rename it?

Possible name, TransactionTestData

[Yael-Starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama and @yoavGrs)

---

crates/papyrus_common/src/transaction_hash.rs line 273 at r1 (raw file):

Previously, yoavGrs wrote…

Consider

done

---

crates/papyrus_common/src/transaction_hash.rs line 278 at r1 (raw file):

Previously, yoavGrs wrote…

Please move the definition up. (maybe right before the first use?)

done

---

crates/papyrus_common/src/transaction_hash.rs line 279 at r1 (raw file):

Previously, ShahakShama wrote…

Consider AsInvoke instead of PreL1Handler

done

---

crates/papyrus_common/src/transaction_hash.rs line 280 at r1 (raw file):

Previously, ShahakShama wrote…

Deprecated is now not a good name. find another name for it

done

---

crates/papyrus_common/src/transaction_hash.rs line 284 at r1 (raw file):

Previously, ShahakShama wrote…

3. Whenever there's a branch, stop the chain and store it in a mid variable and do a match to determine what's the next value you should chain

done

---

crates/papyrus_common/src/transaction_hash_test.rs line 35 at r1 (raw file):

Previously, ShahakShama wrote…

Possible name, TransactionTestData

done

[ShahakShama]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yoavGrs)

[yoavGrs]

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

---

crates/papyrus_common/src/transaction_hash.rs line 285 at r2 (raw file):

) -> Result<StarkHash, StarknetApiError> {
    Ok(PedersenHashChain::new()
        .chain_if(&L1_HANDLER, version > L1HandlerVersions::AsInvoke)

WDYT about a chain_if_else function, for switching between two options by a condition?

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

[Yael-Starkware]

Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @ShahakShama and @yoavGrs)

---

crates/papyrus_common/src/transaction_hash.rs line 285 at r2 (raw file):

Previously, yoavGrs wrote…

WDYT about a chain_if_else function, for switching between two options by a condition?

done

[yoavGrs]

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