Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add util starknet-sierra-compile #35

Merged
merged 1 commit into from
May 29, 2024

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Apr 9, 2024

Replacing #24


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 52.88%. Comparing base (17a76e4) to head (495f5b5).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/starknet_sierra_compile/src/compile.rs 88.23% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   52.20%   52.88%   +0.68%     
==========================================
  Files          22       23       +1     
  Lines         883      900      +17     
  Branches      883      900      +17     
==========================================
+ Hits          461      476      +15     
  Misses        398      398              
- Partials       24       26       +2     

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

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOICEEE 💪

Reviewable status: 0 of 8 files reviewed, 15 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)


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

// TODO(Arni, 02/04/2024): Make the file structure in this repo consistent.
// Folders.
pub mod compiler;

I think this should be a separate crate, it isn't really related to the gateway.

In fact, if this works out alright, perhaps the compiler might be interested in taking charge of it and importing it there.


crates/gateway/src/compiler/compile.rs line 6 at r1 (raw file):

use thiserror::Error;

// TODO(Arni, 1/05/2024): Remove the dependancy on anyhow. Run in thread.

Already using thread 🤔
WDYM?

Code quote:

// TODO(Arni, 1/05/2024): Remove the dependancy on anyhow. Run in thread.

crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

}

fn starknet_sierra_compile(

Consider moving down.

We haven't talked code conventions yet, but in the blockifier we used the rust-analyzer recommendation of ordering by importance, and in particular, public before private.

Yr call until we decide on a standard though.

Code quote:

fn starknet_sierra_compile(

crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

        add_pythonic_hints,
        max_bytecode_size,
    } = compilation_args;

Maybe use them from the instance?
Not sure at all, whichever looks nicer, yr call

Code quote:

    let SierraToCasmCompliationArgs {
        allowed_libfuncs_list_name,
        allowed_libfuncs_list_file,
        add_pythonic_hints,
        max_bytecode_size,
    } = compilation_args;

crates/gateway/src/compiler/compile.rs line 72 at r1 (raw file):

    } = serde_json::from_str(
        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,

Will this require writing the file to disk before calling the function?
Can we get around it by passing the program itself as an argument? or will this break a core assumption of the compiler and cause trouble?

Code quote:

        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,

crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,
    )
    .map_err(|_| CompilationUtilError::DeserializationError)?;

Maybe break into two statements, this is just a bit hard to follow

Code quote:

    let ContractClassIgnoreAbi {
        sierra_program,
        sierra_program_debug_info,
        contract_class_version,
        entry_points_by_type,
        _abi,
    } = serde_json::from_str(
        &fs::read_to_string(file)
            .map_err(|_| CompilationUtilError::ReadInputError { file: file.into() })?,
    )
    .map_err(|_| CompilationUtilError::DeserializationError)?;

crates/gateway/src/compiler/compile.rs line 101 at r1 (raw file):

    };

    let sierra_path_clone = sierra_path.to_string(); // Clone sierra_path

arg name is good

Suggestion:

   let sierra_path_clone = sierra_path.to_string();

crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

    let sierra_path_clone = sierra_path.to_string(); // Clone sierra_path
    let handle = thread::spawn(move || {
        catch_unwind(AssertUnwindSafe(move || {

Are you sure we have to use this?
I think it is generally recommended to avoid using it directly (paragraph 4 here)

IIUC rust handles catching the panics out of the box.
At least, I know tokio does this out of the box for spawn_blocking.

Code quote:

   catch_unwind(AssertUnwindSafe(move || {

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

    });

    let result = handle.join().expect("Failed to join thread");

What are the possible errors here?

Can you add a test for a panicy compilation?
I wanna be sure (and make it evident for readers) that the panic is indeed caught in the result below, and not in the result that join returns.

Code quote:

    let result = handle.join().expect("Failed to join thread");

crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

    });

    let result = handle.join().expect("Failed to join thread");

Also:

  1. add tokio to cargo.toml :P
  2. add async to the function sig
  3. make the test #[tokio::test]

Also plz add a TODO(task_executor) so we'll replace it by calling the (soon to be merged hopefully 🥺 ) task executor.

Rationale: the task executor will use this tokio method for this, so this will behave more similar to it.
(Not using rayon for CPU just yet, we're keeping things simple since we also have IO to think about so let tokio deal with it all.)

spawn_blocking offloads the task to a threadpool, as opposed to std threads, which just run it with impunity (which can starve out other stuff like IO).

BTW, RE the std::thread implementation, I'm not sure the unwind things was necessary even with std threads; I think the panic would just have been caught at join.expect(), as opposed to the match below, but I'm not sure.

Suggestion:

    let result = tokio::task::spawn_blocking(move || {
        starknet_sierra_compile(compilation_args, &sierra_path_clone)    
    }).await;

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

Add a test for this plz, I don’t quite believe in that slick new panic-catching gizmo just yet, y’all.

Code quote:

        Err(e) => {
            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

            // A panic here might be a feature.
            panic!("Compilation panicked: {:?}", e)
        }

I think we can do this if we know that the compilation panic is due to faulty input from the user (and not due to some bug on our end), is that the case?

If we can't, you may also consider let-else pattern to reduce nesting, I think it might fit nicely here.

Suggestion:

let compilation_result = result?;

crates/gateway/src/compiler/compile.rs line 123 at r1 (raw file):

            }
            CompilationUtilError::StarknetSierraCompilationError(_) => {
                unimplemented!("The user writes a contract that fails to compile to Starknet casm")

unimplemented --> wont fix
todo --> will fix.

Ref: Third paragraph from the top here

Suggestion:

                todo!("The user provids a badly formatted Sierra file.")
            }
            CompilationUtilError::AllowedLibfuncsError(_) => {
                todo!("The user writes a contract using libfuncs that are not allowed.")
            }
            CompilationUtilError::StarknetSierraCompilationError(_) => {
                todo!("The user writes a contract that fails to compile to Starknet casm")

crates/gateway/src/compiler/compile.rs line 130 at r1 (raw file):

            }
        },
        Ok(Ok(casm_contract)) => serde_json::to_string_pretty(&casm_contract)

This is for nesting?

Also, is it necessary for the actual flow? or just for debugging?
Asking cause this is a relatively expensive op for big contracts.

Code quote:

serde_json::to_string_pretty

crates/gateway/test/fixtures/compiler/account_faulty.sierra.json line 2 at r1 (raw file):

{
  "sierra_program": [

Should be in gateway/tests/fixtures/
Don't need compiler namespacing I think, might be useful for others.
testsis the canonical location for this I think

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 8 files reviewed, 16 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)


crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

Previously, giladchase wrote…

Consider moving down.

We haven't talked code conventions yet, but in the blockifier we used the rust-analyzer recommendation of ordering by importance, and in particular, public before private.

Yr call until we decide on a standard though.

removed block 😅


crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

Previously, giladchase wrote…

Maybe use them from the instance?
Not sure at all, whichever looks nicer, yr call

removed block 😅


crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

Previously, giladchase wrote…

Maybe break into two statements, this is just a bit hard to follow

removed block 😅


crates/gateway/src/compiler/compile_test.rs line 7 at r1 (raw file):

    let sierra_path = "test/fixtures/compiler/account_faulty.sierra.json";
    let casm = compile_sierra_to_casm(sierra_path);
    assert_eq!(casm.len(), 72304);

Will this length change in between compiler releases?
We don't this test to break whenever we bump a version unless we have to.

Also plz extract to a variable expected_casm_program_len.

Code quote:

    assert_eq!(casm.len(), 72304);

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 9f0e0d6 to b3eb3c8 Compare April 15, 2024 14:37
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 13 files reviewed, 14 unresolved discussions (waiting on @dafnamatsry and @giladchase)


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

Previously, giladchase wrote…

I think this should be a separate crate, it isn't really related to the gateway.

In fact, if this works out alright, perhaps the compiler might be interested in taking charge of it and importing it there.

Done.

About Asking the compilers to take charge of this part: Sounds great and more sound. The biggest hurdle is error handling as I see it.


crates/gateway/src/compiler/compile.rs line 6 at r1 (raw file):

Previously, giladchase wrote…

Already using thread 🤔
WDYM?

Removed.


crates/gateway/src/compiler/compile.rs line 52 at r1 (raw file):

Previously, giladchase wrote…

removed block 😅

Done.


crates/gateway/src/compiler/compile.rs line 61 at r1 (raw file):

Previously, giladchase wrote…

removed block 😅

Done.


crates/gateway/src/compiler/compile.rs line 72 at r1 (raw file):

Previously, giladchase wrote…

Will this require writing the file to disk before calling the function?
Can we get around it by passing the program itself as an argument? or will this break a core assumption of the compiler and cause trouble?

I think we can get around this issue.

I moved this part of the code to a test util. I redefined the function so that it works on a ContractClass. We can later see how we want to use it in practice.


crates/gateway/src/compiler/compile.rs line 74 at r1 (raw file):

Previously, giladchase wrote…

removed block 😅

Moved to another file.
I aimed to move this usage to a test util for this PR. Maybe deal with this issue as a feature in future PR if needed.


crates/gateway/src/compiler/compile.rs line 101 at r1 (raw file):

Previously, giladchase wrote…

arg name is good

Done.


crates/gateway/src/compiler/compile.rs line 123 at r1 (raw file):

Previously, giladchase wrote…

unimplemented --> wont fix
todo --> will fix.

Ref: Third paragraph from the top here

Done.


crates/gateway/src/compiler/compile_test.rs line 7 at r1 (raw file):

Previously, giladchase wrote…

Will this length change in between compiler releases?
We don't this test to break whenever we bump a version unless we have to.

Also plz extract to a variable expected_casm_program_len.

IIUC - the length should not change between compiler releases. This is part of the backward compatibility of compilation to casm.

extract to a variable: Done.


crates/gateway/test/fixtures/compiler/account_faulty.sierra.json line 2 at r1 (raw file):

Previously, giladchase wrote…

Should be in gateway/tests/fixtures/
Don't need compiler namespacing I think, might be useful for others.
testsis the canonical location for this I think

Done.
Moved with the rest of the crate.

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch 2 times, most recently from 6682657 to 834a0eb Compare April 16, 2024 14:05
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 13 files reviewed, 14 unresolved discussions (waiting on @dafnamatsry and @giladchase)


crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

Previously, giladchase wrote…

Are you sure we have to use this?
I think it is generally recommended to avoid using it directly (paragraph 4 here)

IIUC rust handles catching the panics out of the box.
At least, I know tokio does this out of the box for spawn_blocking.

Now using tokio.


crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

Previously, giladchase wrote…

Also:

  1. add tokio to cargo.toml :P
  2. add async to the function sig
  3. make the test #[tokio::test]

Also plz add a TODO(task_executor) so we'll replace it by calling the (soon to be merged hopefully 🥺 ) task executor.

Rationale: the task executor will use this tokio method for this, so this will behave more similar to it.
(Not using rayon for CPU just yet, we're keeping things simple since we also have IO to think about so let tokio deal with it all.)

spawn_blocking offloads the task to a threadpool, as opposed to std threads, which just run it with impunity (which can starve out other stuff like IO).

BTW, RE the std::thread implementation, I'm not sure the unwind things was necessary even with std threads; I think the panic would just have been caught at join.expect(), as opposed to the match below, but I'm not sure.

Done.


crates/gateway/src/compiler/compile.rs line 108 at r1 (raw file):

Previously, giladchase wrote…

What are the possible errors here?

Can you add a test for a panicy compilation?
I wanna be sure (and make it evident for readers) that the panic is indeed caught in the result below, and not in the result that join returns.

Post-change - the only possible error is a JoinError: see.


crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

Previously, giladchase wrote…

I think we can do this if we know that the compilation panic is due to faulty input from the user (and not due to some bug on our end), is that the case?

If we can't, you may also consider let-else pattern to reduce nesting, I think it might fit nicely here.

Not sure I understand. Anyway - the original line of code was changed.


crates/gateway/src/compiler/compile.rs line 114 at r1 (raw file):

Previously, giladchase wrote…

Add a test for this plz, I don’t quite believe in that slick new panic-catching gizmo just yet, y’all.

Added a negative flow test.

Did not add a JoinResult is panic! test yet. Added a TODO, We should start a new thread in a more appropriate location.


crates/gateway/src/compiler/compile.rs line 130 at r1 (raw file):

Previously, giladchase wrote…

This is for nesting?

Also, is it necessary for the actual flow? or just for debugging?
Asking cause this is a relatively expensive op for big contracts.

Now not a part of the actual flow.

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 834a0eb to 70454e4 Compare April 18, 2024 07:45
@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch 4 times, most recently from b4e0e9a to 08f8a4f Compare May 12, 2024 12:45
@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 08f8a4f to 535a9fc Compare May 16, 2024 07:12
Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 13 files at r2, 2 of 4 files at r6, all commit messages.
Reviewable status: 3 of 13 files reviewed, all discussions resolved (waiting on @dafnamatsry)


crates/starknet_sierra_compile/Cargo.toml line 2 at r6 (raw file):

[package]
name = "starknet_sierra_compile"

Note: Naming might be confusing, as it is too similar to cairo-lang-sierra, but we aren't publishing this crate anytime soon, let's worry about it later :)

Code quote:

name = "starknet_sierra_compile"

crates/starknet_sierra_compile/src/lib.rs line 1 at r6 (raw file):

pub mod compile;

Maybe add tiny module-docstring here:

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

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

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

About Asking the compilers to take charge of this part: Sounds great and more sound. The biggest hurdle is error handling as I see it.

Agreed. The thing about the compiler was meant as a "some-day 🥺" kind of thing , so no need to worry about it


crates/gateway/src/compiler/compile.rs line 103 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Now using tokio.

🍣

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 13 files at r2, 2 of 6 files at r3, 2 of 4 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 535a9fc to a7a952b Compare May 21, 2024 07:32
@ArniStarkware ArniStarkware requested a review from giladchase May 21, 2024 07:33
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 13 files reviewed, all discussions resolved (waiting on @dafnamatsry and @giladchase)


crates/starknet_sierra_compile/src/lib.rs line 1 at r6 (raw file):

Previously, giladchase wrote…

Maybe add tiny module-docstring here:

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

Done.

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from a7a952b to c5f4b50 Compare May 22, 2024 11:05
Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @orizi

Reviewed 6 of 13 files at r2, 2 of 6 files at r3, 2 of 4 files at r6, 1 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ArniStarkware)


crates/starknet_sierra_compile/src/compile.rs line 13 at r8 (raw file):

pub mod compile_test;

pub struct SierraToCasmCompliationArgs {

Suggestion:

SierraToCasmCompilationArgs

crates/starknet_sierra_compile/src/compile.rs line 15 at r8 (raw file):

pub struct SierraToCasmCompliationArgs {
    allowed_libfuncs_list_name: Option<String>,
    allowed_libfuncs_list_file: Option<String>,

IIUC only one should be supplied.
In that case, please make it an enum.

Also, which one are we going to use? Maybe we should only allow one of them...

Code quote:

    allowed_libfuncs_list_name: Option<String>,
    allowed_libfuncs_list_file: Option<String>,

crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

    // TODO(task_executor).
    tokio::task::spawn_blocking(move || starknet_sierra_compile(compilation_args, contract_class))

I think managing the compilation inside a thread should be outside this util.

This will be run in the context of another spawned thread, so I'm not sure we should spawn another thread for it.
Let's discuss f2f.

Code quote:

tokio::task::spawn_blocking

crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

    // TODO(task_executor).
    tokio::task::spawn_blocking(move || starknet_sierra_compile(compilation_args, contract_class))
        .await?

What is returned in case of a panic? Does it catch it?
Please document.

Code quote:

.await?

crates/starknet_sierra_compile/src/compile_test.rs line 45 at r8 (raw file):

        entry_points_by_type,
        abi,
    };

Suggestion:

    let mut contract_class = contract_class_from_file(sierra_path);
    contract_class.sierra_program = sierra_program[..100].to_vec();
 

crates/starknet_sierra_compile/src/compile_test.rs line 53 at r8 (raw file):

    } else {
        panic!("Unexpected error.")
    }

Use assert_matches!

Code quote:

    if let CompilationUtilError::AllowedLibfuncsError(AllowedLibfuncsError::SierraProgramError) =
        result.unwrap_err()
    {
        return;
    } else {
        panic!("Unexpected error.")
    }

crates/starknet_sierra_compile/src/lib.rs line 1 at r8 (raw file):

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

Suggestion:

//! A lib for compiling Sierra into Casm.

crates/starknet_sierra_compile/src/test_utils.rs line 10 at r8 (raw file):

/// See: https://github.com/starkware-libs/cairo/blob/d0c0f175a8855242d8c6265c55d3f97f8dfdce40/crates/bin/starknet-sierra-compile/src/main.rs#L34-L43
/// Same as `ContractClass` - but ignores `abi` in deserialization.
/// Enables loading old contract classes.

Suggestion:

/// Same as `ContractClass` - but ignores unnecessary fields like `abi` in deserialization.
/// Enables loading old contract classes.

crates/starknet_sierra_compile/src/test_utils.rs line 12 at r8 (raw file):

/// Enables loading old contract classes.
#[derive(Deserialize)]
struct ContractClassIgnoreAbi {

Suggestion:

DeserializedContractClass

crates/starknet_sierra_compile/src/test_utils.rs line 17 at r8 (raw file):

    pub contract_class_version: String,
    pub entry_points_by_type: ContractEntryPoints,
    pub _abi: Option<serde_json::Value>,

I think you can just delete this.

Code quote:

pub _abi: Option<serde_json::Value>,

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from c5f4b50 to 91a1e42 Compare May 27, 2024 13:04
@ArniStarkware ArniStarkware requested a review from dafnamatsry May 27, 2024 13:18
@ArniStarkware ArniStarkware requested a review from giladchase May 27, 2024 13:18
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @giladchase)


crates/starknet_sierra_compile/src/compile.rs line 13 at r8 (raw file):

pub mod compile_test;

pub struct SierraToCasmCompliationArgs {

Done.


crates/starknet_sierra_compile/src/compile.rs line 15 at r8 (raw file):

Previously, dafnamatsry wrote…

IIUC only one should be supplied.
In that case, please make it an enum.

Also, which one are we going to use? Maybe we should only allow one of them...

Done.


crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Previously, dafnamatsry wrote…

I think managing the compilation inside a thread should be outside this util.

This will be run in the context of another spawned thread, so I'm not sure we should spawn another thread for it.
Let's discuss f2f.

We use the thread here because this is our way to catch panics.

But I see what you are saying.
We can export the function compile_sierra_to_casm and mark (through documentation) as a function that might panic.

Let use discuss f2f.


crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

Previously, dafnamatsry wrote…

What is returned in case of a panic? Does it catch it?
Please document.

Done.
I think I was over-explicit.


crates/starknet_sierra_compile/src/compile_test.rs line 45 at r8 (raw file):

        entry_points_by_type,
        abi,
    };

Done.


crates/starknet_sierra_compile/src/compile_test.rs line 53 at r8 (raw file):

Previously, dafnamatsry wrote…

Use assert_matches!

Done.


crates/starknet_sierra_compile/src/lib.rs line 1 at r8 (raw file):

//! An experimental attempt to compile Sierra into Casm by using cairo-lang as a lib .

Done.


crates/starknet_sierra_compile/src/test_utils.rs line 10 at r8 (raw file):

/// See: https://github.com/starkware-libs/cairo/blob/d0c0f175a8855242d8c6265c55d3f97f8dfdce40/crates/bin/starknet-sierra-compile/src/main.rs#L34-L43
/// Same as `ContractClass` - but ignores `abi` in deserialization.
/// Enables loading old contract classes.

Done.


crates/starknet_sierra_compile/src/test_utils.rs line 12 at r8 (raw file):

/// Enables loading old contract classes.
#[derive(Deserialize)]
struct ContractClassIgnoreAbi {

Done.


crates/starknet_sierra_compile/src/test_utils.rs line 17 at r8 (raw file):

Previously, dafnamatsry wrote…

I think you can just delete this.

Done.

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 91a1e42 to 0bbd647 Compare May 27, 2024 13:26
@ArniStarkware ArniStarkware requested a review from orizi May 28, 2024 08:41
@ArniStarkware
Copy link
Contributor Author

+reviewer:@orizi - using CasmContractClass::from_contract_class in the gateway-mempool.

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @orizi)


crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

We use the thread here because this is our way to catch panics.

But I see what you are saying.
We can export the function compile_sierra_to_casm and mark (through documentation) as a function that might panic.

Let use discuss f2f.

Adding on what dafna said: we'll soon limit all of our spawning to spawns from the executor, so if we spawn here we'll have to pass the executor as an argument into this API, which might be considered out-of-scope of what this crate should know about.

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 6 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 12 of 13 files reviewed, 2 unresolved discussions (waiting on @giladchase and @orizi)

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from 0bbd647 to e5372a4 Compare May 28, 2024 13:03
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)


crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Previously, giladchase wrote…

Adding on what dafna said: we'll soon limit all of our spawning to spawns from the executor, so if we spawn here we'll have to pass the executor as an argument into this API, which might be considered out-of-scope of what this crate should know about.

Done.

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/skelaton branch from e5372a4 to 495f5b5 Compare May 28, 2024 13:06
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)


crates/starknet_sierra_compile/src/compile.rs line 43 at r8 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

Now not using threads and tokio at all in this PR.

Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @giladchase, and @orizi)


crates/starknet_sierra_compile/src/compile.rs line 44 at r8 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.
I think I was over-explicit.

No longer relevant.

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @orizi)

@ArniStarkware ArniStarkware merged commit ac5a2fd into main May 29, 2024
8 checks passed
@ArniStarkware ArniStarkware deleted the arni/sierra_to_casm/skelaton branch May 29, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants