-
Notifications
You must be signed in to change notification settings - Fork 304
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!: Wrapped unconstrained #6751
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… gj/wrapped_unconstrained
… gj/wrapped_unconstrained
TomAFrench
reviewed
Jun 6, 2024
Comment on lines
1
to
+24
#[builtin(is_unconstrained)] | ||
pub fn is_unconstrained() -> bool {} | ||
|
||
#[builtin(assert_unconstrained)] | ||
pub fn assert_unconstrained() {} | ||
|
||
struct Unconstrained<T> { | ||
_value: T, | ||
} | ||
|
||
impl<T> Unconstrained<T> { | ||
fn new(value: T) -> Self { | ||
Self { _value: value } | ||
} | ||
|
||
fn make_constrained<Env, P>(self, constrainer: fn[Env](T) -> P) -> P { | ||
constrainer(self._value) | ||
} | ||
|
||
fn unwrap(self) -> T { | ||
assert_unconstrained(); | ||
self._value | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
#[builtin(is_unconstrained)] | |
pub fn is_unconstrained() -> bool {} | |
#[builtin(assert_unconstrained)] | |
pub fn assert_unconstrained() {} | |
struct Unconstrained<T> { | |
_value: T, | |
} | |
impl<T> Unconstrained<T> { | |
fn new(value: T) -> Self { | |
Self { _value: value } | |
} | |
fn make_constrained<Env, P>(self, constrainer: fn[Env](T) -> P) -> P { | |
constrainer(self._value) | |
} | |
fn unwrap(self) -> T { | |
assert_unconstrained(); | |
self._value | |
} | |
} | |
#[builtin(is_unconstrained)] | |
pub fn is_unconstrained() -> bool {} | |
struct Unconstrained<T> { | |
_value: T, | |
} | |
impl<T> Unconstrained<T> { | |
fn new(value: T) -> Self { | |
Self { _value: value } | |
} | |
fn make_constrained<Env, P>(self, constrainer: fn[Env](T) -> P) -> P { | |
constrainer(self._value) | |
} | |
fn unwrap(self) -> T { | |
assert(is_unconstrained(), "User friendly error message"); | |
self._value | |
} | |
} |
5 tasks
github-merge-queue bot
pushed a commit
to noir-lang/noir
that referenced
this pull request
Jun 6, 2024
# Description ## Problem\* Resolves <!-- Link to GitHub Issue --> ## Summary\* This PR adds a new lint inspired by the review of AztecProtocol/aztec-packages#6751 which enforces that oracle functions must be marked as unconstrained. I've also moved the error for calling an oracle function from a constrained function into the frontend rather than waiting until ACIR gen. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
Thunkar
added a commit
that referenced
this pull request
Jul 5, 2024
…nup (#7354) During the development of the defunct #6751 I realized we weren't constraining the args_hash computation in public interfaces, but promptly forgot during the offsite craze. This PR fixes that and cleanups the interface computation for readability. I would have loved to include the checks in the call interface itself instead of having it in the less clear macros, but we remove an `if` statement if the function takes no args, which in circuit land is very good.
AztecBot
pushed a commit
to AztecProtocol/aztec-nr
that referenced
this pull request
Jul 9, 2024
…nup (#7354) During the development of the defunct AztecProtocol/aztec-packages#6751 I realized we weren't constraining the args_hash computation in public interfaces, but promptly forgot during the offsite craze. This PR fixes that and cleanups the interface computation for readability. I would have loved to include the checks in the call interface itself instead of having it in the less clear macros, but we remove an `if` statement if the function takes no args, which in circuit land is very good.
Closing in favor of #8232 etc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP (only for assessment by the Noir team)
Motivation
Best described by @TomAFrench in noir-lang/noir#4442. This is an alternative approach to solving that problem, using a combination of #6618 and a little bit of extra support by the language at the frontend level.
Overview
The implementation is based on the following components:
Unconstrained<T>
: This PR adds anUnconstrained<T>
type that manifests as a specialUnresolvedTypeData
. This pseudotype gets converted into a regular Struct type in the resolver, that is provided by the stdlib. Adding the type allows a very clean interface that adds a new validation rule to the parser: unconstrained functions must always returnUnconstrained<T>
or nothing at all. This type exposes two basic methods:.unwrap()
to extract the inner value much likeOption<T>
andmake_constrained(constrainer)
, which invites the user to assert the returned values and potentially map them to a "safe" version..as_slice()
, return values from unconstrained functions are automatically coerced toUnconstrained<T>
, avoiding a lot of boilerplateassert_unconstrained
. This forbids the user from using.unwrap()
in a constrained environment,which ensures they have at least attempted to make the value secure (or makes very obvious they didn't).
UX
There's a great example of usage in the standard library
Also meaningful compilation errors are returned with this approach:
Trying to return a regular old value from an
unconstrained
function:Trying to
.unwrap()
an unconstrained value from a constrained function:.unwrap()
being used in a nested unconstrained call:Problems found
Unconstrained<T>
as both a type and an identifier (much likeField
) made Cannot access trait type methods from built-in types. noir-lang/noir#4463 show its ugly head again. I modified the.ident()
parser to accept certain keywords as valid idents, which I think could also be a solution to the aforementioned bug (and doesn't seem to cause any unintended side effects).as_slice()
, but I used the same approach for mine) was broken on entrypoint (or main) functions. The expr_id of the body of a function was being replaced by that of a call, which failed in a later stage of the compiler. I fixed it by wrapping the conversion in a new block.TODOs
aztec-packages
. (sort of done, I need to run e2e, but I migrated all ofaztec-nr
to this new system to ensure it worked and was usable...it's in another branch because these changes probably belong in the noir repo and should be moved there)