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(nargo): Remove unnecessary Brillig oracles used for testing #1911

Closed
vezenovm opened this issue Jul 11, 2023 · 3 comments · Fixed by #3613
Closed

feat(nargo): Remove unnecessary Brillig oracles used for testing #1911

vezenovm opened this issue Jul 11, 2023 · 3 comments · Fixed by #3613
Assignees
Labels
discussion enhancement New feature or request nargo Noir's CLI development tool refactor

Comments

@vezenovm
Copy link
Contributor

Problem

While iterating on the Brillig gen in the SSA refactor, oracles have been included that do not act as finalized builtins but are used entirely for tests. For example the get_number_sequence oracle in this PR (#1909) is unnecessary as an oracle.

Happy Case

We should finalize a few of these oracles as builtins in Noir's stdlib, specifically oracle print. Currently get_number_sequence and get_reverse_number_sequence are useful as tests while we continue finalizing the Brillig gen for slices. Once, we reach a point that we are comfortable with slices in Brillig (or have a Brillig foreign call builtin that returns a slice) we should also remove these functions from the tests. We currently have one brillig_oracle test under test_data_ssa_refactor that checks all oracle operations. Ideally we can have integration tests for specific builtin stdlib oracles like debug_log, get_notes, etc., and get rid of the brillig_oracle test on its own.

Alternatives Considered

No response

Additional Context

Some of the work as part of this issue (#1910) and the logging refactor should remove some of these test oracles

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@vezenovm
Copy link
Contributor Author

These oracles don't do anything but test our ability to return a slice from a foreign call. Until nargo has builtins of its own that require foreign calls that return a slice we would essentially be removing a test for this functionality, thus I am wary to remove them at this moment.

@TomAFrench as part of issue #2013 you have been considering how to rework our test suite. What do you think about enabling some foreign calls only during our testing?

@TomAFrench
Copy link
Member

What do you think about enabling some foreign calls only during our testing?

My question on this is aren't we expecting users to define their own oracle calls and expect them to be able to resolve during execution with nargo CLI? This seems to go against having a restrictive enum with a fixed number of oracle types.

If we remove the enum then this issue goes away, alternatively we can have a freeform entry in the enum which will go out to an external solver (which these tests would then be testing a reference impl of)

@jfecher
Copy link
Contributor

jfecher commented Jul 25, 2023

I do think having some more separation between Oracle (that we're expected to implement ourselves somewhere in the compiler) and Oracle (that is expected to be a result of users adding their own custom handling) would be nice. We should probably give these concepts distinct names as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request nargo Noir's CLI development tool refactor
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants