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

Support ALL circom InfixOp #59

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Support ALL circom InfixOp #59

merged 8 commits into from
Jun 4, 2024

Conversation

voltrevo
Copy link
Collaborator

@voltrevo voltrevo commented May 15, 2024

Resolves #54.

This branch relies on an update to sim-circuit, so you might need to cargo update to make it work.

Adds all infix ops

Notably, this includes AIntDiv which is possibly different from ADiv. For our purposes, division generally means modular division. To reduce confusion, we should perhaps take the position that it always means modular division, since explicit integer division is now available if that's the intention. Currently sim-circuit implements division as integer division, so the two operations are the same.

In circom a / b is (modular) division, and a \ b is integer division.

Discord discussion thread.

Fixes running & debugging individual tests in VS Code (and possibly other IDEs)

Prior to this PR, running a test involved command line argument processing because building a circuit requires an Input, and Input::new reads the command line. This breaks VS Code's functionality for running individual tests because it passes arguments that are unrecognized by that system, which is intended for the CLI, not tests.

I was similarly caught by this issue in the wasm-support branch (demo here), since there is no command line in the browser. The changes to input.rs here are based on the work from that branch, so this will reduce the changes needed later for wasm support.

Now Input::new is a pure function (well except maybe some platform-specific Path gotchas), and main.rs instead uses input_processing::generate_input, which replicates the old behavior (including command line processing).

Possible underconstrained bug

During testing, I was struck by the way we ignore signal outputs that are not assigned any values. I added this test:

pragma circom 2.0.0;

template underConstrained() {
    signal output x;
}

component main = underConstrained();

Currently this just outputs vec![]. I'm guessing it should be an error instead, but I left it as a TODO.

Add test for x == x fixed by sim-circuit update

My test for all infix ops contains several examples of using the same input on both sides of the op. x == x may be a little weird but it's perfectly valid and meaningful in other cases like x * x. sim-circuit was incorrectly generating CyclicDependencyDetected for this, so I added a more specific test that covers the issue.

@voltrevo voltrevo marked this pull request as ready for review May 16, 2024 00:07
Copy link
Collaborator

@brech1 brech1 left a comment

Choose a reason for hiding this comment

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

LGTM

@voltrevo voltrevo merged commit 5acb088 into main Jun 4, 2024
1 check passed
@voltrevo voltrevo deleted the issue-54-all-infix-ops branch June 4, 2024 03:42
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.

Support ALL circom InfixOp
2 participants