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: declare post compilation prime validation #297

Closed

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Jun 27, 2024

This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Jun 27, 2024

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

Join @ArniStarkware and the rest of your teammates on Graphite Graphite

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.56%. Comparing base (9852d18) to head (04deeb9).

Files Patch % Lines
crates/gateway/src/compilation.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #297   +/-   ##
=======================================
  Coverage   83.55%   83.56%           
=======================================
  Files          37       37           
  Lines        1770     1777    +7     
  Branches     1770     1777    +7     
=======================================
+ Hits         1479     1485    +6     
- Misses        212      213    +1     
  Partials       79       79           

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

Copy link
Contributor

@Yael-Starkware Yael-Starkware 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 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)


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

        return Err(GatewayError::InvalidPrime { prime, expected_prime });
    }

Can you explain what this check is? what is the meaning of prime here?

Copy link
Contributor

@Yael-Starkware Yael-Starkware 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 5 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)


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

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

remove.

Code quote:

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

@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from 332ec2c to c5b052a Compare June 27, 2024 11:07
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from 9ee212a to 46e9d96 Compare June 27, 2024 11:07
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 5 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)


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

Previously, Yael-Starkware (YaelD) wrote…

remove.

Done.

@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from c5b052a to f342117 Compare June 27, 2024 15:41
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from 46e9d96 to df78ad6 Compare June 27, 2024 15:41
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from f342117 to e6bdf01 Compare June 30, 2024 08:50
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from df78ad6 to 9089147 Compare June 30, 2024 09:13
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from e6bdf01 to e82f53b Compare June 30, 2024 09:14
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from 9089147 to f3747cf Compare June 30, 2024 09:14
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from e82f53b to f67b511 Compare June 30, 2024 09:26
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from f3747cf to b2d444a Compare June 30, 2024 09:26
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from f67b511 to c2d41e8 Compare July 1, 2024 08:31
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from b2d444a to a179aaf Compare July 1, 2024 08:31
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from c2d41e8 to d0d4053 Compare July 1, 2024 11:34
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from a179aaf to 093d9d0 Compare July 1, 2024 11:34
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from d0d4053 to ab76a25 Compare July 1, 2024 14:15
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from 093d9d0 to f36fd0f Compare July 1, 2024 14:18
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/builtins branch from ab76a25 to 0c30c7a Compare July 2, 2024 08:36
@ArniStarkware ArniStarkware changed the base branch from arni/declare/post_compilation/validation/builtins to main July 2, 2024 12:33
@ArniStarkware
Copy link
Contributor Author

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

Previously, Yael-Starkware (YaelD) wrote…

Can you explain what this check is? what is the meaning of prime here?

This was first referenced here: https://github.com/starkware-industries/starkware/pull/17777

@ArniStarkware
Copy link
Contributor Author

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

Previously, ArniStarkware (Arnon Hod) wrote…

This was first referenced here: https://github.com/starkware-industries/starkware/pull/17777

One step back. This validation was copied from the Python code.

@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from 7dc1d09 to fe9d1f4 Compare July 3, 2024 08:45
@ArniStarkware ArniStarkware changed the base branch from main to arni/declare/compilation/compilation_file July 3, 2024 08:45
@ArniStarkware ArniStarkware force-pushed the arni/declare/compilation/compilation_file branch from 2546a4b to 6a332d4 Compare July 3, 2024 09:12
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from fe9d1f4 to bddbf77 Compare July 3, 2024 09:12
@ArniStarkware ArniStarkware force-pushed the arni/declare/compilation/compilation_file branch from 6a332d4 to 0f229c0 Compare July 3, 2024 10:03
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from bddbf77 to cc45be8 Compare July 3, 2024 10:04
@ArniStarkware ArniStarkware force-pushed the arni/declare/compilation/compilation_file branch from 0f229c0 to 93e2b1b Compare July 3, 2024 12:02
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch 2 times, most recently from 8d7f983 to cde907a Compare July 3, 2024 12:34
@ArniStarkware ArniStarkware changed the base branch from arni/declare/compilation/compilation_file to main July 3, 2024 12:34
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from cde907a to 151c4c5 Compare July 4, 2024 13:41
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 2 of 6 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)


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

    let prime = contract_class.prime.clone();
    let expected_prime =
        BigUint::from_str_radix(&PRIME_STR[2..], 16).expect("Error parsing field prime.");

Can this be a constant?

Code quote:

BigUint::from_str_radix(&PRIME_STR[2..], 16).expect("Error parsing field prime.");

@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch 2 times, most recently from 6fff151 to 9bb8586 Compare July 14, 2024 07:25
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch 2 times, most recently from c834846 to cc10684 Compare July 14, 2024 14:30
@ArniStarkware ArniStarkware force-pushed the arni/declare/post_compilation/validation/prime branch from cc10684 to 04deeb9 Compare July 15, 2024 07:38
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: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @Yoni-Starkware)


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

Previously, ArniStarkware (Arnon Hod) wrote…

One step back. This validation was copied from the Python code.

@Yoni-Starkware agrees this validation is no longer needed. See: https://starkwareindustries.slack.com/archives/C03D7FDE1EZ/p1721116020563769

@ArniStarkware
Copy link
Contributor Author

This validation is not needed for Cairo 1 contracts.

The prime used during the compilation into CASM is independent of the transaction.

@ArniStarkware ArniStarkware deleted the arni/declare/post_compilation/validation/prime branch July 16, 2024 11:22
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