refactor: create executable tx in the gateway process_tx

[ArniStarkware]

In this PR we create the executable_tx. This will be a member of MempoolInput.

This change is 

[ArniStarkware]

• refactor: create executable tx in the gateway process_tx #517 : 3 dependent PRs (#518 , #519 , #694 ) 👈
• chore: define creator for executable transaction #686
• chore: add impl from rpc tx to snapi tx #668
• main

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

Join @ArniStarkware and the rest of your teammates on Graphite

[ArniStarkware]

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

Previously, ArniStarkware (Arnon Hod) wrote…

error!("Failed to convert Starknet API ClassInfo to Blockifier ClassInfo: {:?}", e);

Done.

[dafnamatsry]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @MohammadNassar1, and @yair-starkware)

---

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

        &tx,
        &gateway_compiler,
        &stateful_tx_validator.config.chain_info.chain_id,

Can you add a TODO to move the chain ID config out to the GW config?

Code quote:

&stateful_tx_validator.config.chain_info.chain_id,

---

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

use crate::errors::{GatewaySpecError, StatefulTransactionValidatorResult};

pub fn external_tx_to_executable_tx(

Let's move all this code to SNAPI?

---

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

use crate::errors::{GatewaySpecError, StatefulTransactionValidatorResult};

pub fn external_tx_to_executable_tx(

Here and everywhere else.

Suggestion:

rpc_tx_to_executable_tx

---

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

fn external_declare_tx_to_executable_tx(
    external_tx: &RpcDeclareTransaction,
    gateway_compiler: &GatewayCompiler,

I think this function should get the compiled class_info as an argument.
Convert functions shouldn't hide heavy stuff like compilation.

Code quote:

gateway_compiler: &GatewayCompiler,

---

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

) -> Result<ExecutableDeclareTransaction, GatewaySpecError> {
    let class_info = gateway_compiler.process_declare_tx(external_tx)?;
    let RpcDeclareTransaction::V3(tx) = external_tx;

Just to make sure: if we add another version, i.e. RpcDecalareTransaction::V4, this code won't compile right?

Code quote:

let RpcDeclareTransaction::V3(tx) = external_tx;

---

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

    let declare_tx = DeclareTransaction::V3(DeclareTransactionV3 {
        class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api
                                           * function once ready */

Is it ready?

Code quote:

        class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api
                                           * function once ready */

[dafnamatsry]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @MohammadNassar1, and @yair-starkware)

---

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

Previously, dafnamatsry wrote…

Can you add a TODO to move the chain ID config out to the GW config?

Just noticed you added it in a following PR :)

[ArniStarkware]

Reviewable status: 1 of 9 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @dafnamatsry, @MohammadNassar1, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 33 at r10 (raw file):

Previously, dafnamatsry wrote…

You can do it as part of this PR.

No longer relevant.

---

crates/gateway/src/compilation.rs line 60 at r10 (raw file):

Previously, dafnamatsry wrote…

Why did you change the return value?

The role of the compiler is to compile and shouldn't handle any other unrelated declare tx fields.
Please move the conversion logic to compile_to_casm_and_convert_rpc_to_executable_tx.

no longer relevant.

---

crates/gateway/src/gateway_test.rs line 104 at r10 (raw file):

Previously, dafnamatsry wrote…

Can we calculate the hash without going through an AccountTransaction?

Maybe you can use rpc_tx.into().calculate_transaction_hash().

This function will be deleted. This is out of the scope of this PR.

---

crates/gateway/src/utils.rs line 44 at r10 (raw file):

Previously, dafnamatsry wrote…

No need for the map_err right?

Note that you are mapping a legitimate CompilationFailed and ompiledClassHashMismatch to UnexpectedError.
Please add a test to gateway_test to make sure we return those errors.

Done.

[dafnamatsry]

Reviewed 7 of 7 files at r13, all commit messages.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @MohammadNassar1, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 36 at r13 (raw file):

    }

    // TODO(Arni): Squash this function into `process_declare_tx`.

Remove :)

Code quote:

    // TODO(Arni): Squash this function into `process_declare_tx`.

---

crates/gateway/src/compilation_test.rs line 70 at r13 (raw file):

#[rstest]
fn test_class_info_from_declare_tx_tx_success(

Suggestion:

test_process_declare_tx_success

---

crates/gateway/src/utils.rs line 39 at r13 (raw file):

/// executable contract class.
pub fn compile_contract_and_build_executable_tx(
    rpc_tx: RpcTransaction,

and then you won't need the clone in the gateway...

In general, I think in most cases we only want to pass references as function arguments (unless you want to enforce that an object can't be used after the function call).

Suggestion:

rpc_tx: &RpcTransaction,

---

crates/gateway/src/utils.rs line 85 at r13 (raw file):

fn compile_contract_and_build_executable_declare_tx(
    rpc_tx: RpcDeclareTransaction,

Suggestion:

rpc_tx: &RpcDeclareTransaction,

[ArniStarkware]

Reviewable status: 6 of 9 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @dafnamatsry, @MohammadNassar1, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 36 at r13 (raw file):

Previously, dafnamatsry wrote…

Remove :)

Done.

---

crates/gateway/src/compilation_test.rs line 70 at r13 (raw file):

#[rstest]
fn test_class_info_from_declare_tx_tx_success(

Done.

[dafnamatsry]

Reviewed 2 of 2 files at r14, all commit messages.
Reviewable status: 8 of 9 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @MohammadNassar1, and @yair-starkware)

[ArniStarkware]

crates/gateway/src/utils.rs line 39 at r13 (raw file):

Previously, dafnamatsry wrote…

and then you won't need the clone in the gateway...

In general, I think in most cases we only want to pass references as function arguments (unless you want to enforce that an object can't be used after the function call).

Discussed on DMs. Ignored.
The ownership-taking is by design.

[ArniStarkware]

Reviewed 1 of 2 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware, @dafnamatsry, @MohammadNassar1, and @yair-starkware)

[ArniStarkware]

Merge activity

• Sep 8, 4:31 AM EDT: @ArniStarkware started a stack merge that includes this pull request via Graphite.
• Sep 8, 4:33 AM EDT: Graphite rebased this pull request as part of a merge.
• Sep 8, 4:45 AM EDT: @ArniStarkware merged this pull request with Graphite.