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: SECP related hints #1829

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

odesenfans
Copy link
Contributor

Context: port of the Starknet OS to Rust.

We need an implementation of the hints used by the Starknet OS in the SECP syscalls. These hints rely on private primitives in cairo-vm and must be implemented in this repository.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

Problem: we need an implementation of the hints used by the Starknet OS
in the secp syscalls. These hints rely on private primitives in
`cairo-vm` and need to be implemented here.

Solution: this PR adds an implementation of all the hints that require
`cairo-vm` primitives in the `cairo-vm` repository.
@pefontana
Copy link
Member

Hi @odesenfans !!
Sorry the delay, we didn't saw this PR
Can you merge to main and solve the conflicts?

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 57.26950% with 241 lines in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (63b4a55) to head (5337a25).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...int_processor/builtin_hint_processor/secp/hints.rs 64.57% 169 Missing ⚠️
...int_processor/builtin_hint_processor_definition.rs 14.28% 72 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   94.83%   94.31%   -0.53%     
==========================================
  Files         101      102       +1     
  Lines       39579    40142     +563     
==========================================
+ Hits        37536    37858     +322     
- Misses       2043     2284     +241     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@pefontana
Copy link
Member

@odesenfans some more points:

  • Since the VM is only allowed to run whitelisted hints in production we cant enable the VM to execute this hints in production. There are two ways to solve this, leaving the hints under a feature flag like cairo-0-secp-hints`, the other way is that you implement your own hint processor in your repo
  • There are some lints error in the CI
  • There are some unwraps where you can handle the error, like the pop().unwrap(); and some more cases
  • Can you add some integration tests for the new hints?

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