notes: track transaction note metadata#4218
Conversation
bznein
left a comment
bznein
left a comment
There was a problem hiding this comment.
Codex pointed out this:
The metadata/tombstone model mostly works, but blank notes for transactions that never had notes are now recorded as deletion tombstones through existing send paths. That
can corrupt sync semantics and should be fixed before merging.
Review comment:
- [P2] Avoid tombstones for missing blank notes — /home/bznein-coding-agent/agent-
work/bitbox-wallet-app/backend/accounts/notes/notes.go:108-113
When note == "" and the txID is absent, this now marks the note as changed and
persists an empty entry with fresh ModifiedAt. Current BTC/ETH send paths call
SetTxNote(..., txNote) unconditionally, so sending without a note creates a
tombstone for a note the user never deleted; callers of TransactionNoteEntries()
then cannot distinguish this from an actual deletion and may propagate false
deletes. Keep missing+empty as a no-op, or skip blank send notes before calling
SetTxNote.
| } | ||
|
|
||
| func (notes *Notes) setTransactionMetadata(txID string, metadata TransactionMetadata) { | ||
| if !metadata.ModifiedAt.IsZero() { |
There was a problem hiding this comment.
[nit] you can use the isZero method defined above here if I'm not mistaken
| if len(entry.Note) > MaxNoteLen { | ||
| return errp.Newf("Length of note must be smaller than %d. Got %d", MaxNoteLen, len(entry.Note)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nit] what if instead of early return, we log all errors?
and do something similar to
func validateTransactionNoteEntries(entries map[string]TransactionNoteEntry) error {
var errs []error
for _, entry := range entries {
if len(entry.Note) > MaxNoteLen {
errs = append(errs, errp.Newf(
"Length of note must be smaller than %d. Got %d",
MaxNoteLen,
len(entry.Note),
))
}
}
if len(errs) > 0 {
return errors.Join(errs...)
}
return nil
}I don't feel particularly strong about it tbh, just an idea
There was a problem hiding this comment.
Yeah why not. I am also considering truncating to the max length instead of failing, wdyt?
There was a problem hiding this comment.
Yeah I think that makes sense!
There was a problem hiding this comment.
SetTxNote also has been failing on too large notes, so I'll leave it like this for consistency.
I applied your suggestion.
Very good catch! Fixed. |
Empty notes become tombstones, and we add modified timestamps. This is necessary so BitBoxSync can merge conflicts and sync deletions.
2 participants
Empty notes become tombstones, and we add modified timestamps. This is necessary so BitBoxSync can merge conflicts and sync deletions.