PrivateMutable.replace should return previous note

[olehmisar]

To avoid calling get_note if you need the previous note

before

let prev = x.get_note();
assert(prev.something);
x.replace(y);

after:

let prev = x.replace(y);
assert(prev.something);

[olehmisar]

something like this. It shaves off 4365 gates from PrivateMutable<UintNote> .get_note + .replace.

use aztec::{
    note::{
        lifecycle::{create_note, destroy_note_unsafe},
        note_emission::NoteEmission,
        note_getter::get_note,
        note_interface::{NoteHash, NoteType},
        retrieved_note::RetrievedNote,
    },
    prelude::{PrivateContext, PrivateMutable},
    protocol_types::traits::Packable,
    state_vars::storage::Storage,
};

pub fn private_mutable_replace_note<Note, let N: u32>(
    context: &mut PrivateContext,
    storage_var: PrivateMutable<Note, _>,
    new_note: Note,
) -> (Note, NoteEmission<Note>)
where
    Note: Packable<N> + NoteType + NoteHash,
{
    let (prev_retrieved_note, note_hash_for_read_request): (RetrievedNote<Note>, Field) =
        get_note(context, storage_var.get_storage_slot());

    // Nullify previous note.
    destroy_note_unsafe(context, prev_retrieved_note, note_hash_for_read_request);

    // Add replacement note.
    (prev_retrieved_note.note, create_note(context, storage_var.get_storage_slot(), new_note))
}

[nventuro]

@wildjos is working on a PR that will address this issue. In it we're changing replace to take a function that receives the current note and returns the new note. Your original snippet was this:

let prev = x.get_note();
assert(prev.something);
x.replace(y);

This could be written as:

x.replace(|prev| {
  assert(prev.something);
  y
});

or the slightly wonkier

let prev_ref = &mut std::zeroed();
x.replace(|prev| {
  *prev_ref = prev;
  y
});

assert(prev.something);

While this may seem worse than your alternative, we prefer it for two reasons:

1. this allows people to build the replacement note based on the current note, e.g. increment a counter, which we've found is very common
2. we want to in general avoid having return values that are only sometimes useful, as we typically rely on unused return value warnings to signal danger to users. If replace always returned the previous value, and most people where uninterested in it, they'd be forced to write let _ = state_var.replace(...), and we'd be incorrectly teaching them to hide these valuable warnings in this manner.

[wildjos]

I’ve opened a PR that addresses this issue: it refactors the private_mutable storage pattern to use the new replace(|old| …) closure approach, removing the previous get_note() + replace() pattern.

This is a breaking change (feat!) because code relying on the old pattern will need to be updated.

PR: #17017