fix: Enforce single target network notes in NTB#1166
Conversation
e307d6d to
0681e25
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. Overall, I think this approach works fine for now. We could make it more type-safe by introducing newtype for each type of notes. NetworkNote then could look something like:
pub enum NetworkNote {
SingleTarget(SingleTargetNote),
MultiTarget(MultiTargetNote),
}
pub struct SingleTargetNote(Note);
pub struct MultiTargetNote(Note);And then most of the APIs would work with SingleTargetNote for now.
But I'll defer to @Mirko-von-Leipzig on this.
| // TODO: support multi target network notes. | ||
| if let Some(prefix) = note.account_prefix() { | ||
| // Ignore notes which don't target an existing account. | ||
| let Some(account) = state.fetch_account(prefix).await? else { | ||
| continue; | ||
| }; | ||
| account.add_note(note); | ||
| } |
There was a problem hiding this comment.
I would probably replace the TODO with a simple comment explaining that we handle only single-target accounts for now.
Also, we can probably re-write this as:
if let Some(prefix) = note.account_prefix() {
let Some(account) = state.fetch_account(prefix).await? {
account.add_note(note);
}
}Lastly, I would expand the comment and explain under which circumstances we may get a single-target note that doesn't have a target account.
| // TODO: support multi target network notes. | ||
| if let Some(prefix) = note.account_prefix() { | ||
| self.nullifier_idx.insert(note.nullifier(), prefix); | ||
| // Skip notes which target a non-existent network account. | ||
| if let Some(account) = | ||
| self.fetch_account(prefix).await.context("failed to load account")? | ||
| { | ||
| account.add_note(note); | ||
| } |
There was a problem hiding this comment.
Here too I would replace the TODO with a simple comment about not handling multi-target notes yet (mostly because enabling support for multi-target notes is a much bigger refactoring than a simple TODO here implies).
Also, I would expand the comment and explain why there could be non-existent network accounts.
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
As mentioned; I would prefer a SingleTargetNetworkNote so that we're certain what we're working with everywhere.
We don't know what or how we're going to manage the other variant(s); I think adding a strong type will help us refactor in the future without worrying about screwing up.
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
b454ddf to
914f859
Compare
Context
Closes #1115.
The NTB only supports single target network notes and does not currently check for this when constructing
NetworkNotefromNote.Changes
NetworkNoteto be an enum with single and multi target variants.SingleTargetNetworkNoteandMultiTargetNetworkNotetypes.SingleTargetNetworkNotein NTB.