refactor(gateway): send internal tx instead of thin tx

[MohammadNassar1]

This change is 

[MohammadNassar1]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

a discussion (no related file):
Add default external to internal conversion in GW.

---

[ayeletstarkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)

---

crates/gateway/src/utils.rs line 34 at r1 (raw file):

use crate::errors::StatefulTransactionValidatorResult;

pub fn external_tx_to_internal_tx(

I find the Transaction' / 'internal_tx mismatch very confusing.

Code quote:

external_tx_to_internal_tx

[MohammadNassar1]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/gateway/src/utils.rs line 34 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I find the Transaction' / 'internal_tx mismatch very confusing.

Let's discuss it F2F.

[ArniStarkware]

crates/gateway/src/utils.rs line 34 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Let's discuss it F2F.

Why not external_tx_to_executable_tx?

[MohammadNassar1]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/gateway/src/utils.rs line 34 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why not external_tx_to_executable_tx?

I Like it. Updated!

[ayeletstarkware]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @MohammadNassar1)

---

-- commits line 2 at r3:
fix

Code quote:

internal tx

---

crates/gateway/src/utils.rs line 34 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I Like it. Updated!

are you going to change 'TransactiontoExecutabkleTransaction`?

[MohammadNassar1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/gateway/src/utils.rs line 34 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

are you going to change 'TransactiontoExecutabkleTransaction`?

changed already

[elintul]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

a discussion (no related file):
Please update commit message.

---

---

crates/gateway/src/gateway.rs line 146 at r3 (raw file):

    // TODO(Arni): Add the Sierra and the Casm to the mempool input.
    Ok(MempoolInput {
        tx: external_tx_to_executable_tx(&tx, validate_info.tx_hash, validate_info.sender_address),

Add new_from_rpc_tx to ExecutableTransaction; better than a floating function.

Code quote:

external_tx_to_executable_tx

---

crates/gateway/src/utils.rs line 40 at r3 (raw file):

    tx_hash: TransactionHash,
    sender_address: ContractAddress,
) -> Transaction {

Why is this repeating? I saw this in other PRs.

[MohammadNassar1]

Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

a discussion (no related file):

Previously, elintul (Elin) wrote…

Please update commit message.

updated.

---

---

-- commits line 2 at r3:

Previously, ayeletstarkware (Ayelet Zilber) wrote…

fix

Done.

---

crates/gateway/src/gateway.rs line 146 at r3 (raw file):

Previously, elintul (Elin) wrote…

Add new_from_rpc_tx to ExecutableTransaction; better than a floating function.

What do you mean to rename the function?
Or to use a method instead?

---

crates/gateway/src/utils.rs line 40 at r3 (raw file):

Previously, elintul (Elin) wrote…

Why is this repeating? I saw this in other PRs.

This one converts from external to full transaction.
The other one converts from thin tx to full tx, and it's temporal, removed here.

[elintul]

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 146 at r3 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What do you mean to rename the function?
Or to use a method instead?

Both.

---

crates/gateway/src/utils.rs line 51 at r4 (raw file):

                calldata: Calldata::default(),
                nonce_data_availability_mode: DataAvailabilityMode::L1,
                fee_data_availability_mode: DataAvailabilityMode::L2,

Please be minded.

Suggestion:

L1

[elintul]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/starknet_api/src/rpc_transaction.rs line 89 at r5 (raw file):

    // TODO(Arni): Update the function to support all transaction types.
    pub fn new_from_rpc_tx(
        &self,

How come &self is passed? This should be a c-tor.

Suggestion:

    }
    
    // TODO(Arni): Update the function to support all transaction types.
    pub fn new_from_rpc_tx(
        &self,

[elintul]

Reviewed all commit messages.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/starknet_api/src/rpc_transaction.rs line 89 at r5 (raw file):

Previously, elintul (Elin) wrote…

How come &self is passed? This should be a c-tor.

A c-tor of Transaction.

[elintul]

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

[elintul]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

[elintul]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

[elintul]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

    // TODO(Arni): Update the function to support all transaction types.
    pub fn new_from_rpc_tx(
        rpc_tx: crate::rpc_transaction::RpcTransaction,

Why fully-qualified?

Code quote:

crate::rpc_transaction::RpcTransaction

---

crates/starknet_api/src/executable_transaction.rs line 74 at r9 (raw file):

            tx: crate::transaction::InvokeTransaction::V3(
                crate::transaction::InvokeTransactionV3 {
                    sender_address,

For invoke, take the address from the input transaction.

Code quote:

sender_address,

---

crates/starknet_api/src/executable_transaction.rs line 76 at r9 (raw file):

                    sender_address,
                    tip: *rpc_tx.tip(),
                    nonce: *rpc_tx.nonce(),

Why cloning?

Code quote:

                    tip: *rpc_tx.tip(),
                    nonce: *rpc_tx.nonce(),

[MohammadNassar1]

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/gateway/src/gateway.rs line 146 at r3 (raw file):

Previously, elintul (Elin) wrote…

Both.

Done.

---

crates/gateway/src/utils.rs line 51 at r4 (raw file):

Previously, elintul (Elin) wrote…

Please be minded.

Sorry :(.

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, elintul (Elin) wrote…

Why fully-qualified?

removed

---

crates/starknet_api/src/rpc_transaction.rs line 89 at r5 (raw file):

Previously, elintul (Elin) wrote…

A c-tor of Transaction.

Done.

[MohammadNassar1]

Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @Arni, @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 74 at r9 (raw file):

Previously, elintul (Elin) wrote…

For invoke, take the address from the input transaction.

The sender address is calculated during the stateful validation.
and the method should be generic (@Arni will update it).

---

crates/starknet_api/src/executable_transaction.rs line 76 at r9 (raw file):

Previously, elintul (Elin) wrote…

Why cloning?

It returns &Nonce, and &Tip.

[elintul]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arni, @ArniStarkware, @ayeletstarkware, and @giladchase)

---

crates/gateway/src/utils.rs line 51 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Sorry :(.

No worries.

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

removed

Can it receive it by value? The RpcTransaction isn't used after this.

---

crates/starknet_api/src/executable_transaction.rs line 74 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

The sender address is calculated during the stateful validation.
and the method should be generic (@Arni will update it).

For invoke, it shouldn't be used.

[MohammadNassar1]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arni, @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, elintul (Elin) wrote…

Can it receive it by value? The RpcTransaction isn't used after this.

Can you explain, please, why we need to pass by value and not by ref?
What if we'll use it after this?

[elintul]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arni, @ArniStarkware, @ayeletstarkware, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Can you explain, please, why we need to pass by value and not by ref?
What if we'll use it after this?

But we don't, it's the return value of process_tx; i.e., the last line of the function.

[elintul]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 74 at r9 (raw file):

Previously, elintul (Elin) wrote…

For invoke, it shouldn't be used.

Add a TODO with your suggestion.

[elintul]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

[MohammadNassar1]

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, elintul (Elin) wrote…

But we don't, it's the return value of process_tx; i.e., the last line of the function.

Changed to by value.

---

crates/starknet_api/src/executable_transaction.rs line 74 at r9 (raw file):

Previously, elintul (Elin) wrote…

Add a TODO with your suggestion.

Added.

[elintul]

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @giladchase)

---

crates/starknet_api/src/executable_transaction.rs line 67 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Changed to by value.

For the record: this is temporary, until Sierra class is sent to mempool, to be propagated by P2P.

[ayeletstarkware]

Reviewed 1 of 1 files at r8, 1 of 3 files at r9, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @giladchase)

[ayeletstarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @giladchase)