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(noir-contracts): Option<T> for get_notes #1272

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

iAmMichaelConnor
Copy link
Contributor

@iAmMichaelConnor iAmMichaelConnor commented Jul 30, 2023

Description

closes: #1020

Introduce Option<T> type to Noir.
Update get_notes logic:

  • Remove the is_real member of some notes. Option::is_some() can now be used instead.
  • Remove the is_dummy method of the NoteInterface.
  • When calling the get_notes oracle, a boolean is_some is injected after the nonce to convey whether the note is a dummy or not.
  • Change contract logic to unwrap Option<Note> types.

It's slightly more verbose in places, but it makes the notion of a dummy note more enshrined (and actually aligns more closely to our inital sketches of what utxo syntax might look like).

Thanks to Jake for the Option implementation!

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@iAmMichaelConnor iAmMichaelConnor added the T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature). label Jul 30, 2023
@iAmMichaelConnor
Copy link
Contributor Author

Having had the foresight to add a warning about unwrap_unchecked(), I then spent 45 minutes looking for why this example's e2e test was failing. It's because I was using unwrap_unchecked(). Sigh.

image

@iAmMichaelConnor iAmMichaelConnor marked this pull request as ready for review July 31, 2023 09:32
@iAmMichaelConnor
Copy link
Contributor Author

iAmMichaelConnor commented Jul 31, 2023

Please review this one first: #1099

Edit: ok, that one's been merged. I'll rebase.

Copy link
Collaborator

@ludamad ludamad left a comment

Choose a reason for hiding this comment

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

LGTM although I assume for a lot of these optional note arrays they'd ideally just be dynamic arrays?

@iAmMichaelConnor
Copy link
Contributor Author

LGTM although I assume for a lot of these optional note arrays they'd ideally just be dynamic arrays?

For the code surrounding the get_notes oracle calls, yeah we want these to eventually be dynamic. At the moment, the array lengths are hard-coded to a MAX.

@iAmMichaelConnor iAmMichaelConnor merged commit 584b70f into master Aug 1, 2023
@iAmMichaelConnor iAmMichaelConnor deleted the mc/option-for-get-notes branch August 1, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature).
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Introduce Option<T> type to Noir Contracts stdlib
2 participants