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

Implement circuits. #4

Merged
merged 19 commits into from
Aug 27, 2024
Merged

Implement circuits. #4

merged 19 commits into from
Aug 27, 2024

Conversation

azteca1998
Copy link
Collaborator

No description provided.

@FrancoGiachetta FrancoGiachetta marked this pull request as ready for review August 19, 2024 20:51
Copy link
Contributor

@JulianGCalderon JulianGCalderon left a comment

Choose a reason for hiding this comment

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

Nice Work @FrancoGiachetta!! I left you a few comments.

output5.sierra Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
src/value.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@JulianGCalderon JulianGCalderon left a comment

Choose a reason for hiding this comment

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

@FrancoGiachetta I left you some comments I missed in my first review

src/value.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
@FrancoGiachetta
Copy link
Contributor

Done!

Copy link
Contributor

@JulianGCalderon JulianGCalderon left a comment

Choose a reason for hiding this comment

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

The example I gave in the previous review is still failing:

let a = outputs.get_output(mul);
let b = a.limb1;
drop(b)

This is due to the bad BoundedInt range. Remember to test it again after fixing a bug, just to make sure!

Also, once you fix this, I recommend that you manually test the implementation with a more complex scenario, just to make sure it works. As an example, you can use this test from Cairo Native: https://github.com/lambdaclass/cairo_native/blob/8ee32eb38f1c86598033e493b2908cfb648b9205/src/libfuncs/circuit.rs#L1351

src/vm/circuit.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
src/vm/circuit.rs Outdated Show resolved Hide resolved
@FrancoGiachetta
Copy link
Contributor

Everything should be working now. I added the test you were taking about. I also downloaded the corelib so we can test the programs.

Copy link
Collaborator Author

@azteca1998 azteca1998 left a comment

Choose a reason for hiding this comment

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

You included Cairo 2.7.0 in the PR. I don't think it should be here.

@FrancoGiachetta
Copy link
Contributor

Isn't it required to be able to compile .cairo files? So we can test cairo programs which are not contracts.

@azteca1998
Copy link
Collaborator Author

Sure, but not in our repository. Just add a dependency. If we need the binary we can replicate what we do in native (aka. make deps that downloads stuff).

@FrancoGiachetta
Copy link
Contributor

Oh, I see. I'll remove it then

@FrancoGiachetta
Copy link
Contributor

Done!

@pefontana pefontana merged commit 5707a7b into main Aug 27, 2024
1 check passed
@pefontana pefontana deleted the impl-circuits branch August 27, 2024 23:19
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