feat: compile sierra to casm in declare tx

[ArniStarkware]

This change is 

[ArniStarkware]

• feat: check that the compiled class hash matches the supplied class #246 : 3 dependent PRs (#296 , #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 4.05405% with 71 lines in your changes missing coverage. Please review.

Project coverage is 74.40%. Comparing base (a7477f1) to head (7c93ecc).

Files	Patch %	Lines
crates/starknet_sierra_compile/src/utils.rs	0.00%	48 Missing ⚠️
crates/gateway/src/gateway.rs	11.53%	22 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
- Coverage   78.55%   74.40%   -4.16%     
==========================================
  Files          27       28       +1     
  Lines        1236     1309      +73     
  Branches     1236     1309      +73     
==========================================
+ Hits          971      974       +3     
- Misses        208      278      +70     
  Partials       57       57              

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

[Yael-Starkware]

Reviewed 3 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 5 of 6 files reviewed, 13 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

---

crates/gateway/src/gateway.rs line 129 at r2 (raw file):

    let optional_class_info = get_optional_class_info(&tx)?;

    // TODO(Yael, 19/5/2024): pass the relevant class_info and deploy_account_hash.

Suggestion:

pass the relevant deploy_account_hash.

---

crates/gateway/src/gateway.rs line 141 at r2 (raw file):

fn get_optional_class_info(tx: &ExternalTransaction) -> GatewayResult<Option<ClassInfo>> {
    if let ExternalTransaction::Declare(ExternalDeclareTransaction::V3(tx)) = tx {

Suggestion:

let tx = match tx {
    ExternalTransaction::Declare(ExternalDeclareTransaction::V3(tx)) => tx,
    _ => return Ok(None)
}

---

crates/gateway/src/gateway.rs line 142 at r2 (raw file):

fn get_optional_class_info(tx: &ExternalTransaction) -> GatewayResult<Option<ClassInfo>> {
    if let ExternalTransaction::Declare(ExternalDeclareTransaction::V3(tx)) = tx {
        let starknet_api_contract_class: starknet_api::external_transaction::ContractClass =

Move to use statement
you can set it as StarkNetApiContractClass, BlockifierContractCalss and CairoContractClass

Code quote:

starknet_api::external_transaction::ContractClass

---

crates/gateway/src/gateway.rs line 162 at r2 (raw file):

        };

        // TODO: Handle unwrap.

Is this something you meant to do in this PR?

Code quote:

// TODO: Handle unwrap.

---

crates/gateway/src/gateway.rs line 168 at r2 (raw file):

            ContractClassV1::try_from_json_string(&raw_contract_class).unwrap();

        let blockifier_contract_class: blockifier::execution::contract_class::ContractClass =

move to use statement at the top

Code quote:

blockifier::execution::contract_class::ContractClass

---

crates/gateway/src/gateway.rs line 172 at r2 (raw file):

        let class_info =
            ClassInfo::new(&blockifier_contract_class, sierra_program_length, abi_length)
                .expect("Expects a Cairo 1 contract class");

is that the only possible error?

Code quote:

.expect("Expects a Cairo 1 contract class");

---

crates/gateway/src/gateway.rs line 180 at r2 (raw file):

fn starknet_api_contract_class_to_cairo_lang_contract_class(
    starknet_api_contract_class: starknet_api::external_transaction::ContractClass,

same

Code quote:

 starknet_api_contract_class: starknet_api::external_transaction::ContractClass

---

crates/gateway/src/gateway.rs line 181 at r2 (raw file):

fn starknet_api_contract_class_to_cairo_lang_contract_class(
    starknet_api_contract_class: starknet_api::external_transaction::ContractClass,
) -> cairo_lang_starknet_classes::contract_class::ContractClass {

same

Code quote:

 cairo_lang_starknet_classes::contract_class::ContractClass

---

crates/gateway/src/gateway.rs line 192 at r2 (raw file):

    );

    cairo_lang_starknet_classes::contract_class::ContractClass {

same

Code quote:

cairo_lang_starknet_classes::contract_class::ContractClass

---

crates/gateway/src/gateway.rs line 198 at r2 (raw file):

        entry_points_by_type,
        // The Abi is irrelevant to the computlation.
        abi: None,

This could be error prone, if this function is used for a different purpose.
If complicated to implement I think it should be at least documented at the function level

Code quote:

        // The Abi is irrelevant to the computlation.
        abi: None,

---

crates/gateway/src/gateway.rs line 204 at r2 (raw file):

fn entry_point_by_type_to_contract_entry_points(
    entry_points_by_type: starknet_api::external_transaction::EntryPointByType,
) -> cairo_lang_starknet_classes::contract_class::ContractEntryPoints {

same

Code quote:

    entry_points_by_type: starknet_api::external_transaction::EntryPointByType,
) -> cairo_lang_starknet_classes::contract_class::ContractEntryPoints {

---

crates/gateway/src/gateway.rs line 234 at r2 (raw file):

fn stark_felt_to_big_uint(stark_felt: StarkFelt) -> BigUint {
    // TODO: Solve the unwrap.

I think you need to propagate the error.

Code quote:

// TODO: Solve the unwrap.

---

crates/starknet_sierra_compile/src/compile.rs line 23 at r2 (raw file):

    #[error(transparent)]
    StarknetSierraCompilationError(#[from] StarknetSierraCompilationError),
    #[error("Compilation have paniced")]

Suggestion:

Compilation panicked

[Yael-Starkware]

Reviewed 1 of 6 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

[ArniStarkware]

Reviewable status: 4 of 6 files reviewed, 13 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/gateway/src/gateway.rs line 129 at r2 (raw file):

    let optional_class_info = get_optional_class_info(&tx)?;

    // TODO(Yael, 19/5/2024): pass the relevant class_info and deploy_account_hash.

Done :)

---

crates/gateway/src/gateway.rs line 141 at r2 (raw file):

fn get_optional_class_info(tx: &ExternalTransaction) -> GatewayResult<Option<ClassInfo>> {
    if let ExternalTransaction::Declare(ExternalDeclareTransaction::V3(tx)) = tx {

Done.

---

crates/gateway/src/gateway.rs line 142 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Move to use statement
you can set it as StarkNetApiContractClass, BlockifierContractCalss and CairoContractClass

Done.

---

crates/gateway/src/gateway.rs line 168 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

move to use statement at the top

Done.

---

crates/gateway/src/gateway.rs line 180 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same

Done.

---

crates/gateway/src/gateway.rs line 181 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same

Done.

---

crates/gateway/src/gateway.rs line 192 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same

Done.

---

crates/gateway/src/gateway.rs line 204 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

same

Done.

---

crates/gateway/src/gateway.rs line 234 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think you need to propagate the error.

I looked into the function BigUint::from_radix_be.
It returns an Optional(BigUint). The only case when it returns None is when radix (the second parameter) is not 256.

---

crates/starknet_sierra_compile/src/compile.rs line 23 at r2 (raw file):

    #[error(transparent)]
    StarknetSierraCompilationError(#[from] StarknetSierraCompilationError),
    #[error("Compilation have paniced")]

Done.

[dafnamatsry]

Reviewed 1 of 6 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 4 of 6 files reviewed, 24 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

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

pub enum GatewayError {
    #[error(transparent)]
    CompilationUtilError(#[from] starknet_sierra_compile::compile::CompilationUtilError),

Suggestion:

CompilationError

---

crates/gateway/src/gateway.rs line 198 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

This could be error prone, if this function is used for a different purpose.
If complicated to implement I think it should be at least documented at the function level

Agreed. Consider renaming the function to get_contract_class_for_compilation.

---

crates/gateway/src/gateway.rs line 234 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I looked into the function BigUint::from_radix_be.
It returns an Optional(BigUint). The only case when it returns None is when radix (the second parameter) is not 256.

I think you have BigUint::from_bytes_be().

---

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

}

fn get_optional_class_info(tx: &ExternalTransaction) -> GatewayResult<Option<ClassInfo>> {

Few things about the function signature:

• It should be clear from the name of the function that it compiles the contract class. get_optional_class_info doesn't describe anything about what the function does.
• No reason to get an ExternalTransaction if the function is only relevant for Declare transactions. And the optional in the return value is also not relevant anymore.
• Note that by changing the type of the argument, passing wrong tx type will be caught at compilation rather than by returning None at runtime.

Suggestion:

fn compile_contract_class(tx: &ExternalDeclareTransaction) -> GatewayResult<ClassInfo>

---

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

    Ok(Some(class_info))
}

Please move all the conversion code to some utils file. Maybe even to the compilation crate.

---

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

}

fn starknet_api_contract_class_to_cairo_lang_contract_class(

It can be inferred from the argument that it is SNAPI contract class that is being converted.

Suggestion:

cairo_lang_contract_class

---

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

fn starknet_api_contract_class_to_cairo_lang_contract_class(
    starknet_api_contract_class: StarknetApiContractClass,

Suggestion:

starknet_api_contract_class: &StarknetApiContractClass

---

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

        .map(stark_felt_to_big_uint_as_hex)
        .collect();
    let contract_class_version = starknet_api_contract_class.contract_class_version;

You only use this variable once, so no need to define it here and better to just call starknet_api_contract_class.contract_class_version when needed.

Code quote:

let contract_class_version = starknet_api_contract_class.contract_class_version;

---

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

        contract_class_version,
        entry_points_by_type,
        // The Abi is irrelevant to the computlation.

Suggestion:

compilation

---

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

}

fn entry_point_by_type_to_contract_entry_points(

Suggestion:

cairo_lang_contract_entry_points

---

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

fn entry_point_by_type_to_contract_entry_points(
    entry_points_by_type: StarknetApiEntryPointByType,

Suggestion:

 entry_points_by_type: &StarknetApiEntryPointByType,

---

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

fn starknet_api_entry_points_to_contract_entry_points(
    entry_points: Vec<StarknetApiEntryPoint>,
) -> Vec<CairoLangContractEntryPoint> {

Suggestion:

fn cairo_lang_contract_entry_points_vec(
    entry_points: &Vec<StarknetApiEntryPoint>,
) -> Vec<CairoLangContractEntryPoint> {

---

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

fn entry_point_into_contract_entry_point(
    entry_point: StarknetApiEntryPoint,
) -> CairoLangContractEntryPoint {

Suggestion:

fn cairo_lang_contract_entry_point(
    entry_point: &StarknetApiEntryPoint,
) -> CairoLangContractEntryPoint {

[ArniStarkware]

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

---

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

pub enum GatewayError {
    #[error(transparent)]
    CompilationUtilError(#[from] starknet_sierra_compile::compile::CompilationUtilError),

Done.

---

crates/gateway/src/gateway.rs line 162 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Is this something you meant to do in this PR?

Done. See similar precedent in #225.

---

crates/gateway/src/gateway.rs line 172 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

is that the only possible error?

Yes. Added explicit error handling:

1. for readability
2. for the Rust compiler to tell us if new error variants are supported by this method.

@ayeletstarkware I see you were the last one to handle the ClassInfo::new method. Must it be this unforgiving?

---

crates/gateway/src/gateway.rs line 198 at r2 (raw file):

Previously, dafnamatsry wrote…

Agreed. Consider renaming the function to get_contract_class_for_compilation.

Done.

---

crates/gateway/src/gateway.rs line 234 at r2 (raw file):

Previously, dafnamatsry wrote…

I think you have BigUint::from_bytes_be().

Done.

---

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

Previously, dafnamatsry wrote…

Few things about the function signature:

• It should be clear from the name of the function that it compiles the contract class. get_optional_class_info doesn't describe anything about what the function does.
• No reason to get an ExternalTransaction if the function is only relevant for Declare transactions. And the optional in the return value is also not relevant anymore.
• Note that by changing the type of the argument, passing wrong tx type will be caught at compilation rather than by returning None at runtime.

Done.

---

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

Previously, dafnamatsry wrote…

Please move all the conversion code to some utils file. Maybe even to the compilation crate.

Done.

---

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

Previously, dafnamatsry wrote…

It can be inferred from the argument that it is SNAPI contract class that is being converted.

Done.

---

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

fn starknet_api_contract_class_to_cairo_lang_contract_class(
    starknet_api_contract_class: StarknetApiContractClass,

Done.
This was important. It had sound ripple effects.

---

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

Previously, dafnamatsry wrote…

You only use this variable once, so no need to define it here and better to just call starknet_api_contract_class.contract_class_version when needed.

Done.

---

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

        contract_class_version,
        entry_points_by_type,
        // The Abi is irrelevant to the computlation.

Done.

---

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

}

fn entry_point_by_type_to_contract_entry_points(

Done.

---

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

fn entry_point_by_type_to_contract_entry_points(
    entry_points_by_type: StarknetApiEntryPointByType,

Done.

---

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

fn starknet_api_entry_points_to_contract_entry_points(
    entry_points: Vec<StarknetApiEntryPoint>,
) -> Vec<CairoLangContractEntryPoint> {

Done.

---

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

fn entry_point_into_contract_entry_point(
    entry_point: StarknetApiEntryPoint,
) -> CairoLangContractEntryPoint {

Done.

[dafnamatsry]

Reviewed 1 of 2 files at r3, 2 of 8 files at r4, 1 of 3 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @Yael-Starkware)

---

crates/gateway/src/errors.rs line 23 at r6 (raw file):

    MessageSendError(String),
    #[error(transparent)]
    ProgramError(#[from] ProgramError),

Is this error needed?

Code quote:

    #[error(transparent)]
    ProgramError(#[from] ProgramError),

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Done.
This was important. It had sound ripple effects.

:)

---

crates/gateway/src/gateway.rs line 162 at r6 (raw file):

    // Convert Casm contract class to Starknet contract class directly.
    let blockifier_contract_class = ContractClassV1::try_from(casm_contract_class)?.into();

Needed?

Code quote:

.into()

---

crates/gateway/src/gateway.rs line 173 at r6 (raw file):

            sierra_program_length: _,
        }) => {
            panic!(

Why panic?
The sierra program is an input from the user, and can be invalid and with 0 length.

Code quote:

panic!(

[ArniStarkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/gateway/src/errors.rs line 23 at r6 (raw file):

Previously, dafnamatsry wrote…

Is this error needed?

Yes. The following line needs it:

ContractClassV1::try_from(casm_contract_class)?

---

crates/gateway/src/gateway.rs line 162 at r6 (raw file):

Previously, dafnamatsry wrote…

Needed?

Yes.
blockifier_contract_class needs to be of type ContractClass (blockifier::execution::contract_class::ContractClass). This is an enum where one of it's members is of type: V1, which holds a ContractClassV1 (blockifier::execution::contract_class::ContractClassV1).

---

crates/gateway/src/gateway.rs line 173 at r6 (raw file):

Previously, dafnamatsry wrote…

Why panic?
The sierra program is an input from the user, and can be invalid and with 0 length.

The function ClassInfo::new allows the creation of class info for Cairo 0 and Cairo 1 contracts.
The only way ClassInfo::new can have an error is when there is a mismatch. In this context, we are dealing with the compilation of Cairo 1 contracts (we are compiling Sierra -> Casm). It is an assumption that this error can never happen. We could have even used an expect instead of a match, but I wanted to ensure we are explicit here. If ClassInfo::new will add a new error variant - the compiler will tell us about it.

[dafnamatsry]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @Yael-Starkware)

---

crates/gateway/src/errors.rs line 23 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Yes. The following line needs it:

ContractClassV1::try_from(casm_contract_class)?

It's not clear in the context of the GatewayError, to what program we refer to here. Maybe rename toContractProgramError or similar.

---

crates/gateway/src/gateway.rs line 162 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Yes.
blockifier_contract_class needs to be of type ContractClass (blockifier::execution::contract_class::ContractClass). This is an enum where one of it's members is of type: V1, which holds a ContractClassV1 (blockifier::execution::contract_class::ContractClassV1).

In that case, would ContractClass::V1(ContractClassV1::try_from(casm_contract_class)?) work?
It is more explicit and clear what type we expect

---

crates/gateway/src/gateway.rs line 173 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The function ClassInfo::new allows the creation of class info for Cairo 0 and Cairo 1 contracts.
The only way ClassInfo::new can have an error is when there is a mismatch. In this context, we are dealing with the compilation of Cairo 1 contracts (we are compiling Sierra -> Casm). It is an assumption that this error can never happen. We could have even used an expect instead of a match, but I wanted to ensure we are explicit here. If ClassInfo::new will add a new error variant - the compiler will tell us about it.

Not sure the assumption that this can never happen is right.
We simply pass the length of sierra_program which is given to us by the user as part of tx. It can be 0.
You can say that we assume that if the compilation passed by now then it must be > 0. But I won't count on it.

I think we should return the error here.

[ArniStarkware]

Reviewable status: 5 of 9 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/gateway/src/errors.rs line 23 at r6 (raw file):

Previously, dafnamatsry wrote…

It's not clear in the context of the GatewayError, to what program we refer to here. Maybe rename toContractProgramError or similar.

Done. Changed to DeclaredContractProgramError.

---

crates/gateway/src/gateway.rs line 162 at r6 (raw file):

Previously, dafnamatsry wrote…

In that case, would ContractClass::V1(ContractClassV1::try_from(casm_contract_class)?) work?
It is more explicit and clear what type we expect

Done.

---

crates/gateway/src/gateway.rs line 173 at r6 (raw file):

Previously, dafnamatsry wrote…

Not sure the assumption that this can never happen is right.
We simply pass the length of sierra_program which is given to us by the user as part of tx. It can be 0.
You can say that we assume that if the compilation passed by now then it must be > 0. But I won't count on it.

I think we should return the error here.

Done. Added DeclaredContractClassError.

[dafnamatsry]

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware and @Yael-Starkware)

[Yael-Starkware]

Reviewed 1 of 2 files at r3, 2 of 8 files at r4, 1 of 3 files at r5, 1 of 5 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

[ArniStarkware]

crates/gateway/src/gateway.rs line 198 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

I renamed the function and added the required documentation.

Look for into_contract_class_for_compilation in utils.rs in this PR.

[dafnamatsry]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @Yael-Starkware)

[Yael-Starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)