fix: move recompute logic to OutputNoteBuilder::add_asset to avoid repeated hashing

[PoulavBhowmick03]

Closes #2525

• Removed NoteAssets::add_asset method

• Changed OutputNoteBuilder::assets from NoteAssets to Vec<Asset>

• Moved the asset merging logic into OutputNoteBuilder::add_asset

[bobbinth]

Looks good! Thank you!

[PhilippGackstatter]

TL;DR: Let's merge this PR as-is; we'll need to touch this again in the future anyway.

Long-form thought process

I somehow hadn't realized we would have to move the merge logic to the builder. Ideally we should not have this logic implemented in `OutputNoteBuilder`. It should be part of `NoteAssets` somehow in case we need to reuse it (currently we don't).

I can think of two other approaches:

• Keep NoteAssets::add_asset and remove the cached commitment inside. Instead we'd have to_commitment which would compute the commitment when called.
  • We use NoteAssets::commitment in a few places, so re-hashing it a few times is not ideal but also not really bad.
• Remove NoteAssets::add_asset and introduce a NoteAssetsBuilder that implements the merge logic internally and only constructs NoteAssets when NoteAssetsBuilder::build is called (which computes the commitment once).
  • Then we can have a NoteAssets::into_builder to reconstruct a builder and mutate the note assets easily.

The builder seems a bit overkill, but I think it's the best solution to avoid a bunch of unnecessary hashing.

In the future, assets will be able to customize how they are merged, and so this approach to merging assets as we have it now will no longer work. For the purpose of transaction execution, we'll probably need to let the VM do the merging and then extract the merged asset in a transaction event and insert into NoteAssets. In this case, the current approach will work better than what we've had so far, so I think the PR is still a step in the right direction.