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(AztecMacro): Autogenerate constants for struct field slots for better note-getter options #3011

Closed
Tracked by #5077
spalladino opened this issue Oct 24, 2023 · 5 comments · Fixed by #4508
Closed
Tracked by #5077
Assignees
Labels
C-aztec.nr Component: Aztec smart contract framework T-feedback Type: recording user feedback

Comments

@spalladino
Copy link
Collaborator

When writing a note getter, the dev needs to specify how to filter or sort based on the indices of each field. This is error prone, as a small change in a struct automatically breaks all note getters that depend on it if it changes the position of the fields.

We should somehow automatically generate constants (macros? new language feature?) for getting the index of a field within a struct, to be used in note getters.

struct MyStruct {
    foo: Field;
    bar: Field;
}

NoteGetterOptions::new()
            .select(MyStruct.foo.index, _foo)
            .select(MyStruct.bar.index, _bar)

Requested by @0xShaito from Wonderland

@spalladino spalladino added C-aztec.nr Component: Aztec smart contract framework T-feedback Type: recording user feedback labels Oct 24, 2023
@rahul-kothari rahul-kothari added this to A3 Oct 25, 2023
@github-project-automation github-project-automation bot moved this to Todo in A3 Oct 25, 2023
@spalladino spalladino changed the title Autogenerate constants for struct field slots Autogenerate constants for struct field slots for better note-getter options Jan 8, 2024
@Thunkar
Copy link
Contributor

Thunkar commented Feb 2, 2024

Is this the only use case for this feature @spalladino? I was discussing it with @sirasistant and @rahul-kothari and we found a pretty problematic footgun if implemented naively. Since we allow users to do things like this:

struct TrickyNote {
    a_bool: bool,
    another_bool: bool
}

impl Serialize<1> for TrickyNote {
    fn serialize(self) -> [Field; 1] {
        [self.a_bool as Field + (self.another_bool as Field) * 10]
    }
}

impl Deserialize<1> for TrickyNote {
    fn deserialize(fields: [Field; 1]) -> Self {
        Self {
            a_bool: fields[0] == 1,
            another_bool: fields[0] / 10 == 1
        }
    }
}

And NoteGetterOptions operates in "serialized world", we could have the following situation:

struct Storage {
    notes: Set<TrickyNote>
}

fn main() {
    // oh noes
    let what_to_look_for = "i_dont_know";
    let options = NoteGetterOptions::new().select(
        TrickyNote::get_index_for("what?"),
        what_to_look_for,
        Option::none()
    );
}

Granted it's a convoluted case, but it shows the disconnect between the struct shape and the serialized form. If this is the only usage of such a feature, I think we need a better solution.

@spalladino
Copy link
Collaborator Author

Yep, this is the only use case for this feature (that I know of).

Agree that with your scenario this is a potential footgun, but the current way of doing things is not good either. Maybe this is a hint that note serialization should be handled automatically by the system..?

@Thunkar
Copy link
Contributor

Thunkar commented Feb 5, 2024

Maybe this is a hint that note serialization should be handled automatically by the system..?

It would be pretty easy to create a canonical (de)serialization impl for notes via macro that ensures 1 struct field -> 1 serialization field, but it could be both less efficient and add a significant amount of "magic" to the platform...

@spalladino
Copy link
Collaborator Author

I'm not too familiar with Rust, but maybe this could be solved if we had a derive macro in Noir, such that we could derive(serializable) for notes, using a default implementation (either 1 struct to 1 field, or some form of packed serialization?).

Still, this doesn't solve the MyStruct.foo.index problem when the index in the serialization does not match the one in the struct. Perhaps we need to have a separate method get_index_of_field that needs to be implemented as part of the serializable trait for notes, or gets derived automatically?

@nventuro
Copy link
Contributor

nventuro commented Feb 5, 2024

add a significant amount of "magic" to the platform...

As a new user this doesn't feel like magic: on the contrary, I'd greatly appreciate not having to deal with all of these details (until I absolutely have to). I'd compare this to how Solidity doesn't have you worry about how structs are passed in external calls, received, hashed, emitted in logs, etc.

@LHerskind LHerskind changed the title Autogenerate constants for struct field slots for better note-getter options feat(AztecMacro): Autogenerate constants for struct field slots for better note-getter options Mar 8, 2024
Thunkar added a commit that referenced this issue Mar 19, 2024
Partially addresses:
#4519 (moved
autogeneration to the macro, even if not incremental)
Closes: #3011

Added the `#[aztec(note)]` attribute, which automatically implements
most of the `NoteInterface` trait in a struct marked as such, plus
several utilities. Even if this adds a fair share of "magic" to the note
implementation logic, it is structured in a way that it's hopefully easy
to follow, including meaningful errors attached to the correct span
during the process.

![Screenshot 2024-03-14 at 14 59
07](https://github.com/AztecProtocol/aztec-packages/assets/5404052/84a3d6e4-e346-4cfe-93eb-ec317632f344)

Hey you! Implement the trait!

![Screenshot 2024-03-14 at 14 46
39](https://github.com/AztecProtocol/aztec-packages/assets/5404052/bebfb3dd-c178-44d0-b9bc-005b5c9f0f38)
But only the meat and potatoes though.

As long as the user doesn't want to do any custom stuff, `get_header`,
`set_header`, `compute_note_content_hash`, `get_note_type_id`,
`serialize_content` and `deserialize_content` get automatically
implemented. Any combination of them can be overridden by the developer
though.

A metadata struct is also added, which takes the following form:

```rust 
struct CardNote {
    points: FieldSelector,
    randomness: FieldSelector,
    owner: FieldSelector,
}
```

This is used to implement a `properties()` function, which in turn can
be used in conjunction with the `NoteGetterOptions.select` and `.sort`

<img width="697" alt="Screenshot 2024-03-18 at 15 27 27"
src="https://github.com/AztecProtocol/aztec-packages/assets/5404052/5da531b3-0b7f-4cf9-9908-300ff8d98c6d">
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Mar 19, 2024
AztecBot added a commit to noir-lang/noir that referenced this issue Mar 19, 2024
…ecProtocol/aztec-packages#4508)

Partially addresses:
AztecProtocol/aztec-packages#4519 (moved
autogeneration to the macro, even if not incremental)
Closes: AztecProtocol/aztec-packages#3011

Added the `#[aztec(note)]` attribute, which automatically implements
most of the `NoteInterface` trait in a struct marked as such, plus
several utilities. Even if this adds a fair share of "magic" to the note
implementation logic, it is structured in a way that it's hopefully easy
to follow, including meaningful errors attached to the correct span
during the process.

![Screenshot 2024-03-14 at 14 59
07](https://github.com/AztecProtocol/aztec-packages/assets/5404052/84a3d6e4-e346-4cfe-93eb-ec317632f344)

Hey you! Implement the trait!

![Screenshot 2024-03-14 at 14 46
39](https://github.com/AztecProtocol/aztec-packages/assets/5404052/bebfb3dd-c178-44d0-b9bc-005b5c9f0f38)
But only the meat and potatoes though.

As long as the user doesn't want to do any custom stuff, `get_header`,
`set_header`, `compute_note_content_hash`, `get_note_type_id`,
`serialize_content` and `deserialize_content` get automatically
implemented. Any combination of them can be overridden by the developer
though.

A metadata struct is also added, which takes the following form:

```rust
struct CardNote {
    points: FieldSelector,
    randomness: FieldSelector,
    owner: FieldSelector,
}
```

This is used to implement a `properties()` function, which in turn can
be used in conjunction with the `NoteGetterOptions.select` and `.sort`

<img width="697" alt="Screenshot 2024-03-18 at 15 27 27"
src="https://github.com/AztecProtocol/aztec-packages/assets/5404052/5da531b3-0b7f-4cf9-9908-300ff8d98c6d">
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Mar 20, 2024
Partially addresses:
AztecProtocol/aztec-packages#4519 (moved
autogeneration to the macro, even if not incremental)
Closes: AztecProtocol/aztec-packages#3011

Added the `#[aztec(note)]` attribute, which automatically implements
most of the `NoteInterface` trait in a struct marked as such, plus
several utilities. Even if this adds a fair share of "magic" to the note
implementation logic, it is structured in a way that it's hopefully easy
to follow, including meaningful errors attached to the correct span
during the process.

![Screenshot 2024-03-14 at 14 59
07](https://github.com/AztecProtocol/aztec-packages/assets/5404052/84a3d6e4-e346-4cfe-93eb-ec317632f344)

Hey you! Implement the trait!

![Screenshot 2024-03-14 at 14 46
39](https://github.com/AztecProtocol/aztec-packages/assets/5404052/bebfb3dd-c178-44d0-b9bc-005b5c9f0f38)
But only the meat and potatoes though.

As long as the user doesn't want to do any custom stuff, `get_header`,
`set_header`, `compute_note_content_hash`, `get_note_type_id`,
`serialize_content` and `deserialize_content` get automatically
implemented. Any combination of them can be overridden by the developer
though.

A metadata struct is also added, which takes the following form:

```rust 
struct CardNote {
    points: FieldSelector,
    randomness: FieldSelector,
    owner: FieldSelector,
}
```

This is used to implement a `properties()` function, which in turn can
be used in conjunction with the `NoteGetterOptions.select` and `.sort`

<img width="697" alt="Screenshot 2024-03-18 at 15 27 27"
src="https://github.com/AztecProtocol/aztec-packages/assets/5404052/5da531b3-0b7f-4cf9-9908-300ff8d98c6d">
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
Partially addresses:
AztecProtocol/aztec-packages#4519 (moved
autogeneration to the macro, even if not incremental)
Closes: AztecProtocol/aztec-packages#3011

Added the `#[aztec(note)]` attribute, which automatically implements
most of the `NoteInterface` trait in a struct marked as such, plus
several utilities. Even if this adds a fair share of "magic" to the note
implementation logic, it is structured in a way that it's hopefully easy
to follow, including meaningful errors attached to the correct span
during the process.

![Screenshot 2024-03-14 at 14 59
07](https://github.com/AztecProtocol/aztec-packages/assets/5404052/84a3d6e4-e346-4cfe-93eb-ec317632f344)

Hey you! Implement the trait!

![Screenshot 2024-03-14 at 14 46
39](https://github.com/AztecProtocol/aztec-packages/assets/5404052/bebfb3dd-c178-44d0-b9bc-005b5c9f0f38)
But only the meat and potatoes though.

As long as the user doesn't want to do any custom stuff, `get_header`,
`set_header`, `compute_note_content_hash`, `get_note_type_id`,
`serialize_content` and `deserialize_content` get automatically
implemented. Any combination of them can be overridden by the developer
though.

A metadata struct is also added, which takes the following form:

```rust 
struct CardNote {
    points: FieldSelector,
    randomness: FieldSelector,
    owner: FieldSelector,
}
```

This is used to implement a `properties()` function, which in turn can
be used in conjunction with the `NoteGetterOptions.select` and `.sort`

<img width="697" alt="Screenshot 2024-03-18 at 15 27 27"
src="https://github.com/AztecProtocol/aztec-packages/assets/5404052/5da531b3-0b7f-4cf9-9908-300ff8d98c6d">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-aztec.nr Component: Aztec smart contract framework T-feedback Type: recording user feedback
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants