Custom implementation of `NoteInterface::get_note_type_id` needs to return the same id as generated by macros

[benesjan]

When a note with a custom implementation of NoteInterface::get_note_type_id does not return the value as is set by macros then the code becomes broken in multiple places:

1. compute_note_hash_and_optionally_a_nullifier function uses an incorrect id and the code panics as a result,
2. note type id in the TS artifact is incorrect which can cause various issues in tests etc.

This is caused by the fact that in the macros we use the counter value and not the one from the custom impl.:

pub comptime fn note_custom_interface(s: StructDefinition) -> Quoted {
    let (packable_impl, note_packed_len) = derive_packable_if_not_implemented_and_get_len(s);
    **let note_type_id = get_next_note_type_id()**;

    let (indexed_fixed_fields, indexed_nullable_fields) = index_note_fields(s, &[]);
    register_note(
        s,
        note_packed_len,
        **note_type_id**,
        indexed_fixed_fields,
        indexed_nullable_fields,
    );

...
}

I don't think we can get a hold of the note type id here because the function is comptime and hence we cannot call the runtime NoteInterface::get_note_type_id function here.

I don't really see a value of letting the user define custom get_note_type_id function so I would say trying to rethink the interface would make sense.

The #[note_custom_interface] macro is not really used much which is the reason why we have not stumbled upon this yet.

[benesjan]

I was thinking more about this and I would say that most developers will not care about implementing the compute_note_hash function nor the nullifier one. They will be implementing their own notes only because they need a data container, not to fiddle with hash functions.

So I would say that by default we should not bother the dev with implementing any of these. Currently all the notes in aztec-packages use the same compute_nullifier function implementation --> it's copied around literally 10 times.

So I would say by default all the notes should just look like this:

#[note]
pub struct ValueNote {
    value: Field,
    owner: AztecAddress,
}

But we still want the flexibility and since we cannot partially implement a trait using macros we need to split the interface into 2.

Currently this is already the case and we have:

pub trait NoteInterface {
    fn get_note_type_id() -> Field;
    fn compute_note_hash(...) -> Field;
}
pub trait NullifiableNote {
    fn compute_nullifier(...) -> Field;
    unconstrained fn compute_nullifier_without_context(...) -> Field;
}

Instead I propose having the following 2:

pub trait NoteInterface {
    fn get_note_type_id() -> Field;
}
pub trait NoteHashing {
    fn compute_note_hash(...) -> Field;
    fn compute_nullifier(...) -> Field;
    unconstrained fn compute_nullifier_without_context(...) -> Field;
}

I checked the current trait bounds in our codebase and I realized that this would actually improve them because we are either:

1. Computing note hashes and nullifiers,
2. emitting note logs,
3. doing both.

For the first case we only need the hash functions but we don't need the note type id. This would result in the trait bounds getting simplified!:

For the second case we don't need the hashing function but we need the note type id (it gets stored in logs) and we need to pack the note so the trait bounds would be unchanged and they would stay NOTE: NoteInterface + Packable<N>.

For the 3rd case we need all of these and trait bounds would also be unchanged.

How to update the macros

I propose having #[note] macro that would auto-implement both NoteInterface and NoteHashing. Then I would have #[partial_note] which would also implement both but with the partial notes functionality. And then I would have #[custom_note] and #[custom_partial_note] and both of these would not implement the NoteHashing interface and the only difference between the 2 would be that #[custom_partial_note] would implement the setup and finalization payloads and #[custom_note] would not.

Update: Realized that #[custom_partial_note] macro does not make sense as the setup and finalization payloads of partial notes are tightly coupled with the implementation of NoteHashing interface. Hence auto-generating them does not work when NoteHashing is not auto-generatted.

Summary

1. This would simplify the code and make ~97% of the smart contract devs not waste time with boilerplate nullifier functions,
2. ~2.9% of devs will most likely just want to play with nullifier schemes (as Mike's PLUME scheme suggests) and for this ~2.9% the DEVEX will become slightly worse by having to manually implement the compute_note_hash function,
3. the ~0.1% of devs which will want to implement their own compute_note_hash function will still be able to do so,
4. this tackles this issue as now we will be able to force the implementation of get_note_type_id function by macros in all the cases while retaining the flexibility for the ~0.1% of devs which want to implement their own custom compute_note_hash functions.