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: use the command line compiler in gateway #599

Merged

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Aug 26, 2024

This change is Reviewable

@ArniStarkware ArniStarkware marked this pull request as ready for review August 26, 2024 13:00
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 4bc38ea to 610aec6 Compare August 26, 2024 13:06
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from efe31b3 to bcc1df0 Compare August 26, 2024 13:06
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 610aec6 to d14d3de Compare August 26, 2024 13:13
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from bcc1df0 to 73d32b7 Compare August 26, 2024 13:13
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.49%. Comparing base (d1aa365) to head (7868f5c).
Report is 6 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #599       +/-   ##
===========================================
- Coverage   76.62%   28.49%   -48.14%     
===========================================
  Files         351      209      -142     
  Lines       37223    23945    -13278     
  Branches    37223    23945    -13278     
===========================================
- Hits        28522     6823    -21699     
- Misses       6388    16375     +9987     
+ Partials     2313      747     -1566     

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

@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from d14d3de to 17e0fd5 Compare August 26, 2024 16:51
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 73d32b7 to efe83f3 Compare August 26, 2024 16:51
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 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)


crates/gateway/build.rs line 1 at r1 (raw file):

// Sets up the environment variable OUT_DIR.

Suggestion:

// Sets up the environment variable OUT_DIR, which holds the cairo compiler binary.
// The binary is dowloaded to OUT_DIR by the starknet_sierra_compile crate.

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

    stateful_validator: StatefulTransactionValidator,
) {
    let optional_class_info =

Can you add the optional_class_info as a case argument and set it to None?
i.e.:

Suggestion:

#[case::invalid_tx(invoke_tx(CairoVersion::Cairo1), Err(STATEFUL_VALIDATOR_FEE_ERROR)), None]
fn test_stateful_tx_validator(
    #[case] external_tx: RpcTransaction,
    #[case] expected_result: BlockifierStatefulValidatorResult<ValidateInfo>,
    #[case] class_info: Option<ClassInfo>,
    stateful_validator: StatefulTransactionValidator,
) {

crates/tests-integration/build.rs line 2 at r1 (raw file):

// Sets up the environment variable OUT_DIR. This is necessary for for the crate
// starknet_sierra_compile.

Suggestion:

// Sets up the environment variable OUT_DIR. This is necessary for for the crate
// starknet_sierra_compile to download the cairo compiler binary.

@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 17e0fd5 to d98265e Compare August 27, 2024 08:19
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from efe83f3 to 32566bd Compare August 27, 2024 08:20
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from d98265e to e3e4d61 Compare August 27, 2024 09:13
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 32566bd to ebd8ce3 Compare August 27, 2024 09:13
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: 3 of 7 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry)


crates/gateway/build.rs line 1 at r1 (raw file):

// Sets up the environment variable OUT_DIR.

Done.


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

Previously, dafnamatsry wrote…

Can you add the optional_class_info as a case argument and set it to None?
i.e.:

Done.
Even better.


crates/tests-integration/build.rs line 2 at r1 (raw file):

// Sets up the environment variable OUT_DIR. This is necessary for for the crate
// starknet_sierra_compile.

Done.

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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from e3e4d61 to b0708b1 Compare August 27, 2024 13:42
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from ebd8ce3 to 853b855 Compare August 27, 2024 13:43
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from b0708b1 to 052c771 Compare August 27, 2024 14:29
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 853b855 to e2d090a Compare August 27, 2024 14:30
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 052c771 to 6eb922f Compare August 28, 2024 02:44
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from e2d090a to 906ba9b Compare August 28, 2024 02:44
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 6eb922f to 4621f8f Compare August 28, 2024 06:41
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 906ba9b to 0b37d48 Compare August 28, 2024 06:41
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/create_compiler branch from 4621f8f to 1ee6ade Compare September 2, 2024 11:35
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 0b37d48 to 8ebcf11 Compare September 2, 2024 11:35
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 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

Copy link
Contributor Author

ArniStarkware commented Sep 3, 2024

Merge activity

@ArniStarkware ArniStarkware changed the base branch from arni/starknet_sierra_compile/zip/create_compiler to graphite-base/599 September 3, 2024 06:09
@ArniStarkware ArniStarkware changed the base branch from graphite-base/599 to main September 3, 2024 06:19
@ArniStarkware ArniStarkware force-pushed the arni/starknet_sierra_compile/zip/use_in_gateway branch from 8ebcf11 to 7868f5c Compare September 3, 2024 06:20
@ArniStarkware ArniStarkware merged commit f4af7b9 into main Sep 3, 2024
14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2024
@ArniStarkware ArniStarkware deleted the arni/starknet_sierra_compile/zip/use_in_gateway branch September 22, 2024 11:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants