refactor: index NoteUpdateTracker by details commitment#2267
refactor: index NoteUpdateTracker by details commitment#2267juan518munoz wants to merge 2 commits into
NoteUpdateTracker by details commitment#2267Conversation
JereSalo
left a comment
There was a problem hiding this comment.
I think this is great and I like the simplification. However, there seems to be a roundtrip problem on notes that have lost their note_id information but we still keep that info in the input_notes_by_id index.
This can happen when a note originally had metadata but after being externally consumed we can no longer compute its id, so it's lost and input_notes_by_id gets rebuilt with less information than what it originally had. It's not critical though, I proposed a fix and added a test here if you want to check it out, there's no need to merge it, I just did it in order to effectively check if it was an actual bug or not.
| (matches!( | ||
| update.update_type, | ||
| NoteUpdateType::Insert | NoteUpdateType::Update | NoteUpdateType::InsertCommitted | ||
| ) && update.inner().is_consumed()) |
There was a problem hiding this comment.
| (matches!( | |
| update.update_type, | |
| NoteUpdateType::Insert | NoteUpdateType::Update | NoteUpdateType::InsertCommitted | |
| ) && update.inner().is_consumed()) | |
| (update.update_type != NoteUpdateType::None && update.inner().is_consumed()) |
If you like we could simplify it to this. Though it's completely valid if you prefer to keep it explicit just in case.
There was a problem hiding this comment.
Addressed this and comment above in fae267c
Partly tackles #2227
Specifically, it resolves the
NoteUpdateTrackerfollowup flagged directly in code:https://github.com/0xMiden/miden-client/blob/31b387d5ff006b442640275fdd8745062668df6c/crates/rust-client/src/note/note_update_tracker.rs#L226-L229
NoteUpdateTrackernow keys all input notes byNoteDetailsCommitment(always available) instead of theNoteId/commitment split, which removes theexpected_input_notesmap and theInsertCommittedre-keying.