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.

Sign up for GitHub

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

Jump to bottom

Assert future or past Mock Oracle calls #4652

Closed
nventuro opened this issue Mar 27, 2024 · 1 comment · Fixed by #4789
Closed

Assert future or past Mock Oracle calls #4652

nventuro opened this issue Mar 27, 2024 · 1 comment · Fixed by #4789
Labels
enhancement New feature or request

Comments

@nventuro
Copy link
Contributor

nventuro commented Mar 27, 2024 •
edited
Loading

Problem

OracleMock is currently useful for replacing an oracle and injecting values into code under test, but it doesn't let us test how the circuit calls the oracle. This will be quite important in some scenarios, e.g. in Aztec when using oracles to perform public storage writes, where we want to know the storage slot and value passed to the call.

Happy Case

There's multiple ways to do this. Some options off the top of my head:

  • have the oracle call a closure in which the user might perform some assertion, or store the value somewhere for later inspection
  • provide a way to query past oracle calls
  • provide a way to perform an assertion over the next oracle call

The past assertion over past and future call approches might be problematic in functions where the oracle is called multiple times - how would we handle this? Foundry follows this approach for events and calls, as as long as one of them in the transaction matches the assertion the test is considered as passing.

Project Impact

Blocker

Impact Context

There's some design patterns that are simply untestable without this feature.

Workaround

None
@nventuro nventuro added the enhancement New feature or request label Mar 27, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Mar 27, 2024
@nventuro
Copy link
Contributor Author

Related to #4371, though I believe this one is a bit broader.

@nventuro nventuro mentioned this issue Mar 27, 2024
Merged
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Apr 10, 2024
@nventuro @benesjan
feat: shared mutable storage (#5490)
c4e41a9 
(Large) part of
#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- #5491
- #5492 (this
takes care of padding during storage slot allocation)
- #5501
- #5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Apr 11, 2024
@nventuro @benesjan @AztecBot
feat: shared mutable storage (#5490)
04847fc 
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
@nventuro nventuro mentioned this issue Apr 11, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 12, 2024
@nventuro @TomAFrench
feat: get last mock oracles params (#4789)
1d96937 
Resolves #4652

# Description

This adds a new foreign call `get_mock_last_params` that can be used to
configure mocked oracles. It returns the last parameters the mock was
called with, or panics if it wasn't.

Note that this doesn't play very nicely with the `times` feature, since
e.g. if a mock is setup with `times(1)` it is then impossible to ever
retrieve the last params, as the mock will be automatically cleared.
There's a bunch of ways this could be handled, including keeping track
of cleared mocks and llowing these queries over them. I didn't stress
too much about this since we're only now starting to use the mocks, and
I think it might be better to experiment more with them and get more
feedback before worrying about having a nicer API.

I also moved the mock oracle tests outside of `execution_success` into
`noir_test_success`, and added a few simpler cases that I think also
help illustrate how the mocks are supposed to be used (which I
personally found quite useful). I considered adding tests for the
scenarios in which the mocked oracle engine panics (e.g. if no mock
matches the foreign call, or if requesting last params for a mock that
was never called), but run into quite a lot of trouble getting this
working and ultimately gave up. I did at least improve a misleading
error message.

---------

Co-authored-by: Tom French <tom@tomfren.ch>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Apr 12, 2024
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
@superstar0402 @benesjan
feat: shared mutable storage (#5490)
6e8664d 
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
Noir
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: get last mock oracles params nventuro/noir
1 participant
@nventuro