feat: declare post compilation builtin validation

[ArniStarkware]

This change is 

[ArniStarkware]

• feat: declare post compilation builtin validation #296 👈
• feat: check that the compiled class hash matches the supplied class #246 : 2 other dependent PRs (#298 , #312 )
• test: add tests for add tx on declare tx #235
• feat: compile sierra to casm in declare tx #234
• 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 82.14286% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.11%. Comparing base (d7802ae) to head (4a80554).

Files	Patch %	Lines
crates/gateway/src/gateway.rs	68.75%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #296   +/-   ##
=======================================
  Coverage   82.11%   82.11%           
=======================================
  Files          32       32           
  Lines        1560     1588   +28     
  Branches     1560     1588   +28     
=======================================
+ Hits         1281     1304   +23     
- Misses        215      219    +4     
- Partials       64       65    +1     

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

[Yael-Starkware]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

---

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

        "segment_arena".to_string(),
    ]
}

must these be strings? enum would be prettier but I assume the set of builtins is defined as strings by the cairo library so it doesn't make sense to store it differently only for this check.

Code quote:

fn get_supported_builtins() -> Vec<String> {
    vec![
        "pedersen".to_string(),
        "range_check".to_string(),
        "ecdsa".to_string(),
        "bitwise".to_string(),
        "ec_op".to_string(),
        "poseidon".to_string(),
        "segment_arena".to_string(),
    ]
}

---

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

    for entry_point in entry_points_iterator {
        let builtins = &entry_point.builtins;
        println!("{builtins:?}");

left here by mistake?

Code quote:

println!("{builtins:?}");

---

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

    offset == subsequence.len()
}

From ChatGPT :)

Suggestion:

/// Checks whether 'subsequence' is a subsequence of 'sequence'.
pub fn is_subsequence(subsequence: &[String], sequence: &[String]) -> bool {
    subsequence.iter().all(|item| sequence.contains(item))
}

[ArniStarkware]

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

---

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

Previously, Yael-Starkware (YaelD) wrote…

must these be strings? enum would be prettier but I assume the set of builtins is defined as strings by the cairo library so it doesn't make sense to store it differently only for this check.

Their definition in Cairo-lang is somewhat complicated.

---

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

Previously, Yael-Starkware (YaelD) wrote…

left here by mistake?

Removed.

---

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

Previously, Yael-Starkware (YaelD) wrote…

From ChatGPT :)

Done.

I used a different code. I did that, assuming the order matters. I need to get back to you why the order is important. Also added a test.

[ArniStarkware]

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

---

crates/gateway/src/utils.rs line 164 at r1 (raw file):
According to @ilyalesokhin-starkware, the order is important.

Yes, the order is important the OS does not support arbitrary order

[ArniStarkware]

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

Previously, ArniStarkware (Arnon Hod) wrote…

Their definition in Cairo-lang is somewhat complicated.

Added this PR on top of this one: #319. It is connected to a PR in the starknet-api repo: see: starkware-libs/starknet-api#288

[yair-starkware]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @ilyalesokhin-starkware, and @Yael-Starkware)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Added this PR on top of this one: #319. It is connected to a PR in the starknet-api repo: see: starkware-libs/starknet-api#288

Why not use a static variable?

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

According to @ilyalesokhin-starkware, the order is important.

Yes, the order is important the OS does not support arbitrary order

Is the order being checked by the contains function, which consumes the iterator up to the found item?
IMO this is very confusing. Consider using nested for loops.

[ArniStarkware]

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

Previously, yair-starkware (Yair) wrote…

Is the order being checked by the contains function, which consumes the iterator up to the found item?
IMO this is very confusing. Consider using nested for loops.

Did you see the version from Revision 1? Is it preferable?

[Yael-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @ilyalesokhin-starkware)

---

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

Previously, yair-starkware (Yair) wrote…

I think so.
Also, consider making the function generic instead of String type:

fn is_subsequence<T: Eq>(subsequence: &[T], sequence: &[T]) -> bool

I agree that this is confusing, not sure I understand how the order is kept here.

[ArniStarkware]

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

Previously, Yael-Starkware (YaelD) wrote…

I agree that this is confusing, not sure I understand how the order is kept here.

Done.

[dafnamatsry]

Reviewed 2 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ilyalesokhin-starkware, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/errors.rs line 42 at r4 (raw file):

    StatelessTransactionValidatorError(#[from] StatelessTransactionValidatorError),
    #[error("{builtins:?} is not a subsquence of {supported_builtins:?}")]
    SupportedBuiltins { builtins: Vec<String>, supported_builtins: Vec<String> },

UnsupportedBuiltins?

Code quote:

SupportedBuiltins

---

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

Previously, yair-starkware (Yair) wrote…

Why not use a static variable?

+1 for static variable.

---

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

fn get_supported_builtins() -> Vec<String> {
    let builtins =
        ["pedersen", "range_check", "ecdsa", "bitwise", "ec_op", "poseidon", "segment_arena"];

Add a comment that the order is important.

Code quote:

    let builtins =
        ["pedersen", "range_check", "ecdsa", "bitwise", "ec_op", "poseidon", "segment_arena"];

---

crates/gateway/src/utils_test.rs line 32 at r4 (raw file):

    true
)]
#[case::subsequence_1(

I guess that's you meant.

Suggestion:

#[case::subsequence_2(
    &["b"],
    &["a", "b", "c"],
    true
)]
#[case::subsequence_3(

[ArniStarkware]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry and @ilyalesokhin-starkware)

---

crates/gateway/src/errors.rs line 42 at r4 (raw file):

Previously, dafnamatsry wrote…

UnsupportedBuiltins?

Done.

---

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

Previously, dafnamatsry wrote…

+1 for static variable.

Done.

---

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

Previously, dafnamatsry wrote…

Add a comment that the order is important.

Done.

---

crates/gateway/src/utils_test.rs line 32 at r4 (raw file):

Previously, dafnamatsry wrote…

I guess that's you meant.

Done.

[dafnamatsry]

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

---

crates/gateway/src/gateway.rs line 206 at r5 (raw file):

            ["pedersen", "range_check", "ecdsa", "bitwise", "ec_op", "poseidon", "segment_arena"];
        SUPPORTED_BUILTIN_NAMES.iter().map(|builtin| builtin.to_string()).collect::<Vec<String>>()
    };

It can be just a static const.

Suggestion:

    // The OS expects this order for the builtins.
    const SUPPORTED_BUILTINS: [&'static str; 7] = [
            "pedersen", "range_check", "ecdsa", "bitwise", "ec_op", "poseidon", "segment_arena"];
    };

---

crates/gateway/src/gateway.rs line 224 at r5 (raw file):

            });
        }
    }

Please avoid defining variables that are only used once, and are self explanatory.

Suggestion:

    for entry_point in entry_points_iterator {
        let builtins = &entry_point.builtins;
        if !is_subsequence(builtins, supported_builtins) {
            return Err(GatewayError::UnsupportedBuiltins {
                builtins: builtins.clone(),
                supported_builtins: &SUPPORTED_BUILTINS.to_vec(),
            });
        }
    }

[ArniStarkware]

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

---

crates/gateway/src/gateway.rs line 206 at r5 (raw file):

Previously, dafnamatsry wrote…

It can be just a static const.

Done.

constants have by default a `'static` lifetime

---

crates/gateway/src/gateway.rs line 224 at r5 (raw file):

Previously, dafnamatsry wrote…

Please avoid defining variables that are only used once, and are self explanatory.

At least in the current implementation, we need to convert &str -> String and save the result to a variable.
See the updated code. (Note taken for upstream PR).

[dafnamatsry]

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

---

crates/gateway/src/gateway.rs line 224 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

At least in the current implementation, we need to convert &str -> String and save the result to a variable.
See the updated code. (Note taken for upstream PR).

Oh. I missed the conversion to string thing...
In that case let's go back to the lazy static.

Sorry for the confusion.

[ArniStarkware]

crates/gateway/src/gateway.rs line 224 at r5 (raw file):

Previously, dafnamatsry wrote…

Oh. I missed the conversion to string thing...
In that case let's go back to the lazy static.

Sorry for the confusion.

Done.

[dafnamatsry]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

[dafnamatsry]

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

[Yael-Starkware]

Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @dafnamatsry and @ilyalesokhin-starkware)

[Yael-Starkware]

Reviewed 1 of 3 files at r1, 2 of 6 files at r5, 3 of 3 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry and @ilyalesokhin-starkware)