refactor: gateway compiler handle declare tx

[ArniStarkware]

This change is 

[ArniStarkware]

• feat: post compilation size limit validation #298 : 2 dependent PRs (#348 , #384 )
• chore: add declare_tx fixture to compilation tests #515
• refactor: gateway compiler handle declare tx #439 👈
• chore: add the gateway compiler to appstate #391
• chore: create struct for gateway compiler #383 : 1 other dependent PR (#392 )
• main

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

Join @ArniStarkware and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.25000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (d09a690) to head (d6075c4).

Files	Patch %	Lines
crates/gateway/src/compilation.rs	81.39%	5 Missing and 3 partials ⚠️
crates/gateway/src/gateway.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
+ Coverage   83.59%   83.64%   +0.04%     
==========================================
  Files          37       37              
  Lines        1780     1785       +5     
  Branches     1780     1785       +5     
==========================================
+ Hits         1488     1493       +5     
  Misses        215      215              
  Partials       77       77              

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

[Yael-Starkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @yair-starkware)

---

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

        self.validate_casm_class(&casm_contract_class)?;

        build_result_class_info(

Suggestion:

into_class_info

[ArniStarkware]

Reviewable status: 2 of 7 files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

---

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

        self.validate_casm_class(&casm_contract_class)?;

        build_result_class_info(

Done.

[Yael-Starkware]

Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @yair-starkware)

---

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

fn validate_compiled_class_hash(
    casm_contract_class: &CasmContractClass,
    suppled_compiled_class_hash: CompiledClassHash,

Suggestion:

supplied_compiled_class_hash

---

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

#[rstest]
fn test_compile_contract_class_compiled_class_hash_missmatch(

Suggestion:

mismatch

---

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

        GatewayError::CompiledClassHashMismatch { supplied, hash_result }
        if supplied == supplied_hash && hash_result == expected_hash_result
    );

you can use assert_eq here

Code quote:

    assert_matches!(
        result.unwrap_err(),
        GatewayError::CompiledClassHashMismatch { supplied, hash_result }
        if supplied == supplied_hash && hash_result == expected_hash_result
    );

---

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

            AllowedLibfuncsError::SierraProgramError
        ))
    )

same

Code quote:

    assert_matches!(
        result.unwrap_err(),
        GatewayError::CompilationError(CompilationUtilError::AllowedLibfuncsError(
            AllowedLibfuncsError::SierraProgramError
        ))
    )

---

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

    let optional_class_info = match &tx {
        RPCTransaction::Declare(declare_tx) => {
            Some(gateway_compiler.handle_declare_tx(declare_tx)?)

not sure about the name, it is a bit too general
declare_tx_is handled also in the stateful and stateless validator.

how about process_declare_tx / calculate_class_info ?

Code quote:

handle_declare_tx

[ArniStarkware]

Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions (waiting on @Yael-Starkware and @yair-starkware)

---

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

fn validate_compiled_class_hash(
    casm_contract_class: &CasmContractClass,
    suppled_compiled_class_hash: CompiledClassHash,

Done.

---

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

#[rstest]
fn test_compile_contract_class_compiled_class_hash_missmatch(

Done.

---

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

Previously, Yael-Starkware (YaelD) wrote…

you can use assert_eq here

The enum GatewayError can not derive PartialEq (it contains many variants that do not support it, like JoinError).

---

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

Previously, Yael-Starkware (YaelD) wrote…

same

Same.

---

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

Previously, Yael-Starkware (YaelD) wrote…

not sure about the name, it is a bit too general
declare_tx_is handled also in the stateful and stateless validator.

how about process_declare_tx / calculate_class_info ?

1. Done.
2. I think part of the issue is that the struct is called gateway_compiler. Do you have a better suggestion for a name for it?
3. We look for a verb that says "doing this component's job on a declare tx".

[Yael-Starkware]

Reviewable status: 0 of 11 files reviewed, all discussions resolved (waiting on @yair-starkware)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

1. Done.
2. I think part of the issue is that the struct is called gateway_compiler. Do you have a better suggestion for a name for it?
3. We look for a verb that says "doing this component's job on a declare tx".

All it does is compilation and validation of the related fields , right?
so maybe Compiler is fine.
why is the word gateway needed? since it uses gateway size limits?
sounds ok to me.

[yair-starkware]

Reviewable status: 7 of 11 files reviewed, all discussions resolved (waiting on @dafnamatsry)

[dafnamatsry]

Reviewed 1 of 6 files at r4, 2 of 4 files at r6, 1 of 5 files at r7, all commit messages.
Reviewable status: 7 of 11 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware and @yair-starkware)

---

crates/gateway/src/compilation.rs line 56 at r7 (raw file):

    fn compile(
        &self,
        cairo_lang_contract_class: CairoLangContractClass,

Suggestion:

cairo_lang_contract_class: &CairoLangContractClass

---

crates/gateway/src/compilation.rs line 66 at r7 (raw file):

                Err(GatewayError::CompilationError(CompilationUtilError::CompilationPanic))
            }
        }

Please use map_err.

Code quote:

        match catch_unwind_result {
            Ok(compilation_result) => Ok(compilation_result?),
            Err(_) => {
                // TODO(Arni): Log the panic.
                Err(GatewayError::CompilationError(CompilationUtilError::CompilationPanic))
            }
        }

---

crates/gateway/src/compilation.rs line 104 at r7 (raw file):

/// Validates that the compiled class hash of the compiled contract class matches the supplied
/// compiled class hash.
fn validate_compiled_class_hash(

Should it be also a method of the Compiler like validate_casm_class?

Code quote:

validate_compiled_class_hash

---

crates/gateway/src/compilation.rs line 106 at r7 (raw file):

fn validate_compiled_class_hash(
    casm_contract_class: &CasmContractClass,
    supplied_compiled_class_hash: CompiledClassHash,

Suggestion:

&CompiledClassHash

---

crates/gateway/src/compilation_test.rs line 30 at r7 (raw file):

) {
    let supplied_hash = CompiledClassHash::default();
    let expected_hash_result = compiled_class_hash;

Suggestion:

    let wrong_supplied_hash = CompiledClassHash::default();
    let expected_hash = compiled_class_hash;

---

crates/gateway/src/compilation_test.rs line 32 at r7 (raw file):

    let expected_hash_result = compiled_class_hash;

    let result = validate_compiled_class_hash(&casm_contract_class, supplied_hash);

Please call process_declare_tx which is the public API of the compiler, and the one we want to test.

Code quote:

let result = validate_compiled_class_hash(&casm_contract_class, supplied_hash);

---

crates/gateway/src/compilation_test.rs line 45 at r7 (raw file):

    mut contract_class: RpcContractClass,
) {
    // Create a currupted contract class.

Suggestion:

corrupted

---

crates/gateway/src/compilation_test.rs line 49 at r7 (raw file):

    let cairo_lang_contract_class = into_contract_class_for_compilation(&contract_class);
    let result = gateway_compiler.compile(cairo_lang_contract_class);

Same here. call process_declare_tx.

Code quote:

gateway_compiler.compile

---

crates/gateway/src/compilation_test.rs line 59 at r7 (raw file):

#[rstest]
fn test_handle_declare_tx(gateway_compiler: GatewayCompiler) {

Suggestion:

test_process_declare_tx_success

[dafnamatsry]

Reviewed 1 of 6 files at r1, 2 of 9 files at r3, 3 of 5 files at r7.
Reviewable status: 10 of 11 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware and @yair-starkware)

[ArniStarkware]

Reviewable status: 10 of 11 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry, @dorimedini-starkware, and @yair-starkware)

---

crates/gateway/src/compilation.rs line 56 at r7 (raw file):

    fn compile(
        &self,
        cairo_lang_contract_class: CairoLangContractClass,

The function: CasmContractClass::from_contract_class (from the cairo-lang crate) takes ownership of the CairoLangContractClass

---

crates/gateway/src/compilation.rs line 66 at r7 (raw file):

Previously, dafnamatsry wrote…

Please use map_err.

Done.
(Thank @dorimedini-starkware - for making us not overthink stuff).

---

crates/gateway/src/compilation.rs line 104 at r7 (raw file):

Previously, dafnamatsry wrote…

Should it be also a method of the Compiler like validate_casm_class?

I did not make it a method as it does not use self. I have no objection.

---

crates/gateway/src/compilation.rs line 106 at r7 (raw file):

fn validate_compiled_class_hash(
    casm_contract_class: &CasmContractClass,
    supplied_compiled_class_hash: CompiledClassHash,

Done.
Does not make much of a difference as this struct implements Copy.

---

crates/gateway/src/compilation_test.rs line 30 at r7 (raw file):

) {
    let supplied_hash = CompiledClassHash::default();
    let expected_hash_result = compiled_class_hash;

Done.

---

crates/gateway/src/compilation_test.rs line 32 at r7 (raw file):

Previously, dafnamatsry wrote…

Please call process_declare_tx which is the public API of the compiler, and the one we want to test.

I disagree. This is a unit test for the validate_compiled_class_hash function.
(the declare tx is irrelevant).

This was the original intent for this refactor, per @yair-starkware 's suggestion.

---

crates/gateway/src/compilation_test.rs line 45 at r7 (raw file):

    mut contract_class: RpcContractClass,
) {
    // Create a currupted contract class.

Done.

---

crates/gateway/src/compilation_test.rs line 49 at r7 (raw file):

Previously, dafnamatsry wrote…

Same here. call process_declare_tx.

This is a unit test for compile.

(the declare tx is irrelevant).

---

crates/gateway/src/compilation_test.rs line 59 at r7 (raw file):

#[rstest]
fn test_handle_declare_tx(gateway_compiler: GatewayCompiler) {

Done.

[dorimedini-starkware]

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

---

crates/gateway/src/compilation.rs line 66 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.
(Thank @dorimedini-starkware - for making us not overthink stuff).

🫡

[dafnamatsry]

Reviewed 1 of 5 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @yair-starkware)

---

crates/gateway/src/compilation.rs line 104 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I did not make it a method as it does not use self. I have no objection.

The other validate_* method gets self, but does not use it, so just make them consistent with each other.

---

crates/gateway/src/compilation_test.rs line 32 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I disagree. This is a unit test for the validate_compiled_class_hash function.
(the declare tx is irrelevant).

This was the original intent for this refactor, per @yair-starkware 's suggestion.

Discussed f2f.

---

crates/gateway/src/compilation_test.rs line 49 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This is a unit test for compile.

(the declare tx is irrelevant).

Discussed f2f.

[ArniStarkware]

Reviewable status: 6 of 11 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/gateway/src/compilation.rs line 104 at r7 (raw file):

Previously, dafnamatsry wrote…

The other validate_* method gets self, but does not use it, so just make them consistent with each other.

Done*.
The method, compile still keeps its self. In the following PR: #348 compile uses self.

---

crates/gateway/src/compilation_test.rs line 32 at r7 (raw file):

Previously, dafnamatsry wrote…

Discussed f2f.

Done.

---

crates/gateway/src/compilation_test.rs line 49 at r7 (raw file):

Previously, dafnamatsry wrote…

Discussed f2f.

Done.

[dafnamatsry]

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

---

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

    // TODO(Arni): Pass the compilation args from the config.
    // TODO(Arni): Pass the compiler with dependancy injection.

Note that the compiler should be injected to process_declare_tx and not here, which is the actual compile implementation.

Code quote:

// TODO(Arni): Pass the compiler with dependancy injection.

---

crates/gateway/src/compilation_test.rs line 29 at r10 (raw file):

    tx.compiled_class_hash = supplied_hash;
    let declare_tx = RPCDeclareTransaction::V3(tx);

Suggestion:

    let expected_hash = tx.compiled_class_hash;
    let wrong_supplied_hash = CompiledClassHash::default();
    tx.compiled_class_hash = wrong_supplied_hash;

    let declare_tx = RPCDeclareTransaction::V3(tx);

---

crates/mempool_test_utils/src/starknet_api_test_utils.rs line 95 at r10 (raw file):

/// Get the contract class used for testing.
#[fixture]

Not used as a fixture anymore I think. Also compiled_class_hash()

Code quote:

#[fixture]

[ArniStarkware]

Reviewable status: 8 of 11 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

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

Previously, dafnamatsry wrote…

Note that the compiler should be injected to process_declare_tx and not here, which is the actual compile implementation.

Moved the todo to a more relevant place.

---

crates/gateway/src/compilation_test.rs line 29 at r10 (raw file):

    tx.compiled_class_hash = supplied_hash;
    let declare_tx = RPCDeclareTransaction::V3(tx);

Done.

---

crates/mempool_test_utils/src/starknet_api_test_utils.rs line 95 at r10 (raw file):

Previously, dafnamatsry wrote…

Not used as a fixture anymore I think. Also compiled_class_hash()

Done.

[dafnamatsry]

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

[dafnamatsry]

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