fix: skip unrecognized BIP329 label entries instead of failing the w…#670
fix: skip unrecognized BIP329 label entries instead of failing the w…#670Sandipmandal25 wants to merge 3 commits intobitcoinppl:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR changes label import to return a LabelParseReport (imported, skipped), switches Rust JSONL parsing to tolerant line-by-line Label deserialization, and surfaces skipped-count results through updated UniFFI, Android, and iOS UI flows so partial-import outcomes are reported. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Client/UI
participant Manager as LabelManager
participant Parser as Line Parser
participant Storage as Storage/Persistence
rect rgba(100,149,237,0.5)
Note over User,Storage: Line-by-line import with report
User->>UI: provide JSONL / scanned payload
UI->>Manager: import(jsonl) / importLabels(labels)
Manager->>Parser: parse each non-empty line into Label
loop per line
Parser->>Parser: attempt deserialize
alt deserializes to supported Label
Parser->>Manager: collect label
else unparsable or unsupported
Parser->>Manager: increment skipped (debug log)
end
end
Parser-->>Manager: (Labels, report {imported, skipped})
Manager->>Storage: save_imported_labels(parsed supported Labels)
Storage-->>Manager: ok (returns report)
Manager-->>UI: LabelParseReport {imported, skipped}
alt skipped > 0
UI->>UI: show partial-success alert/snackbar
else
UI->>UI: show full-success alert
end
UI->>Manager: refresh transactions
UI-->>User: display feedback
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the Nunchuck BIP329 import failure by making Confidence Score: 4/5Safe to merge after fixing the iOS file-import path that silently discards the skip report. The Rust fix is correct and well-tested, and Android is fully wired. One P1 gap remains: the iOS ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Raw JSONL Input] --> B[parse_labels]
B --> C{Each line}
C -->|valid & supported| D[parsed vec]
C -->|valid but unsupported type| E[skipped++]
C -->|parse error| F[first_error captured\nskipped++]
D --> G{parsed.is_empty?}
G -->|yes - all bad| H[Err: LabelManagerError::Parse]
G -->|no| I[LabelParseReport\nimported / skipped]
I --> J[save_imported_labels]
J --> K[DB insert]
J --> L[Return LabelParseReport]
L --> M{Caller}
M -->|Android File| N[✅ Shows skip count snackbar]
M -->|Android NFC| O[✅ onPartialSuccess callback]
M -->|Android QR Scan| P[✅ Alert with skip count]
M -->|iOS ScanManager QR/File| Q[✅ Shows skip count alert]
M -->|iOS fileImporter| R[❌ Report discarded\nAlways shows success]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/src/label_manager.rs (1)
536-544: Optional: avoid allocatingVec<&str>for lines.You only need the input count for the summary log. You can stream through
lines()with counters instead of collecting, at the cost of slightly more verbose code. Not a blocker.♻️ Sketch
- let lines: Vec<&str> = jsonl.trim().lines().filter(|line| !line.trim().is_empty()).collect(); - - if lines.is_empty() { - return Ok(Labels::new(vec![])); - } - - let mut parsed = Vec::with_capacity(lines.len()); - let mut first_error: Option<String> = None; - let mut skipped = 0usize; - - for line in &lines { + let mut parsed = Vec::new(); + let mut first_error: Option<String> = None; + let mut skipped = 0usize; + let mut total = 0usize; + + for line in jsonl.trim().lines().filter(|l| !l.trim().is_empty()) { + total += 1; match serde_json::from_str::<Label>(line) { Ok(label) => parsed.push(label), Err(e) => { if first_error.is_none() { first_error = Some(e.to_string()); } warn!("skipping unrecognized label entry: {e}"); skipped += 1; } } } + + if total == 0 { + return Ok(Labels::new(vec![])); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/label_manager.rs` around lines 536 - 544, The current code allocates a Vec<&str> from jsonl.trim().lines() only to count and iterate; change to streaming over jsonl.trim().lines() instead: remove the lines: Vec<&str> allocation and instead iterate the Lines iterator once, keeping counters (total_lines, skipped) and filling parsed (Vec) as you go while capturing first_error; preserve behavior that returns Ok(Labels::new(vec![])) when total_lines == 0 and keep using parsed, first_error, and skipped variable names so the summary log can use total_lines for the count. Ensure you still trim/skip empty lines (line.trim().is_empty()) during the single pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/label_manager.rs`:
- Around line 584-598: The comment and test description misspell the product
name "Nunchuk" as "Nunchuck"; update all occurrences in the comment blocks and
any related string literals or docstrings around the parse_labels tests (e.g.,
the comment above BAD_VOUT_OUTPUT and the test
test_parse_labels_skips_bad_vout_line) to use "Nunchuk" instead, and ensure the
PR description uses the corrected spelling as well so names are consistent.
---
Nitpick comments:
In `@rust/src/label_manager.rs`:
- Around line 536-544: The current code allocates a Vec<&str> from
jsonl.trim().lines() only to count and iterate; change to streaming over
jsonl.trim().lines() instead: remove the lines: Vec<&str> allocation and instead
iterate the Lines iterator once, keeping counters (total_lines, skipped) and
filling parsed (Vec) as you go while capturing first_error; preserve behavior
that returns Ok(Labels::new(vec![])) when total_lines == 0 and keep using
parsed, first_error, and skipped variable names so the summary log can use
total_lines for the count. Ensure you still trim/skip empty lines
(line.trim().is_empty()) during the single pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ccbcdaf-b407-4ef7-95db-e5628e13e97c
📒 Files selected for processing (1)
rust/src/label_manager.rs
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
8e73a59 to
ff26cc3
Compare
|
@Sandipmandal25 this looks good thanks, 2 questions before approving
|
we dont have a real nunchuk export to verify the exact format the issue mentioned coin collection labels with custom hashtag types but didnt shared a sanitized file. what we do know is bip329 spec explicitly says "importing wallets complying to this specification should ignore types not defined here", so making Cove less strict isnt a workaround its what the spec requires. the old
agreed right now skips only go to the debug log. surfacing a count + reasons to the user means changing import to return something richer than Result which touches the Uniffi boundary and needs ui changes to show the warning. can to add it here or track it as a follow up issue whichever you prefer. |
|
@Sandipmandal25 lets do the full change including sending all the info back, but for now lets keep the UI simple and just say |
sounds good updating the pr shortly! thanks |
| } | ||
|
|
||
| fn parse_labels(jsonl: &str) -> Result<Labels, LabelManagerError> { | ||
| fn parse_labels(jsonl: &str) -> Result<(Labels, u32), LabelManagerError> { |
There was a problem hiding this comment.
shouldnt we just return LabelImportResult here too?
lets think of a better name and use it everywhere mabye LabelParseReport ? Then later we can add reasons to this struct
There was a problem hiding this comment.
missed it renamed to labelparsereport and using it in parse labels too. also regenerated ios and android bindings
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt (1)
63-81:⚠️ Potential issue | 🟡 MinorOn transaction-refresh failure, the "labels skipped" feedback is suppressed.
If
getTransactions()throws at Line 69, the function shows the "transaction list may need manual refresh" snackbar and returns early, so theresult.skipped > 0branch at Line 76 never runs. The user never learns that some labels were skipped. Consider either showing the skipped-count snackbar before refreshing, or combining both pieces of information in the fallback snackbar.♻️ Suggested reorder
- val result = currentManager.rust.labelManager().use { labelManager -> - labelManager.import(fileContents.trim()) - } - - // refresh transactions with updated labels - try { - currentManager.rust.getTransactions() - } catch (refreshError: Exception) { - android.util.Log.e(tag, "failed to refresh transactions after label import", refreshError) - snackbarHostState.showSnackbar("Labels imported successfully, but transaction list may need manual refresh") - return@launch - } - - if (result.skipped > 0u) { - val noun = if (result.skipped == 1u) "label" else "labels" - snackbarHostState.showSnackbar("Labels imported. Could not import ${result.skipped} $noun") - } else { - snackbarHostState.showSnackbar("Labels imported successfully") - } + val result = currentManager.rust.labelManager().use { labelManager -> + labelManager.import(fileContents.trim()) + } + + val baseMessage = if (result.skipped > 0u) { + val noun = if (result.skipped == 1u) "label" else "labels" + "Labels imported. Could not import ${result.skipped} $noun" + } else { + "Labels imported successfully" + } + + try { + currentManager.rust.getTransactions() + snackbarHostState.showSnackbar(baseMessage) + } catch (refreshError: Exception) { + android.util.Log.e(tag, "failed to refresh transactions after label import", refreshError) + snackbarHostState.showSnackbar("$baseMessage — transaction list may need manual refresh") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt` around lines 63 - 81, The snackbar about skipped labels can be suppressed by early return when currentManager.rust.getTransactions() throws; before calling getTransactions() (in the block using currentManager.rust.labelManager()), check result.skipped and show the skipped-count snackbar via snackbarHostState.showSnackbar("... ${result.skipped} ...") (or build a combined message) so the user is always informed; keep the existing refresh attempt with currentManager.rust.getTransactions() and its fallback snackbar, but do not return before emitting the skipped-labels notification.ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
349-371:⚠️ Potential issue | 🟡 MinorFile-import flow doesn't surface
skippedcount, unlike other entry points.
try labelManager.import(jsonl: fileContents)discards the returnedLabelImportResultand unconditionally shows "Labels have been imported successfully." This is inconsistent with the QR flow below (Line 432-447) and withCoveMainView.swift/ Android counterparts, all of which now show a "Labels Imported / Could not import X labels" message whenskipped > 0. Users importing a non-compliant BIP329 file here will silently lose entries without notice — the exact scenario this PR is meant to improve.✏️ Proposed fix
- let file = try result.get() - let fileContents = try FileReader(for: file).read() - try labelManager.import(jsonl: fileContents) - - app.alertState = .init( - .general( - title: "Success!", - message: "Labels have been imported successfully." - ) - ) + let file = try result.get() + let fileContents = try FileReader(for: file).read() + let importResult = try labelManager.import(jsonl: fileContents) + + if importResult.skipped > 0 { + app.alertState = .init( + .general( + title: "Labels Imported", + message: "Could not import \(importResult.skipped) labels" + ) + ) + } else { + app.alertState = .init( + .general( + title: "Success!", + message: "Labels have been imported successfully." + ) + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift` around lines 349 - 371, The import branch in SelectedWalletScreen currently ignores the return of labelManager.import(jsonl:) and always shows a success alert; change it to capture the returned LabelImportResult, check its skipped count, and set app.alertState to either a full success message or a partial-success message ("Labels Imported / Could not import X labels") when result.skipped > 0 (mirroring the QR flow handling). Keep the Task { await manager.rust.getTransactions() } call after a successful/partial import so transactions refresh, and reference LabelImportResult and the skipped property when constructing the alert text.
🧹 Nitpick comments (4)
rust/src/backup/import.rs (1)
525-547: Prefermap_err_strper coding guidelines.Both of these touched lines match the exact anti-pattern called out in the guidelines. Replacing the closures with
map_err_str(fromcove_util::ResultExt) keeps behavior identical and is cleaner.♻️ Proposed refactor
+use cove_util::result_ext::ResultExt as _; @@ fn import_labels(id: &WalletId, jsonl: &str) -> Result<(), BackupError> { let manager = LabelManager::new(id.clone()); - manager.import(jsonl).map(drop).map_err(|e| BackupError::Restore(e.to_string())) + manager.import(jsonl).map(drop).map_err_str(BackupError::Restore) } @@ LabelRestoreBehavior::PreserveCloudBackupClean => manager .import_without_cloud_backup_dirty(jsonl) .map(drop) - .map_err(|error| BackupError::Restore(error.to_string())), + .map_err_str(BackupError::Restore), };(Adjust the import to the correct
ResultExtpath if it differs here — e.g., it's imported asuse cove_util::result_ext::ResultExt as _;inrust/src/label_manager.rs.)As per coding guidelines: "Use
cove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string()))— it's cleaner and equivalent".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` around lines 525 - 547, Replace the manual .map_err(|e| BackupError::Restore(e.to_string())) calls with the ResultExt::map_err_str helper: in import_labels (which calls manager.import(...)) and in restore_wallet_labels where manager.import_without_cloud_backup_dirty(...) is used, import the trait (e.g., use cove_util::result_ext::ResultExt as _;) and call map_err_str to convert errors to BackupError::Restore uniformly; this removes the closure anti-pattern and keeps behavior identical while relying on the ResultExt helper.ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
432-449: Success alert diverges from other iOS success paths.Other iOS import sites use
.importedLabelsSuccessfully(e.g.,CoveMainView.swiftLines 420, 506) for the zero-skipped case, while this uses a generic.general(title: "Success!", ...). Minor inconsistency — consider aligning to.importedLabelsSuccessfullyfor a uniform UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift` around lines 432 - 449, The success branch in SelectedWalletScreen.swift uses a generic .general alert instead of the app's standard .importedLabelsSuccessfully case; update the zero-skipped branch where you handle result from labelManager.importLabels(labels:) to set app.alertState = .init(.importedLabelsSuccessfully) (instead of the current .general(...) message) so behavior matches other import flows, leaving the skipped>0 branch unchanged and keeping the subsequent Task { await manager.rust.getTransactions() } call.rust/src/label_manager.rs (1)
575-577: Avoid.unwrap()even when the invariant holds.The invariant (
parsed.is_empty()⇒ at least one parse error) meansfirst_errorisSomehere, but an explicit fallback makes this robust against future refactors and avoids a potential panic if the invariant is ever broken.🛡️ Proposed defensive tweak
- if parsed.is_empty() { - return Err(LabelManagerError::Parse(first_error.unwrap())); - } + if parsed.is_empty() { + return Err(LabelManagerError::Parse( + first_error.unwrap_or_else(|| "no parseable label entries".to_string()), + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/label_manager.rs` around lines 575 - 577, The code returns Err(LabelManagerError::Parse(first_error.unwrap())) assuming first_error is Some when parsed.is_empty(); make this defensive by replacing the unwrap with a safe extraction (e.g., match or unwrap_or/unwrap_or_else) that supplies a sensible fallback error when first_error is None, so in the parsed.is_empty() branch you always construct LabelManagerError::Parse with either the actual parse error or a default error value/message; update the use sites around parsed and first_error in label_manager.rs to avoid .unwrap().android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
356-362: Pluralization inconsistency withMoreInfoPopover.kt.Here the message is always
"Could not import ${result.skipped} labels", butMoreInfoPopover.ktcorrectly picks"label"vs"labels"based on count. Consider aligning for consistency across entry points.✏️ Proposed fix
if (result.skipped > 0u) { + val noun = if (result.skipped == 1u) "label" else "labels" alertState = TaggedItem( AppAlertState.General( title = "Labels Imported", - message = "Could not import ${result.skipped} labels" + message = "Could not import ${result.skipped} $noun" ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt` around lines 356 - 362, The alert message in AppManager.kt always uses "labels" regardless of result.skipped; update the TaggedItem(AppAlertState.General(...)) assignment (where alertState is set) to pluralize correctly by checking result.skipped and using "label" when result.skipped == 1 and "labels" otherwise (or call the existing pluralization helper used by MoreInfoPopover.kt if available), so the message reads "Could not import 1 label" vs "Could not import N labels".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.kt`:
- Around line 89-95: The current logic treats a partial import (when
importResult.skipped > 0u) as a full error by calling onError, which
misrepresents success — change the flow in the block after val importResult =
labelManager.import(text.trim()) so that successful imports call onSuccess() and
skipped counts are reported separately; either add a new callback
onPartialSuccess(skipped: UInt) and invoke it when importResult.skipped > 0u
(while still calling onSuccess for the successful work) or keep calling
onSuccess() and surface the skipped count via an additional UI/state method;
also format the message to handle singular vs plural for the skipped count
(e.g., "Could not import 1 label" vs "labels") and ensure nfcReader.reset() is
still called regardless.
In `@rust/src/backup/import.rs`:
- Line 527: The restore path currently ignores the skipped-label count from
parse_labels, so change the import flow (starting at manager.import(jsonl)) and
the restore_wallet_labels return types to propagate skipped counts into
LabelRestoreOutcome (or augment LabelRestoreWarning) and ensure
BackupImportReport includes that skipped count; specifically, have parse_labels
return/sketch its skipped number to be carried through restore_wallet_labels and
consumed by manager.import(jsonl) result mapping so the resulting
LabelRestoreOutcome/LabelRestoreWarning records imported vs skipped counts and
BackupImportReport surfaces partial-label imports to the user.
In `@rust/src/label_manager.rs`:
- Around line 335-348: The code in save_imported_labels computes imported from
labels.len() before calling self.db.labels.insert_labels, which can overstate
actual inserts if insert_labels deduplicates/updates; change the implementation
so insert_labels returns an outcome (e.g., counts of inserted vs updated) and
compute imported from that result instead of labels.len(), update the call site
in save_imported_labels to use the returned inserted count when building
LabelImportResult, and preserve the mark_cloud_backup_dirty() behavior and error
mapping via LabelManagerError::Save.
---
Outside diff comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt`:
- Around line 63-81: The snackbar about skipped labels can be suppressed by
early return when currentManager.rust.getTransactions() throws; before calling
getTransactions() (in the block using currentManager.rust.labelManager()), check
result.skipped and show the skipped-count snackbar via
snackbarHostState.showSnackbar("... ${result.skipped} ...") (or build a combined
message) so the user is always informed; keep the existing refresh attempt with
currentManager.rust.getTransactions() and its fallback snackbar, but do not
return before emitting the skipped-labels notification.
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift`:
- Around line 349-371: The import branch in SelectedWalletScreen currently
ignores the return of labelManager.import(jsonl:) and always shows a success
alert; change it to capture the returned LabelImportResult, check its skipped
count, and set app.alertState to either a full success message or a
partial-success message ("Labels Imported / Could not import X labels") when
result.skipped > 0 (mirroring the QR flow handling). Keep the Task { await
manager.rust.getTransactions() } call after a successful/partial import so
transactions refresh, and reference LabelImportResult and the skipped property
when constructing the alert text.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt`:
- Around line 356-362: The alert message in AppManager.kt always uses "labels"
regardless of result.skipped; update the TaggedItem(AppAlertState.General(...))
assignment (where alertState is set) to pluralize correctly by checking
result.skipped and using "label" when result.skipped == 1 and "labels" otherwise
(or call the existing pluralization helper used by MoreInfoPopover.kt if
available), so the message reads "Could not import 1 label" vs "Could not import
N labels".
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift`:
- Around line 432-449: The success branch in SelectedWalletScreen.swift uses a
generic .general alert instead of the app's standard .importedLabelsSuccessfully
case; update the zero-skipped branch where you handle result from
labelManager.importLabels(labels:) to set app.alertState =
.init(.importedLabelsSuccessfully) (instead of the current .general(...)
message) so behavior matches other import flows, leaving the skipped>0 branch
unchanged and keeping the subsequent Task { await manager.rust.getTransactions()
} call.
In `@rust/src/backup/import.rs`:
- Around line 525-547: Replace the manual .map_err(|e|
BackupError::Restore(e.to_string())) calls with the ResultExt::map_err_str
helper: in import_labels (which calls manager.import(...)) and in
restore_wallet_labels where manager.import_without_cloud_backup_dirty(...) is
used, import the trait (e.g., use cove_util::result_ext::ResultExt as _;) and
call map_err_str to convert errors to BackupError::Restore uniformly; this
removes the closure anti-pattern and keeps behavior identical while relying on
the ResultExt helper.
In `@rust/src/label_manager.rs`:
- Around line 575-577: The code returns
Err(LabelManagerError::Parse(first_error.unwrap())) assuming first_error is Some
when parsed.is_empty(); make this defensive by replacing the unwrap with a safe
extraction (e.g., match or unwrap_or/unwrap_or_else) that supplies a sensible
fallback error when first_error is None, so in the parsed.is_empty() branch you
always construct LabelManagerError::Parse with either the actual parse error or
a default error value/message; update the use sites around parsed and
first_error in label_manager.rs to avoid .unwrap().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1734a067-830e-4926-9e9f-fd366a3e5b25
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.ktios/Cove/CoveMainView.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/src/backup/import.rsrust/src/label_manager.rs
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
c1eee31 to
4bded16
Compare
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
4bded16 to
1f8193a
Compare
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
1f8193a to
d1e5124
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
I found three correctness issues that still need to be addressed before this is ready:
-
rust/src/label_manager.rs:318-320
importLabels()reports all parsed labels as imported because it useslabels.len(), butLabelsTable::insert_label_with_write_txn()still drops unsupported BIP329 variants likepubkeyandxpubin its_arm. That means scanner imports can show full success even when some records were never persisted. The import report should be computed after filtering to the label types the wallet can actually store. -
android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt:355-356
The skipped-label handling still does not apply to QR/camera imports with a malformed JSONL line.MultiFormat::try_from_string()buildsBip329Labelswith the strictbip329::Labels::try_from_strparser, so those scans fail beforeimportLabels(multiFormat.v1)is ever reached. Scanner imports should also go through the tolerant parser so valid labels are still imported. -
android/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.kt:91-94
Partial NFC imports are still reported as failures. This code callsonSuccess()and then immediately callsonError()when some labels are skipped.WalletSheetsHosttreatsonErroras a hard failure, so the user gets aFailed to import labels...snackbar even though labels were imported and transactions were refreshed. Partial imports need a success-with-warning path instead of reusing the failure callback.
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
d1e5124 to
cb36212
Compare
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
cb36212 to
695b5db
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt (1)
67-78:⚠️ Potential issue | 🟡 MinorDon’t lose partial-import details when refresh fails.
If
getTransactions()throws after a partial import, the earlyreturn@launchskips theresult.skippedbranch and shows a full-success message.💬 Proposed restructuring
// refresh transactions with updated labels +val importMessage = + if (result.skipped > 0u) { + val noun = if (result.skipped == 1u) "label" else "labels" + "Labels imported. Could not import ${result.skipped} $noun" + } else { + "Labels imported successfully" + } + try { currentManager.rust.getTransactions() } catch (refreshError: Exception) { android.util.Log.e(tag, "failed to refresh transactions after label import", refreshError) - snackbarHostState.showSnackbar("Labels imported successfully, but transaction list may need manual refresh") + snackbarHostState.showSnackbar("$importMessage, but transaction list may need manual refresh") return@launch } -if (result.skipped > 0u) { - val noun = if (result.skipped == 1u) "label" else "labels" - snackbarHostState.showSnackbar("Labels imported. Could not import ${result.skipped} $noun") -} else { - snackbarHostState.showSnackbar("Labels imported successfully") -} +snackbarHostState.showSnackbar(importMessage)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt` around lines 67 - 78, The catch block for currentManager.rust.getTransactions() returns early and thus suppresses the subsequent result.skipped handling; change the flow so partial-import details are always shown: in MoreInfoPopover.kt, when catching the refreshError (currentManager.rust.getTransactions()), log the error with android.util.Log.e(tag,...), show the "Labels imported successfully, but transaction list may need manual refresh" snackbar, but do NOT return@launch; instead continue to evaluate result.skipped and call snackbarHostState.showSnackbar("Labels imported. Could not import ${result.skipped} $noun") when result.skipped > 0u so partial-import info is preserved. Ensure the error path still communicates both the refresh failure and any skipped count to the user.rust/src/backup/import.rs (1)
525-546:⚠️ Potential issue | 🟡 MinorUse
map_err_strfor these error conversions.Both
import()andimport_without_cloud_backup_dirty()returnResult<LabelParseReport, LabelManagerError>, which converts toBackupError::Restore(String). Themap_err_strhelper is cleaner and aligns with repository patterns.♻️ Proposed cleanup
+use cove_util::ResultExt as _; + fn import_labels(id: &WalletId, jsonl: &str) -> Result<(), BackupError> { let manager = LabelManager::new(id.clone()); - manager.import(jsonl).map(drop).map_err(|e| BackupError::Restore(e.to_string())) + manager.import(jsonl).map(drop).map_err_str(BackupError::Restore) } ... LabelRestoreBehavior::PreserveCloudBackupClean => manager .import_without_cloud_backup_dirty(jsonl) .map(drop) - .map_err(|error| BackupError::Restore(error.to_string())), + .map_err_str(BackupError::Restore),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` around lines 525 - 546, Replace the manual error string conversions with the map_err_str helper: in import_labels use manager.import(jsonl).map(drop).map_err(map_err_str) (so it returns BackupError::Restore via the helper) and in restore_wallet_labels replace the .map_err(|error| BackupError::Restore(error.to_string())) on manager.import_without_cloud_backup_dirty(jsonl).map(drop) with .map_err(map_err_str); target the import_labels function and the call site inside restore_wallet_labels that calls import_without_cloud_backup_dirty.
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.kt (1)
80-96: Key the NFC collector to its real dependencies.
onPartialSuccess,onSuccess, andonErrorare captured inside a long-livedLaunchedEffect(Unit), so recomposition can leave this collector calling stale callbacks. UserememberUpdatedStatefor callbacks and key the effect to stable dependencies such asnfcReaderandlabelManager.♻️ Suggested shape
+val currentOnSuccess by rememberUpdatedState(onSuccess) +val currentOnPartialSuccess by rememberUpdatedState(onPartialSuccess) +val currentOnError by rememberUpdatedState(onError) + - LaunchedEffect(Unit) { + LaunchedEffect(nfcReader, labelManager) { nfcReader.startScanning() nfcReader.scanResults.collect { result -> ... - onPartialSuccess(importResult.skipped) + currentOnPartialSuccess(importResult.skipped) ... - onSuccess() + currentOnSuccess() ... - onError("No text data found on NFC tag") + currentOnError("No text data found on NFC tag") ... - onError(result.message) + currentOnError(result.message)Per coding guidelines: "LaunchedEffect should be keyed to actual dependencies, not Unit".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.kt` around lines 80 - 96, The LaunchedEffect in NfcLabelImportSheet is keyed to Unit and captures potentially-stale callbacks (onPartialSuccess, onSuccess, onError); change it to key the effect to the actual stable dependencies (nfcReader and labelManager) and wrap each callback with rememberUpdatedState so the collector always calls the latest lambda. Specifically, create val currentOnPartialSuccess = rememberUpdatedState(onPartialSuccess) (and equivalents for onSuccess/onError), then use LaunchedEffect(nfcReader, labelManager) { ... collect { ... use currentOnPartialSuccess.value(), currentOnSuccess.value(), currentOnError.value() ... } } to avoid stale captures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt`:
- Around line 303-546: Remove the dead AppManager.handleMultiFormat() method
(the entire function body and its signature) since scanning now funnels through
Scanner.handleMultiFormat() via ScanManager (used by MainActivity, NfcScanSheet,
SendFlowHardwareScreen); after deletion, remove or tidy any imports/fields that
become unused because of the method removal and run a build/check to ensure no
references remain to AppManager.handleMultiFormat.
In `@android/app/src/main/java/org/bitcoinppl/cove/ScanManager.kt`:
- Around line 72-77: The alert message for skipped imports in ScanManager.kt
currently always uses the plural "labels"; update the message construction where
app.alertState is set (TaggedItem -> AppAlertState.General) to use singular
"label" when result.skipped == 1u and plural "labels" otherwise (e.g., choose
the word based on result.skipped or build the message with a
conditional/ternary-like expression so "Could not import 1 label" is shown for
one skipped item and "Could not import N labels" for all other counts).
In `@android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt`:
- Around line 298-308: In the onPartialSuccess lambda, preserve and display the
actual skipped count in both success and failure paths: when calling
snackbarHostState.showSnackbar inside the catch block (after onDismissNfcScanner
and manager.rust.getTransactions()), include the skipped variable in the message
(e.g., "Labels imported (X skipped)") instead of the generic "some entries
skipped"; also log the caught exception details using android.util.Log.e along
with the tag to aid debugging while keeping the skipped count in the UI message.
In `@ios/Cove/ScanManager.swift`:
- Around line 39-43: The alert message uses a hardcoded plural ("labels") when
displaying report.skipped, causing "Could not import 1 labels"; update both
places that set app.alertState (the .general alert initialization where
report.skipped is interpolated) to choose singular "label" when report.skipped
== 1 and plural "labels" otherwise, e.g., compute a small pluralized message
string (based on report.skipped) and pass that into the .general(title:message:)
initializer so the alert reads correctly for 0/1/>1 skipped entries.
In `@rust/src/label_manager.rs`:
- Around line 240-247: _import_labels currently receives an Arc<Bip329Labels>
that was created after silently dropping invalid BIP329 lines, so the returned
LabelParseReport shows skipped = 0; fix by either (A) routing the raw BIP329
JSONL through the existing import(&str) path so parsing/skip-counting happens
there before calling import_labels, or (B) augmenting Bip329Labels to carry a
skipped_count field (set where filter_map(...ok()) currently drops items in
multi_format.rs) and update MultiFormat/import_labels and _import_labels to
propagate that skipped_count into LabelParseReport; update LabelParseReport
population to include that skipped value so mobile clients see correct skipped
counts.
- Around line 313-331: parse_labels currently treats all deserialized Label
variants as imported even though insert_label_with_write_txn silently drops
PublicKey and ExtendedPublicKey; update parse_labels to compute supported vs
skipped the same way import_labels does: iterate the parsed Labels and count
supported variants using the same matches check (Label::Transaction,
Label::Address, Label::Input, Label::Output), set imported = supported and
skipped = total - supported in the LabelParseReport, then pass the original
labels and that report into save_imported_labels so raw JSONL imports report
skipped BIP329 variants consistently with import_labels.
In `@rust/src/multi_format.rs`:
- Around line 145-154: The parsing code for bip329 labels currently drops parse
failures via bip329::Label::try_from_str(...).ok() so the LabelParseReport never
sees how many lines failed; change the flow to count parse failures and
propagate that count into import logic (either by extending Bip329Labels to
carry a skipped: u32 alongside labels or by returning both parsed labels and
skipped count to the existing importLabels call) so LabelManager.importLabels()
can aggregate parse-time and import-time skips into LabelParseReport; while
here, remove the redundant .trim() on each line (string was already trimmed),
and log parse failures with debug!/warn! including the offending line or error
for diagnosability.
---
Outside diff comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt`:
- Around line 67-78: The catch block for currentManager.rust.getTransactions()
returns early and thus suppresses the subsequent result.skipped handling; change
the flow so partial-import details are always shown: in MoreInfoPopover.kt, when
catching the refreshError (currentManager.rust.getTransactions()), log the error
with android.util.Log.e(tag,...), show the "Labels imported successfully, but
transaction list may need manual refresh" snackbar, but do NOT return@launch;
instead continue to evaluate result.skipped and call
snackbarHostState.showSnackbar("Labels imported. Could not import
${result.skipped} $noun") when result.skipped > 0u so partial-import info is
preserved. Ensure the error path still communicates both the refresh failure and
any skipped count to the user.
In `@rust/src/backup/import.rs`:
- Around line 525-546: Replace the manual error string conversions with the
map_err_str helper: in import_labels use
manager.import(jsonl).map(drop).map_err(map_err_str) (so it returns
BackupError::Restore via the helper) and in restore_wallet_labels replace the
.map_err(|error| BackupError::Restore(error.to_string())) on
manager.import_without_cloud_backup_dirty(jsonl).map(drop) with
.map_err(map_err_str); target the import_labels function and the call site
inside restore_wallet_labels that calls import_without_cloud_backup_dirty.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.kt`:
- Around line 80-96: The LaunchedEffect in NfcLabelImportSheet is keyed to Unit
and captures potentially-stale callbacks (onPartialSuccess, onSuccess, onError);
change it to key the effect to the actual stable dependencies (nfcReader and
labelManager) and wrap each callback with rememberUpdatedState so the collector
always calls the latest lambda. Specifically, create val currentOnPartialSuccess
= rememberUpdatedState(onPartialSuccess) (and equivalents for
onSuccess/onError), then use LaunchedEffect(nfcReader, labelManager) { ...
collect { ... use currentOnPartialSuccess.value(), currentOnSuccess.value(),
currentOnError.value() ... } } to avoid stale captures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18a6dbf8-df1d-4fc4-8735-f06f37684527
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (13)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/CoveMainView.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/ScanManager.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/src/backup/import.rsrust/src/label_manager.rsrust/src/multi_format.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- ios/CoveCore/Sources/CoveCore/CoveCore.swift
- ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
2 times, most recently
from
6b243bd to
8732227
Compare
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
8732227 to
1b47d11
Compare
Hey @praveenperera addressed all LabelParseReport with imported/skipped counts is wired up across Rust iOS and Android. All import paths (QR, NFC, file) now go through the tolerant line by line parser and surface partial import warnings to the user. Also hit a pretty large merge conflict during rebase since this overlapped with #685 took a bit to ort out cleanly. Should be good for another look when you get a chance! |
|
also squashed the commits into 1 to keep the repo clean. |
|
thanks but don't squash anymore @Sandipmandal25, makes it harder to review just the changes made since last review, thanks |
praveenperera
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
1b47d11 to
819a2d5
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
looks, good almost ready, just some small changes and some questions.
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
819a2d5 to
25248f2
Compare
done the chnages maybe you can took a final look. |
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
25248f2 to
f381fab
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
rust/src/backup/import.rs (1)
527-527: Prefermap_err_strper repo guidelines.- manager.import(jsonl).map_err(|e| BackupError::Restore(e.to_string()))?; + manager.import(jsonl).map_err_str(BackupError::Restore)?;Requires importing
cove_util::ResultExt. Same applies to line 546 (.map_err(|error| BackupError::Restore(error.to_string()))).As per coding guidelines: "Use
cove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string()))— it's cleaner and equivalent".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` at line 527, Replace the explicit closures that convert errors to BackupError::Restore with the repo helper: import the trait via use cove_util::ResultExt; and change manager.import(jsonl).map_err(|e| BackupError::Restore(e.to_string()))? and the similar .map_err(|error| BackupError::Restore(error.to_string())) occurrences to use .map_err_str(BackupError::Restore)? so the ResultExt helper handles the to_string conversion cleanly.rust/src/multi_format.rs (1)
145-154: Tolerant parse loses diagnosability and skipped-count telemetry.Two residual issues in this block (previously flagged and marked addressed, but still present in the shown code):
- Line 148:
.trim()is redundant —stringwas already trimmed at line 105.- Parse failures are silently dropped via
filter_map(... .ok())with nodebug!/warn!logging, so malformed JSONL lines are undiagnosable from logs. Even thoughLabelManager::importlater counts import-time skips, parse-time drops at this layer won't appear in logs for support/debug.♻️ Proposed fix
- let parsed_labels: Vec<bip329::Label> = string - .trim() - .lines() - .filter_map(|line| bip329::Label::try_from_str(line.trim()).ok()) - .collect(); + let parsed_labels: Vec<bip329::Label> = string + .lines() + .filter_map(|line| { + let line = line.trim(); + if line.is_empty() { + return None; + } + match bip329::Label::try_from_str(line) { + Ok(label) => Some(label), + Err(e) => { + debug!("skipping unparseable BIP329 line: {e}"); + None + } + } + }) + .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/multi_format.rs` around lines 145 - 154, The parsing block currently re-trims an already-trimmed `string` and silently discards parse errors via `filter_map(... .ok())`; change the loop that builds `parsed_labels` to iterate over `string.lines()` (no extra `.trim()`), attempt `bip329::Label::try_from_str` for each line, collect successes into `parsed_labels` and for failures log a `debug!` or `warn!` (including the offending line and the parse error) so malformed JSONL is diagnosable; keep the existing return of `Self::Bip329Labels(Arc::new(bip329::Labels::new(parsed_labels).into()))` when non-empty and ensure any skipped-count telemetry continues to be tracked by `LabelManager::import` or by incrementing a local counter reported via logs.rust/src/label_manager.rs (1)
602-652: Test coverage is solid; consider one more case.The five cases cover the core matrix well. One small gap: a test where all lines are valid JSON
Labels but unsupported variants (e.g.,pubkey/xpub) would lock in the behavior of theall-skipped-as-unsupportedbranch (see comment on lines 586–588) and guard against regressions ifparse_labels's semantics change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/label_manager.rs` around lines 602 - 652, Add a test that feeds parse_labels valid JSONL lines whose "type" fields are supported-by-JSON-but-unsupported-by-parse_labels (e.g., "pubkey" and "xpub") to exercise the "all-skipped-as-unsupported" branch: construct a JSONL string of two valid Label objects with types "pubkey" and "xpub", call parse_labels(&jsonl).unwrap(), then assert labels.is_empty(), report.imported == 0, and report.skipped == 2; reference parse_labels and the Label variant names ("pubkey", "xpub") when adding the new test in the tests module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/label_manager.rs`:
- Around line 318-322: The is_supported closure definition in label_manager.rs
is exceeding rustfmt width and caused CI to fail; run cargo fmt (or rustfmt) to
reformat the closure so its body is wrapped onto multiple lines, ensuring the
matches! arm for Label::Transaction/Address/Input/Output is line-wrapped; locate
the is_supported closure near the comment and insert_label_with_write_txn usage
to verify formatting after running cargo fmt.
- Around line 249-252: After parsing in import(), guard against empty input:
call parse_labels(jsonl) as before, then if labels.is_empty() return Ok(report)
(i.e., do an early return) or pass mark_cloud_backup_dirty = !labels.is_empty()
to save_imported_labels so no cloud-dirty flag is set for a no-op. Update the
function import to use the labels variable from parse_labels and only mark cloud
backup dirty / call save_imported_labels when at least one label will be
inserted.
- Around line 586-588: When parsed.is_empty() currently returns
Err(LabelManagerError::Parse(first_error.unwrap_or_default())), change the logic
in the function that builds `parsed`/`first_error` so you distinguish two cases:
(1) parsed.is_empty() && first_error.is_some() => return
LabelManagerError::Parse(the original error), and (2) parsed.is_empty() &&
first_error.is_none() => return a clearer error indicating "all labels were
skipped as unsupported" (either add a new enum variant like
LabelManagerError::AllUnsupported or return LabelManagerError::Parse with an
explicit message such as "all labels were skipped as unsupported
(pubkey/xpub)"); update any call sites/tests expecting the previous empty-string
parse error to use the new message/variant. Ensure references to `parsed`,
`first_error`, and `LabelManagerError::Parse` are adjusted accordingly.
---
Nitpick comments:
In `@rust/src/backup/import.rs`:
- Line 527: Replace the explicit closures that convert errors to
BackupError::Restore with the repo helper: import the trait via use
cove_util::ResultExt; and change manager.import(jsonl).map_err(|e|
BackupError::Restore(e.to_string()))? and the similar .map_err(|error|
BackupError::Restore(error.to_string())) occurrences to use
.map_err_str(BackupError::Restore)? so the ResultExt helper handles the
to_string conversion cleanly.
In `@rust/src/label_manager.rs`:
- Around line 602-652: Add a test that feeds parse_labels valid JSONL lines
whose "type" fields are supported-by-JSON-but-unsupported-by-parse_labels (e.g.,
"pubkey" and "xpub") to exercise the "all-skipped-as-unsupported" branch:
construct a JSONL string of two valid Label objects with types "pubkey" and
"xpub", call parse_labels(&jsonl).unwrap(), then assert labels.is_empty(),
report.imported == 0, and report.skipped == 2; reference parse_labels and the
Label variant names ("pubkey", "xpub") when adding the new test in the tests
module.
In `@rust/src/multi_format.rs`:
- Around line 145-154: The parsing block currently re-trims an already-trimmed
`string` and silently discards parse errors via `filter_map(... .ok())`; change
the loop that builds `parsed_labels` to iterate over `string.lines()` (no extra
`.trim()`), attempt `bip329::Label::try_from_str` for each line, collect
successes into `parsed_labels` and for failures log a `debug!` or `warn!`
(including the offending line and the parse error) so malformed JSONL is
diagnosable; keep the existing return of
`Self::Bip329Labels(Arc::new(bip329::Labels::new(parsed_labels).into()))` when
non-empty and ensure any skipped-count telemetry continues to be tracked by
`LabelManager::import` or by incrementing a local counter reported via logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c636f0b-ab08-48ef-ba3c-17216a778090
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (11)
android/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/nfc/NfcLabelImportSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/ScanManager.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/src/backup/import.rsrust/src/label_manager.rsrust/src/multi_format.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt
- ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
- ios/Cove/ScanManager.swift
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
f381fab to
d66cb28
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
I found three actionable issues in the diff. The main one is that MultiFormat still drops malformed BIP329 rows before LabelManager sees them, so the scan/file-open paths that import MultiFormat::Bip329Labels can partially import labels while still reporting full success. There is also an iOS file-import path that still ignores the new LabelParseReport, and import_labels() currently treats unsupported-only payloads as a successful import. I could not verify prior PR discussion in this environment because prc was unavailable and both requested gh calls failed to reach GitHub, so this review is based on the checked-out diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/src/multi_format.rs (1)
378-389: Consider removingDerefimplementation in favor ofAsRefonly (optional).Implementing
Derefon a non-smart-pointer wrapper likeBip329Labelsis flagged by the Rust API Guidelines (C-DEREF: "Deref and DerefMut are implemented only for smart pointers"), because deref coercion can make method resolution surprising. Verification confirms that no call sites rely on deref coercion; all uses explicitly access.labelsfield directly. TheAsRef<bip329::Labels>implementation (already present) provides an explicit alternative when needed, and the public field ensures direct field access is available. Low priority — flagging since the type ispuband exposed viauniffi::Object.♻️ Proposed refactor
-impl std::ops::Deref for Bip329Labels { - type Target = bip329::Labels; - fn deref(&self) -> &Self::Target { - &self.labels - } -} - impl AsRef<bip329::Labels> for Bip329Labels { fn as_ref(&self) -> &bip329::Labels { &self.labels } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/multi_format.rs` around lines 378 - 389, Remove the std::ops::Deref implementation for Bip329Labels and rely solely on the existing AsRef<bip329::Labels> impl and the public labels field; specifically delete the impl block named "impl std::ops::Deref for Bip329Labels" (the deref method returning &self.labels) so the type no longer provides deref coercion, leaving the "impl AsRef<bip329::Labels> for Bip329Labels" and the pub labels field intact; after removal, run cargo check/CI to ensure no call sites relied on deref coercion and update any callers to use .as_ref() or .labels explicitly if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift`:
- Around line 352-368: The import alert logic incorrectly pluralizes skipped
labels and shows a positive "Labels Imported" title even when nothing was
imported; extract the duplicated alert construction into a helper like
labelImportAlertState(for result: LabelParseReport) that returns an
AppAlertState, implement singular/plural for "label"/"labels" and choose title
"Import Failed" when result.imported == 0, "Labels Imported" when
result.imported > 0 && result.skipped > 0, else the existing "Success!" message,
then replace the inline alert blocks in the import path (the try
labelManager.import(...) handling) and onChangeOfScannedLabels with calls to
this helper.
---
Nitpick comments:
In `@rust/src/multi_format.rs`:
- Around line 378-389: Remove the std::ops::Deref implementation for
Bip329Labels and rely solely on the existing AsRef<bip329::Labels> impl and the
public labels field; specifically delete the impl block named "impl
std::ops::Deref for Bip329Labels" (the deref method returning &self.labels) so
the type no longer provides deref coercion, leaving the "impl
AsRef<bip329::Labels> for Bip329Labels" and the pub labels field intact; after
removal, run cargo check/CI to ensure no call sites relied on deref coercion and
update any callers to use .as_ref() or .labels explicitly if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8649379-0a27-4ebf-ae58-62677259b0d7
📒 Files selected for processing (4)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftrust/src/backup/import.rsrust/src/label_manager.rsrust/src/multi_format.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/label_manager.rs
| let result = try labelManager.import(jsonl: fileContents) | ||
|
|
||
| app.alertState = .init( | ||
| .general( | ||
| title: "Success!", | ||
| message: "Labels have been imported successfully." | ||
| if result.skipped > 0 { | ||
| app.alertState = .init( | ||
| .general( | ||
| title: "Labels Imported", | ||
| message: "Could not import \(result.skipped) labels" | ||
| ) | ||
| ) | ||
| ) | ||
| } else { | ||
| app.alertState = .init( | ||
| .general( | ||
| title: "Success!", | ||
| message: "Labels have been imported successfully." | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Minor polish: pluralization and misleading title when nothing imported.
Two small UX issues with the new partial-import alert:
"Could not import \(result.skipped) labels"will render as"Could not import 1 labels"when exactly one row is skipped. The AI-generated summary suggests singular/plural handling was intended here, but the code doesn't implement it.- The title
"Labels Imported"is shown wheneverresult.skipped > 0, even ifresult.imported == 0(i.e., every row was invalid). In that case the user sees a positive-sounding title for what is effectively a failed import.
Also note that this entire alert block is duplicated almost verbatim in onChangeOfScannedLabels (Lines 441–456) — consider extracting a small helper like labelImportAlert(for: LabelParseReport) to keep the two paths in sync.
🛠️ Suggested refactor
Add a helper and reuse from both import paths:
private func labelImportAlertState(for result: LabelParseReport) -> AppAlertState {
if result.imported == 0 {
let noun = result.skipped == 1 ? "label" : "labels"
return .init(.general(
title: "Import Failed",
message: "Could not import \(result.skipped) \(noun)."
))
}
if result.skipped > 0 {
let noun = result.skipped == 1 ? "label" : "labels"
return .init(.general(
title: "Labels Imported",
message: "Could not import \(result.skipped) \(noun)."
))
}
return .init(.general(
title: "Success!",
message: "Labels have been imported successfully."
))
}Then at the call site:
- if result.skipped > 0 {
- app.alertState = .init(
- .general(
- title: "Labels Imported",
- message: "Could not import \(result.skipped) labels"
- )
- )
- } else {
- app.alertState = .init(
- .general(
- title: "Success!",
- message: "Labels have been imported successfully."
- )
- )
- }
+ app.alertState = labelImportAlertState(for: result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift` around lines
352 - 368, The import alert logic incorrectly pluralizes skipped labels and
shows a positive "Labels Imported" title even when nothing was imported; extract
the duplicated alert construction into a helper like labelImportAlertState(for
result: LabelParseReport) that returns an AppAlertState, implement
singular/plural for "label"/"labels" and choose title "Import Failed" when
result.imported == 0, "Labels Imported" when result.imported > 0 &&
result.skipped > 0, else the existing "Success!" message, then replace the
inline alert blocks in the import path (the try labelManager.import(...)
handling) and onChangeOfScannedLabels with calls to this helper.
6ae7eaf to
e3904d3
Compare
Sandipmandal25
force-pushed
the
fix/nunchuck-label-vout-parse
branch
from
e3904d3 to
8c86ab5
Compare
2 participants
Summary
Fixes #277
When importing BIP329 labels from Nunchuck, a single entry with an unrecognized ref format (e.g. non-numeric
vout) caused the entire import to fail with"Unable to parse file: error parsing vout", resulting in all labels being lost.parse_labelsnow processes input line-by-line: valid entries are imported, invalid ones are logged and skipped. The import only fails if every line is unrecognized.Testing
5 unit tests added covering:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests