refactor(transaction): add common getters to transaction

[meship-starkware]

This change is 

[meship-starkware]

@tdelabro I am not sure this is what you had in mind
please LMK if you have any comments

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @noaov1)

[tdelabro]

Yup this is what I ment.
There may be other ones you could add, have you checked for every fields?

[meship-starkware]

In the case of the blockier transaction, the only other common field they have is the tx, but everyone is of a different type
So I cannot write a getter for it. In the case of the Transaction object in the starknet API they do have more common fields but they are divided by versions and already have some common getters for some of the fields.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @tdelabro)

---

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

Previously, tdelabro (Timothée Delabrouille) wrote…

Don't name getters methods using the get_ prefix

https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

Done.

---

crates/blockifier/src/transaction/transaction_execution.rs line 101 at r1 (raw file):

Previously, tdelabro (Timothée Delabrouille) wrote…

same

Done.

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @tdelabro)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Done.

Someone already added this getter to the account transaction, so I only needed to add it to transaction_execution.
Also, @ArniStarkware returns TransactionHash inside of &TransactionHash, so I decided to go with it
LMK if it might cause a problem.

sequencer/crates/blockifier/src/transaction/account_transaction.rs

Line 648 in 7082e88

pub fn tx_hash(&self) -> TransactionHash {

[ArniStarkware]

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

Previously, meship-starkware (Meshi Peled) wrote…

Someone already added this getter to the account transaction, so I only needed to add it to transaction_execution.
Also, @ArniStarkware returns TransactionHash inside of &TransactionHash, so I decided to go with it
LMK if it might cause a problem.

sequencer/crates/blockifier/src/transaction/account_transaction.rs

Line 648 in 7082e88

pub fn tx_hash(&self) -> TransactionHash {

*instead of?

[ArniStarkware]

-- commits line 2 at r2:

Suggestion:

- abb33f7: refactor(blockifier): add getter for tx_hash to transaction

[ArniStarkware]

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

---

-- commits line 2 at r2:
Make sure the merged commit message has no typo.

Code quote:

tramsaction

[meship-starkware]

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

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

*instead of?

In the end, I changed my getter to return the object instead of a reference. So the question was for @tdelabro if it fits his need

[ArniStarkware]

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

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @tdelabro)

[noaov1]

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

[tdelabro]

already have some common getters for some of the fields

Yeah that was what I was referring to. The issue is in the "some".
Those getters have been added erratically, when someone in the Starkware team needed them, without a concern for exhaustivity.
This PR could solve this by checking that every common field is covered by a getter

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

[ArniStarkware]

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

[codecov]

Codecov Report

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

Project coverage is 69.89%. Comparing base (b0cfe82) to head (836b981).
Report is 106 commits behind head on main.

Files with missing lines	Patch %	Lines
...lockifier/src/transaction/transaction_execution.rs	0.00%	15 Missing ⚠️
.../blockifier/src/transaction/account_transaction.rs	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
- Coverage   74.18%   69.89%   -4.30%     
==========================================
  Files         359       87     -272     
  Lines       36240    11246   -24994     
  Branches    36240    11246   -24994     
==========================================
- Hits        26886     7860   -19026     
+ Misses       7220     2998    -4222     
+ Partials     2134      388    -1746     

Flag	Coverage Δ
	69.89% <0.00%> (-4.30%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[meship-starkware]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @tdelabro)

---

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

        (max_fee, Option<Fee>)
    );

I'm not sure about the option here. It might be better to return an error or panic, as I did with the only V3 case.
Just to be clear, I think that all getters who do not have all options need to act the same. When we decide on a better solution, I will refactor all of them to act the same.

---

crates/starknet_api/src/transaction.rs line 299 at r4 (raw file):

        }
    }

I am not sure this getter is needed, but all the other fields had one, so I added this.

---

crates/starknet_api/src/transaction.rs line 411 at r4 (raw file):

        $(pub fn $field(&self) -> $field_type {
            match self {
                Self::V3(tx) => tx.$field.clone(),

I am not sure the panic here is a good thing. It does make the code cleaner (no need to wrap in result or option), but I think it better to let the user decide how to handle errors this way and not panic if he accidentally asks a field that does not exist for the wanted version. WDYT?

[ArniStarkware]

Reviewed 1 of 4 files at r4.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @meship-starkware, @noaov1, and @tdelabro)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I'm not sure about the option here. It might be better to return an error or panic, as I did with the only V3 case.
Just to be clear, I think that all getters who do not have all options need to act the same. When we decide on a better solution, I will refactor all of them to act the same.

Maybe return default class hash.

---

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

            Self::Invoke(tx) => Some(tx.tx.sender_address()),
        }
    }

Deploy account has the sender address as a member of the transaction on an external layer.

Also - make sure we don't have both getters for contract address and sender address.

Suggestion:

    pub fn sender_address(&self) -> ContractAddress {
        match self {
            Self::Declare(tx) => tx.tx.sender_address(),
            Self::DeployAccount(tx) => tx.contract_address(),
            Self::Invoke(tx) => tx.tx.sender_address(),
        }
    }

---

crates/starknet_api/src/executable_transaction.rs line 160 at r4 (raw file):

        (fee_data_availability_mode, DataAvailabilityMode),
        (paymaster_data, PaymasterData),
        (max_fee, Option<Fee>)

I don't like the fact that executable transaction can return max fee.

the executable transaction should be a creature of v3 transaction only (I know it is not, this is the ideal).

Code quote:

        (max_fee, Option<Fee>)

---

crates/starknet_api/src/transaction.rs line 299 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure this getter is needed, but all the other fields had one, so I added this.

I sent you a DM about it.

---

crates/starknet_api/src/transaction.rs line 411 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure the panic here is a good thing. It does make the code cleaner (no need to wrap in result or option), but I think it better to let the user decide how to handle errors this way and not panic if he accidentally asks a field that does not exist for the wanted version. WDYT?

Maybe you can return the default, but I don't like that either...

[meship-starkware]

Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @tdelabro)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Deploy account has the sender address as a member of the transaction on an external layer.

Also - make sure we don't have both getters for contract address and sender address.

Done.
in this pr https://reviewable.io/reviews/starkware-libs/sequencer/699

[meship-starkware]

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @tdelabro)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Maybe return default class hash.

added relevant comment in this PR https://reviewable.io/reviews/starkware-libs/sequencer/699

---

crates/starknet_api/src/executable_transaction.rs line 160 at r4 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I don't like the fact that executable transaction can return max fee.

the executable transaction should be a creature of v3 transaction only (I know it is not, this is the ideal).

will do in this PR https://reviewable.io/reviews/starkware-libs/sequencer/699

[ArniStarkware]

- post expansion of the scope of this PR, and the split.

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware, @noaov1, and @tdelabro)

[meship-starkware]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @tdelabro)

---

crates/starknet_api/src/executable_transaction.rs line 160 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

will do in this PR https://reviewable.io/reviews/starkware-libs/sequencer/699

Done.

[ArniStarkware]

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

[ArniStarkware]

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

[ArniStarkware]

crates/blockifier/src/transaction/transaction_execution.rs line 52 at r9 (raw file):

        }
    }
    pub fn from_api(

spaces.

Suggestion:

    pub fn nonce(&self) -> Nonce {
        match self {
            Self::AccountTransaction(tx) => tx.nonce(),
            Self::L1HandlerTransaction(tx) => tx.tx.nonce,
        }
    }
    
    pub fn sender_address(&self) -> ContractAddress {
        match self {
            Self::AccountTransaction(tx) => tx.sender_address(),
            Self::L1HandlerTransaction(tx) => tx.tx.contract_address,
        }
    }
    
    pub fn from_api(

[ArniStarkware]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware, @noaov1, and @tdelabro)

[meship-starkware]

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

---

crates/blockifier/src/transaction/transaction_execution.rs line 52 at r9 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

spaces.

Done.

[ArniStarkware]

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

[ArniStarkware]

Reviewed 2 of 3 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @tdelabro)

[tdelabro]

Reviewed 2 of 3 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1)

[meship-starkware]

Dismissed @tdelabro from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[noaov1]

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