fix(labels): correct payjoin transaction labeling

[ethicnology]

Fixes incorrect labeling of completed payjoin transactions.

Merge order: independent — any time after #2443. No dependency on PR1-PR5, PR7.
#2443 → PR6 (this)

[ethicnology]

@claude audit this PR please

[claude]

Claude finished @ethicnology's task in 4m 42s —— View job

---

Audit of PR #2482 — fix(labels): correct payjoin transaction labeling

• Read repo guidelines (no CLAUDE.md in this repo)
• Review the diff against origin/payjoin-upgrade
• Read surrounding code for context (labels feature, mapper, usecases, send cubit, payjoin core)
• Verify the claimed bugs exist on the base branch
• Post audit findings

Verdict

The two fixes are real and correct. I verified both bugs on origin/payjoin-upgrade:

1. _validateTxid ignored its parameter (label_entity.dart:44-58): it validated the full reference field instead of the input slice, so every well-formed input/output/publicKey label (txid:vout, 66+ chars) was unconditionally rejected. The fix is minimal and correct.
2. store() persisted before validating (labels_repository_adapter.dart:14-48): the old code inserted the row, then threw while constructing the return LabelEntity. Combined with bug 1, a single such row poisoned every subsequent fetchAll/fetchByReference (the old .map().toList() propagated the first bad row's exception, blanking all labels app-wide). Validate-before-write plus _mapRowsTolerantly addresses both the cause and the symptom for DBs already corrupted in the field. Tests cover the regressions well.

Findings

1. fetchById is not tolerant — inconsistent with the other fetches (labels_repository_adapter.dart:67-72)
It still calls LabelMapper.toLabelEntity(row) directly, so a corrupt row fetched by id throws instead of being skipped/logged like everywhere else. Suggest returning null (with the same warning log) on mapping failure. Fix this →

2. A txid:vout reference without a colon throws RangeError, not LabelValidationException (label_entity.dart:31-38)
reference.split(':') followed by parts[1] blows up with a RangeError when there's no :. Reads survive (the tolerant mapper catches everything) and StoreLabelUsecase has a catch-all, but the exception type contract is broken for any caller that filters on LabelValidationException. Also txid:0:extra is silently accepted. A if (parts.length != 2) throw LabelValidationException(...) guard would close both.

3. Throwaway LabelEntity(id: 0, ...) for its validation side effect (labels_repository_adapter.dart:22-28)
It works, but constructing an entity purely to trigger a constructor side effect is easy for a future reader to "clean up" and thereby reintroduce the bug. Consider a static LabelEntity.validateReference(LabelType type, String reference) (called by both the constructor and store()), or validating in NewLabel's constructor so every NewLabel is valid by construction — that would also protect any future write path automatically.

4. Corrupt rows already in users' DBs are now hidden, never cleaned
_mapRowsTolerantly drops bad rows on every read and logs a warning each time — for an affected user, that's log noise on every fetchAll forever, and the rows still count against the (label, reference) unique constraint. Consider a one-time cleanup (migration or startup sweep) that deletes rows failing validation. Not blocking, but worth a follow-up issue.

5. Scope: nothing in this diff is payjoin-specific — please confirm the intent
The PR body says "Fixes incorrect labeling of completed payjoin transactions." If the symptom was labels disappearing/blank on payjoin transactions because a poisoned row broke every label read, this PR fixes it. But if the symptom was the label being attached to the wrong txid: on a payjoin send, the user label is stored against payjoinSender.originalTxId (send_cubit.dart:1886-1892, note the existing TODO there), and nothing re-points it to the final payjoin txid when the proposal — with a different txid — is what actually confirms. That wrong-txid problem is not addressed by this diff. Worth stating in the PR description which of the two this fixes.

Minor / nits

• The comment blocks in store() and _validateTxid narrate the historical bug ("the previous shape…"). That context is valuable in the commit message; in the code it will read as noise once merged. Consider trimming to just the invariant ("must validate before the insert" / "validate the slice, not the field").
• Pre-existing, not this PR: _validateIndex accepts '+1' and ' 1' (Dart's int.tryParse allows a sign and surrounding whitespace), so txid:+1 passes validation.
• Test gaps: no test for fetchById on a corrupt row (finding 1), none for the missing-colon reference (finding 2), and no store() test with an input-type NewLabel exercising the new pre-write validation end to end.

Caveats

Flutter isn't available in this runner and no CI runs are attached to the PR yet, so I couldn't execute flutter analyze/flutter test — the review is static. The new tests read as correct against SqliteDatabase, LabelsCompanion, and LabelTypeColumn.tx as they exist in the tree.