Fix descriptor export for Sparrow compatibility

[praveenperera]

Export descriptors as a single BIP-389 multipath descriptor with <0;1> notation and h-hardened notation, matching Coldcard's default format.

BDK's miniscript uses ' (apostrophe) for hardened steps, but Sparrow normalizes to h before validating the checksum, which causes a mismatch. The new to_normalized_string() and to_multipath_string() methods on DescriptorExt produce h-notation strings with correctly recomputed checksums.

Summary by CodeRabbit

• Bug Fixes

  • Improved descriptor checksum handling to be consistent across notation variants and exports.

• New Features

  • Added multipath (BIP-389) export: external/internal paths are combined into a single descriptor when compatible.
  • Added canonical descriptor normalization for consistent display and export; export now favors a single multipath line with a two-line fallback.

• Tests

  • Added tests for checksum, normalization, multipath exports, and matching/mismatching descriptor pairs.

• Documentation

  • Clarified export behavior in public docs.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds descriptor normalization, checksum recomputation, BIP-389 multipath generation and export formatting to the descriptor extension; updates wallet export to use the new export string; adds tests and maps a new NotMatchingPair error into the internal parse error conversion.

Changes

Cohort / File(s)	Summary
Descriptor extension rust/crates/cove-bdk/src/descriptor_ext.rs	Imports descriptor::checksum::desc_checksum, adds Error::NotMatchingPair, extends DescriptorExt with to_normalized_string, to_multipath_string, to_export_string, and implements them for Descriptor<DescriptorPublicKey> (h-notation normalization, <0;1> multipath rewrite, checksum recompute, fallback two-line export).
Wallet export integration rust/src/manager/wallet_manager.rs	get_public_descriptor_content() now returns DescriptorExt::to_export_string(external, internal) (uses multipath when valid; otherwise two-line normalized export). Added doc comment describing behavior.
Error mapping & tests rust/src/keys.rs, tests/...	From<cove_bdk::descriptor_ext::Error> for DescriptorKeyParseError now maps NotMatchingPair to UnsupportedDescriptor(...). Large suite of unit tests added/updated covering checksum normalization, multipath formation, matching/mismatching descriptor pairs, and various descriptor round-trips.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I hop through strings and checksums bright,

I swap ' for h by soft moonlight.
I braid two paths into one neat line,
recompute the knot, and call it fine.
A rabbit cheers: descriptors align!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly describes the main change: fixing descriptor export to achieve Sparrow compatibility. This directly reflects the PR's primary objective of addressing checksum mismatches by converting to h-notation and multipath format for Sparrow compatibility.
Docstring Coverage	✅ Passed	Docstring coverage is 89.66% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch descriptor-sparrow-compatibilty

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4308a20 and 0b2d055.

📒 Files selected for processing (3)

• rust/crates/cove-bdk/src/descriptor_ext.rs
• rust/src/keys.rs
• rust/src/manager/wallet_manager.rs

[greptile-apps]

Greptile Summary

This PR fixes a Sparrow wallet compatibility issue by replacing the raw BDK descriptor output (which uses ' apostrophe hardened notation with a mismatched checksum) with properly normalized descriptors that use h hardened notation and recomputed checksums, and optionally combines them into a single BIP-389 multipath <0;1> descriptor.

Key changes:

• Adds to_normalized_string() on DescriptorExt to produce h-notation descriptors with correctly recomputed checksums
• Adds to_multipath_string() to combine external/internal descriptor pairs into a single BIP-389 <0;1> descriptor, validating the pair matches beyond the keychain index
• Adds to_export_string() as the top-level export path that tries multipath first and falls back to two normalized descriptors on a single newline
• Updates get_public_descriptor_content in wallet_manager.rs to use to_export_string for both the keychain and BDK wallet fallback paths
• Includes thorough test coverage validating checksum correctness across notation variants, roundtrips, and BDK wallet public descriptors

Confidence Score: 5/5

• Safe to merge. The fix correctly addresses the Sparrow checksum mismatch for all supported descriptor types with proper validation and test coverage.
• The core logic is sound and well-tested. The ' → h replacement with checksum recomputation is the correct approach to fix the Sparrow normalization issue. The string-based pattern matching for multipath construction works correctly for all supported descriptor types (wpkh, pkh, tr, sh(wpkh)) since /0/*) appears exactly once at the keychain boundary by design. One minor doc/code inconsistency remains but does not affect functionality.
• No files require special attention.

_{Last reviewed commit: f7d380a}

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[praveenperera]

@greptile-apps re-review

[coderabbitai]

♻️ Duplicate comments (1)

rust/crates/cove-bdk/src/descriptor_ext.rs (1)

93-96: ⚠️ Potential issue | 🟠 Major

Validate and use internal before producing the <0;1> descriptor.

Line 93 currently ignores internal, so mismatched external/internal descriptors are accepted silently and can yield an invalid combined export.

Proposed fix

-    fn to_multipath_string(external: &Self, _internal: &Self) -> String {
-        let body = format!("{external:#}").replace('\'', "h").replace("/0/*)", "/<0;1>/*)");
+    fn to_multipath_string(external: &Self, internal: &Self) -> String {
+        let ext_body = format!("{external:#}").replace('\'', "h");
+        let int_body = format!("{internal:#}").replace('\'', "h");
+
+        let expected_internal = ext_body.replacen("/0/*)", "/1/*)", 1);
+        assert_eq!(
+            int_body, expected_internal,
+            "external/internal descriptors are not a matching keychain pair"
+        );
+
+        let body = ext_body.replacen("/0/*)", "/<0;1>/*)", 1);
         let checksum = desc_checksum(&body).expect("valid descriptor body");
         format!("{body}#{checksum}")
     }

As per coding guidelines, rust/**/*.rs: Verify correct functionality and logic consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/crates/cove-bdk/src/descriptor_ext.rs` around lines 93 - 96, The
to_multipath_string function currently ignores the provided internal descriptor;
change it to accept and use the internal descriptor (remove the leading
underscore from the parameter) and validate that external and internal are
consistent before generating the combined export. Specifically, in
to_multipath_string(external: &Self, internal: &Self) produce the body using the
internal descriptor where the <0;1> replacement is needed (e.g.,
format!("{internal:#}") or construct the multipath body from both descriptors),
and add a sanity check (compare structure or relevant fields of external and
internal, panic or return an error if they mismatch) before computing
desc_checksum and returning the final "{body}#{checksum}" string.

🧹 Nitpick comments (1)

rust/src/keys.rs (1)

488-495: Make checksum helper parsing strict with split_once('#').

extract_checksum/compute_checksum currently accept missing # implicitly. Requiring a separator gives clearer, immediate test failures.

Proposed refactor

     fn extract_checksum(desc_str: &str) -> &str {
-        desc_str.rsplit('#').next().unwrap()
+        desc_str
+            .split_once('#')
+            .map(|(_, checksum)| checksum)
+            .expect("descriptor must include checksum")
     }

     /// Helper: compute expected checksum from descriptor body (before '#')
     fn compute_checksum(desc_str: &str) -> String {
-        let body = desc_str.split('#').next().unwrap();
+        let (body, _) =
+            desc_str.split_once('#').expect("descriptor must include checksum");
         bdk_wallet::miniscript::descriptor::checksum::desc_checksum(body).unwrap()
     }

As per coding guidelines, rust/**/*.rs: Check for idiomatic Rust usage and performance best practices.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/keys.rs` around lines 488 - 495, The helpers extract_checksum and
compute_checksum should require the '#' separator explicitly: change
extract_checksum to use desc_str.split_once('#') (or rsplit_once) and
.expect(...) to return the checksum slice only if the separator exists, and
change compute_checksum to use desc_str.split_once('#').expect(...) to obtain
the descriptor body before calling
bdk_wallet::miniscript::descriptor::checksum::desc_checksum(body).unwrap(); this
makes parsing strict and yields clear failures when '#' is missing; reference
functions: extract_checksum and compute_checksum.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@rust/crates/cove-bdk/src/descriptor_ext.rs`:
- Around line 93-96: The to_multipath_string function currently ignores the
provided internal descriptor; change it to accept and use the internal
descriptor (remove the leading underscore from the parameter) and validate that
external and internal are consistent before generating the combined export.
Specifically, in to_multipath_string(external: &Self, internal: &Self) produce
the body using the internal descriptor where the <0;1> replacement is needed
(e.g., format!("{internal:#}") or construct the multipath body from both
descriptors), and add a sanity check (compare structure or relevant fields of
external and internal, panic or return an error if they mismatch) before
computing desc_checksum and returning the final "{body}#{checksum}" string.

---

Nitpick comments:
In `@rust/src/keys.rs`:
- Around line 488-495: The helpers extract_checksum and compute_checksum should
require the '#' separator explicitly: change extract_checksum to use
desc_str.split_once('#') (or rsplit_once) and .expect(...) to return the
checksum slice only if the separator exists, and change compute_checksum to use
desc_str.split_once('#').expect(...) to obtain the descriptor body before
calling
bdk_wallet::miniscript::descriptor::checksum::desc_checksum(body).unwrap(); this
makes parsing strict and yields clear failures when '#' is missing; reference
functions: extract_checksum and compute_checksum.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b2d055 and 13fa7ee.

📒 Files selected for processing (3)

• rust/crates/cove-bdk/src/descriptor_ext.rs
• rust/src/keys.rs
• rust/src/manager/wallet_manager.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• rust/src/manager/wallet_manager.rs

[praveenperera]

@greptile-apps re-review

[coderabbitai]

🧹 Nitpick comments (1)

rust/crates/cove-bdk/src/descriptor_ext.rs (1)

106-119: Consider using replacen for precise single replacement.

The current implementation uses replace() which replaces all occurrences of /0/*) and /1/*). While standard single-key descriptors typically have only one such pattern, using replacen(..., 1) would be more defensive and align with the previous review suggestion.

♻️ Proposed refinement

     fn to_multipath_string(external: &Self, internal: &Self) -> Result<String> {
         let ext_body = format!("{external:#}").replace('\'', "h");
         let int_body = format!("{internal:#}").replace('\'', "h");

         // both descriptors must be identical except for the keychain index (0 vs 1)
-        let expected_internal = ext_body.replace("/0/*)", "/1/*)");
+        let expected_internal = ext_body.replacen("/0/*)", "/1/*)", 1);
         if int_body != expected_internal {
             return Err(Error::NotMatchingPair);
         }

-        let body = ext_body.replace("/0/*)", "/<0;1>/*)");
+        let body = ext_body.replacen("/0/*)", "/<0;1>/*)", 1);
         let checksum = desc_checksum(&body).expect("valid descriptor body");
         Ok(format!("{body}#{checksum}"))
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/crates/cove-bdk/src/descriptor_ext.rs` around lines 106 - 119, In
to_multipath_string, the code uses ext_body.replace("/0/*)", "/1/*)") and
ext_body.replace("/0/*)", "/<0;1>/*)") which will replace all occurrences;
change both calls to use replacen(..., 1) so only the first occurrence is
replaced (e.g., expected_internal = ext_body.replacen("/0/*)", "/1/*)", 1) and
body = ext_body.replacen("/0/*)", "/<0;1>/*)", 1)), keeping the comparisons with
int_body and subsequent desc_checksum the same.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rust/crates/cove-bdk/src/descriptor_ext.rs`:
- Around line 106-119: In to_multipath_string, the code uses
ext_body.replace("/0/*)", "/1/*)") and ext_body.replace("/0/*)", "/<0;1>/*)")
which will replace all occurrences; change both calls to use replacen(..., 1) so
only the first occurrence is replaced (e.g., expected_internal =
ext_body.replacen("/0/*)", "/1/*)", 1) and body = ext_body.replacen("/0/*)",
"/<0;1>/*)", 1)), keeping the comparisons with int_body and subsequent
desc_checksum the same.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13fa7ee and f7d380a.

📒 Files selected for processing (3)

• rust/crates/cove-bdk/src/descriptor_ext.rs
• rust/src/keys.rs
• rust/src/manager/wallet_manager.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• rust/src/keys.rs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

rust/src/keys.rs (1)

336-344: ⚠️ Potential issue | 🔴 Critical

Fix moved-value usage in From<...Error> conversion.

Line 343 uses error.to_string() inside match error { ... }, but error is consumed by the match and inaccessible in the E::NotMatchingPair arm.

Proposed fix

 impl From<cove_bdk::descriptor_ext::Error> for DescriptorKeyParseError {
     fn from(error: cove_bdk::descriptor_ext::Error) -> Self {
         use cove_bdk::descriptor_ext::Error as E;
         match error {
             E::NoOrigin => Self::NoOrigin,
             E::UnsupportedDescriptor(s) => Self::UnsupportedDescriptor(s),
             E::UnsupportedDescriptorType(s) => Self::UnsupportedDescriptorType(s),
-            E::NotMatchingPair => Self::UnsupportedDescriptor(error.to_string()),
+            e @ E::NotMatchingPair => Self::UnsupportedDescriptor(e.to_string()),
         }
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/keys.rs` around lines 336 - 344, In the
From<cove_bdk::descriptor_ext::Error> for DescriptorKeyParseError impl the match
arm for E::NotMatchingPair tries to call error.to_string() after error is moved
by the match; fix by computing the error string before matching (e.g. let
err_str = error.to_string();) and then match on error, returning
Self::UnsupportedDescriptor(err_str) in the E::NotMatchingPair arm; update the
E::UnsupportedDescriptor/E::UnsupportedDescriptorType arms to keep using their
captured s variables unchanged.

♻️ Duplicate comments (1)

rust/crates/cove-bdk/src/descriptor_ext.rs (1)

106-128: ⚠️ Potential issue | 🟠 Major

Prevent false-positive multipath conversion when /0/*) is not present.

Line 111 and Line 116 use global replace, and a no-op replacement can still return Ok(...). That lets to_export_string (Line 122) emit a single-line export even when no valid external/internal pair was collapsed.

Proposed fix

 fn to_multipath_string(external: &Self, internal: &Self) -> Result<String> {
     let external_body = format!("{external:#}").replace('\'', "h");
     let internal_body = format!("{internal:#}").replace('\'', "h");

-    // both descriptors must be identical except for the keychain index (0 vs 1)
-    let expected_internal = external_body.replace("/0/*)", "/1/*)");
-    if internal_body != expected_internal {
+    // both descriptors must be identical except for exactly one keychain index (0 vs 1)
+    let expected_internal = external_body.replacen("/0/*)", "/1/*)", 1);
+    let body = external_body.replacen("/0/*)", "/<0;1>/*)", 1);
+
+    // require that a replacement actually happened
+    if expected_internal == external_body || body == external_body || internal_body != expected_internal {
         return Err(Error::NotMatchingPair);
     }

-    let body = external_body.replace("/0/*)", "/<0;1>/*)");
     let checksum = desc_checksum(&body).expect("valid descriptor body");
     Ok(format!("{body}#{checksum}"))
 }

As per coding guidelines: rust/**/*.rs: "Verify correct functionality and logic consistency."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/crates/cove-bdk/src/descriptor_ext.rs` around lines 106 - 128, The
current to_multipath_string uses global replace that can be a no-op and still
produce Ok; update to_multipath_string to first verify that external_body
contains the exact substring "/0/*)" and internal_body contains "/1/*)" before
performing the replacement and checksum logic (return
Err(Error::NotMatchingPair) if those substrings are absent), so the
expected_internal check only runs when a real index substitution is possible;
adjust references to external_body, internal_body, and expected_internal
accordingly and keep to_export_string behavior unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@rust/src/keys.rs`:
- Around line 336-344: In the From<cove_bdk::descriptor_ext::Error> for
DescriptorKeyParseError impl the match arm for E::NotMatchingPair tries to call
error.to_string() after error is moved by the match; fix by computing the error
string before matching (e.g. let err_str = error.to_string();) and then match on
error, returning Self::UnsupportedDescriptor(err_str) in the E::NotMatchingPair
arm; update the E::UnsupportedDescriptor/E::UnsupportedDescriptorType arms to
keep using their captured s variables unchanged.

---

Duplicate comments:
In `@rust/crates/cove-bdk/src/descriptor_ext.rs`:
- Around line 106-128: The current to_multipath_string uses global replace that
can be a no-op and still produce Ok; update to_multipath_string to first verify
that external_body contains the exact substring "/0/*)" and internal_body
contains "/1/*)" before performing the replacement and checksum logic (return
Err(Error::NotMatchingPair) if those substrings are absent), so the
expected_internal check only runs when a real index substitution is possible;
adjust references to external_body, internal_body, and expected_internal
accordingly and keep to_export_string behavior unchanged.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7d380a and 7a078df.

📒 Files selected for processing (3)

• rust/crates/cove-bdk/src/descriptor_ext.rs
• rust/src/keys.rs
• rust/src/manager/wallet_manager.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• rust/src/manager/wallet_manager.rs