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

Consider moving return value witnesses directly after the inputs so function signature fully defines ABI #5104

Closed
TomAFrench opened this issue May 24, 2024 · 2 comments · Fixed by #5142

Comments

@TomAFrench
Copy link
Member

We recently moved to a situation where we always generate distinct witnesses for all of the return values (#4914). This means that we can know exactly how many witnesses will be needed for the return values before performing acir gen.

This then raises the potential for just reserving these witness indices so that instea of [...program inputs, ...intermediate witnesses, ...program return values] the witnesses are laid out as [...program inputs, ...program return values, ...intermediate witnesses].

The result of this is that ABI encoding/decoding of arguments/return values will be entirely defined by the function signature and independent of the rest of the circuit. This will allow us to have a foundry cast like tool for ABI encoding/decoding witnesses.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 24, 2024
@sirasistant
Copy link
Contributor

This will make it so param_witnesses and return_witnesses are not needed anymore right? since they can be completely derived from the types

@TomAFrench
Copy link
Member Author

Yep, that's correct.

github-merge-queue bot pushed a commit that referenced this issue May 31, 2024
…5142)

# Description

## Problem\*

Resolves #5104 
## Summary\*

This PR preallocates some witnesses to hold the return values at the
beginning of ACIR gen and then adds assertions to fill these witnesses
with the return values. This ensures that the return values will be
placed in the witness map directly after any function inputs (reasons
for this being desirable are laid out in #5104)

## 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.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir May 31, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
# Description

## Problem\*

Followup to #5142 
Related to #5104 

## Summary\*

This PR deletes the `param_witnesses` and `return_witnesses` fields from
the ABI as the locations of all inputs/return values are defined
implicitly by the types themselves.

I've left this as draft as there's some changes which need to be made to
the JS code which will require some extra work (it needs to be made
aware of the sizes of various types.)

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants