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.

Sign up for GitHub

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

Jump to bottom

FEAT(stark252 field): Adds Stark252 curve #494

Merged
merged 9 commits into from
May 1, 2024
Merged

FEAT(stark252 field): Adds Stark252 curve #494

merged 9 commits into from
May 1, 2024

Conversation

PatStiles
Copy link
Contributor

Describe the changes

Rebase of #493 after merging of V2.

@jeremyfelder
Copy link
Collaborator

@PatStiles Thanks for adding this! 🚀 👍🏻
Can you format the Rust and Cpp code according to https://github.com/ingonyama-zk/icicle?tab=readme-ov-file#development-contributions

@PatStiles
Copy link
Contributor Author

@jeremyfelder I'm having trouble addressing the failing cargo test in the CI which is failing due to a lack of ArkField declarationfor stark252 (arkworks does not implement it). Out of curiosity I ran the same test for icicle-babybear and saw it also failed within that crate. Let me know if you have any suggestions for addressing the issue.
Screenshot 2024-04-25 at 15 05 21

@jeremyfelder
Copy link
Collaborator

@jeremyfelder I'm having trouble addressing the failing cargo test in the CI which is failing due to a lack of ArkField declarationfor stark252 (arkworks does not implement it). Out of curiosity I ran the same test for icicle-babybear and saw it also failed within that crate. Let me know if you have any suggestions for addressing the issue. Screenshot 2024-04-25 at 15 05 21

The CI tests for stark252 need to be separated out like the babybear tests; https://github.com/PatStiles/icicle/blob/feat/stark252_field/.github/workflows/rust.yml#L64-L73.

jeremyfelder
jeremyfelder requested changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@jeremyfelder jeremyfelder left a comment

Choose a reason for hiding this comment

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

@PatStiles Looks great!
Can you add a build.rs file for stark252 to link to the static lib. Similar to babybear's but with the EXT_FIELD = OFF

scripts/gen_c_api.py Show resolved Hide resolved
@PatStiles
Copy link
Contributor Author

@jeremyfelder addressed!

@PatStiles
Copy link
Contributor Author

@ChickenLover Should be resolved!

@jeremyfelder jeremyfelder self-requested a review April 28, 2024 01:38
jeremyfelder
jeremyfelder requested changes Apr 28, 2024
View reviewed changes
Copy link
Collaborator

@jeremyfelder jeremyfelder left a comment

Choose a reason for hiding this comment

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

I think this should be the last change

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@jeremyfelder jeremyfelder merged commit bdc3da9 into ingonyama-zk:main May 1, 2024
24 checks passed
yshekel pushed a commit that referenced this pull request May 19, 2024
@PatStiles @yshekel
FEAT(stark252 field): Adds Stark252 curve (#494)
c329559 
## Describe the changes

Adds support for the stark252 base field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChickenLover ChickenLover ChickenLover approved these changes

@jeremyfelder jeremyfelder jeremyfelder approved these changes

@LeonHibnik LeonHibnik LeonHibnik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PatStiles @jeremyfelder @ChickenLover @LeonHibnik