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

Refactor and simplify string formatting #4238

Open
vezenovm opened this issue Feb 2, 2024 · 2 comments
Open

Refactor and simplify string formatting #4238

vezenovm opened this issue Feb 2, 2024 · 2 comments
Labels
discussion enhancement New feature or request

Comments

@vezenovm
Copy link
Contributor

vezenovm commented Feb 2, 2024

Problem

Before this PR #4101 the fmtstr type is only resolved when trying to print the type using a Brillig foreign call. Now with resolve_assert_message we also must handle formatting of strings as we are saving an assertion message during execution that is to be displayed upon an unsatisfied constrain. The new dynamic assertion message resolution also reuses the same methods println does for formatting.

Happy Case

We should have a uniform startegy for formatting strings. Ideally this would not be in the "backend" and done by foreign calls. Foreign calls should be only for requesting a service from the caller that is executing the program such as printing to stdout or saving a failure message. Right now the current foreign call builtins on nargo perform these functions but also format the string output.

A possible solution was posted here (#4101 (comment)). We should settle on the strategy for simplifying formatting in this discussion issue.

Alternatives Considered

We could lean into a format oracle that returns a string of variable length (this would have to be added to the language). println and resolve_assert_message would then have more isolated functionality and call format themselves.

A Display trait of some kind could be added and then it would be up to users to format their types in Noir (with the core team formatting the primitive and stdlib types). It would most likely only be available in an unconstrained environment. This would most likely require some kind of string builtins or a format oracle, so we would still have formatting in the "backend".

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@vezenovm vezenovm added the enhancement New feature or request label Feb 2, 2024
@jfecher
Copy link
Contributor

jfecher commented Feb 2, 2024

We could lean into a format oracle that returns a string of variable length

I don't think this would be possible to add as a type. It is similar to a string slice but if we return it from an oracle and its dependent on some other data's display method then we wouldn't know the length of the string at compile-time.

@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 2, 2024

I don't think this would be possible to add as a type.

You are correct we couldn't return it from the unconstrained environment to the constrained environment, but I was thinking we could still have it as a type similar to slices. We would then have a format oracle in the stdlib, but not a public wrapper around it for users to access.

It would just be a way to make sure each foreign call has it owns separate functionality. println would end up looking something like this where the only thing print does is display a string rather than formatting and writing to stdout:

#[oracle(format)]
unconstrained fn format_oracle<T>(input: T) -> String {}

unconstrained pub fn println<T>(input: T) {
    let input = format_oracle(input);
    print_oracle(true, input);
}

AztecBot added a commit that referenced this issue Mar 8, 2024
…l/aztec-packages#5086)

Closes #4520.

I did some work adding tests for this, but ultimately decided to go back
on it. We don't really have any tests for other parts of the `addNotes`
flow (such as the checks for note existence, inclusion in the provided
tx, non-nullification, etc.), and this didn't feel like the right place
to work on those.

We definitely should however. Part of the problem is that writing a
contract that allows for manual note creation, deletion and retrieval
from a test is quite annoying - I did some of this in `TestContract` for
#4238. For this we'd also need to have different functions for different
note types, and even then some of these notes can only be added
automatically via broadcast due to the random values in the note.
AztecBot added a commit that referenced this issue Mar 8, 2024
…l/aztec-packages#5086)

Closes #4520.

I did some work adding tests for this, but ultimately decided to go back
on it. We don't really have any tests for other parts of the `addNotes`
flow (such as the checks for note existence, inclusion in the provided
tx, non-nullification, etc.), and this didn't feel like the right place
to work on those.

We definitely should however. Part of the problem is that writing a
contract that allows for manual note creation, deletion and retrieval
from a test is quite annoying - I did some of this in `TestContract` for
#4238. For this we'd also need to have different functions for different
note types, and even then some of these notes can only be added
automatically via broadcast due to the random values in the note.
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
Projects
Status: 📋 Backlog
Development

No branches or pull requests

2 participants