Add watch-only multisig public descriptor support#8
Open
notTanveer wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a new public ChangesWatch‑only multisig support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/multisig.rs (2)
77-78: Nit: leaking "v1" into the user-facing error message.
"wsh(thresh(...)) is not supported in v1"bakes an implementation/release concept into the message. If a later version adds support, this message becomes stale; if not, the mention is noise to end users. Suggest dropping thein v1suffix.- #[error("Unsupported policy descriptor: wsh(thresh(...)) is not supported in v1")] + #[error("Unsupported policy descriptor: wsh(thresh(...)) is not supported")] UnsupportedPolicyDescriptor,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/multisig.rs` around lines 77 - 78, Update the user-facing error message for the UnsupportedPolicyDescriptor enum variant to remove the implementation/version detail "in v1"; locate the variant named UnsupportedPolicyDescriptor (the #[error(...)] attribute on that enum) and change its message from "Unsupported policy descriptor: wsh(thresh(...)) is not supported in v1" to a version-agnostic string such as "Unsupported policy descriptor: wsh(thresh(...)) is not supported".
172-177: Optional: avoid re-parsing after splitting a multipath descriptor.
singles[i]is already a fully-parsedDescriptor<DescriptorPublicKey>. Stringifying each and callingparse_and_validate_singlere-runs the miniscript parser twice for no added safety. Extracting a helper that takes aDescriptordirectly (and extracts metadata from its canonical string) would be cleaner and avoids a double parse on the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/multisig.rs` around lines 172 - 177, The code is re-parsing each split descriptor via parse_and_validate_single(&singles[i].to_string()), which duplicates work; add a new helper (e.g., parse_and_validate_single_descriptor) that accepts a Descriptor<DescriptorPublicKey> directly, extracts required metadata from the descriptor's canonical string or its existing structure without running the miniscript parser again, and returns the same ext/int result type as parse_and_validate_single; then replace the two calls using parse_and_validate_single(&singles[0].to_string()) and parse_and_validate_single(&singles[1].to_string()) with parse_and_validate_single_descriptor(&singles[0]) and parse_and_validate_single_descriptor(&singles[1]), removing the unnecessary stringify+parse step.src/lib.rs (1)
78-78: Consider documenting multisig in the crate-level "Supported descriptors" list.The rustdoc at the top still says "Supported descriptors: Single Sig" (see lines 11–13). Now that
multisigis a public module wired intoFormat, the crate docs (and ideally a short usage example mirroring the existing ones) should reflect that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` at line 78, Update the crate-level documentation to list "multisig" in the "Supported descriptors" section and add a short usage example demonstrating the multisig module similar to the existing single-sig example; specifically edit the top-level doc comment (crate docs) to change the supported descriptors line to include multisig and add a brief code snippet showing how to construct/use a multisig via the public module multisig and/or its integration point Format so readers can see how to instantiate or parse a multisig descriptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/multisig.rs`:
- Around line 170-190: The current branch treats any non-2 result from
descriptor.into_single_descriptors() as Error::MissingInternalDescriptor which
is misleading; change this to return a dedicated error (e.g.
Error::DescriptorPairMismatch or Error::WrongNumberOfSingleDescriptors) when
singles.len() != 2, with a message stating that exactly two paths (external and
internal, e.g. <0;1>) are required; add the new error variant to the Error enum
(or reuse a clearer existing one) and update any places that match on
Error::MissingInternalDescriptor if they need to distinguish these cases,
keeping the rest of the logic using parse_and_validate_single, validate_pair,
and the construction of Self as-is.
- Around line 333-362: validate_pair currently relies on
normalize_for_comparison which masks branch info; enforce that the parsed
singles have the correct branch roles explicitly after parse_and_validate_single
by checking the original (pre-normalized) descriptor string or ParsedSingle
metadata to ensure external uses /0/* (receive) and internal uses /1/* (change).
In function validate_pair (and/or immediately after
into_single_descriptors()/parse_and_validate_single), add explicit checks that
ext.descriptor.to_string() (or a branch field on ParsedSingle if present)
contains the /0/* pattern and int.descriptor.to_string() contains the /1/*
pattern, returning Error::DescriptorPairMismatch when they do not. Keep the
existing threshold/num_signers/function/structure checks but perform these role
checks before normalizing so order-dependent reversed multipath or
duplicate-external cases fail fast. Add a regression test that supplies the same
/0/* descriptor for both lines and asserts DescriptorPairMismatch.
---
Nitpick comments:
In `@src/lib.rs`:
- Line 78: Update the crate-level documentation to list "multisig" in the
"Supported descriptors" section and add a short usage example demonstrating the
multisig module similar to the existing single-sig example; specifically edit
the top-level doc comment (crate docs) to change the supported descriptors line
to include multisig and add a brief code snippet showing how to construct/use a
multisig via the public module multisig and/or its integration point Format so
readers can see how to instantiate or parse a multisig descriptor.
In `@src/multisig.rs`:
- Around line 77-78: Update the user-facing error message for the
UnsupportedPolicyDescriptor enum variant to remove the implementation/version
detail "in v1"; locate the variant named UnsupportedPolicyDescriptor (the
#[error(...)] attribute on that enum) and change its message from "Unsupported
policy descriptor: wsh(thresh(...)) is not supported in v1" to a
version-agnostic string such as "Unsupported policy descriptor: wsh(thresh(...))
is not supported".
- Around line 172-177: The code is re-parsing each split descriptor via
parse_and_validate_single(&singles[i].to_string()), which duplicates work; add a
new helper (e.g., parse_and_validate_single_descriptor) that accepts a
Descriptor<DescriptorPublicKey> directly, extracts required metadata from the
descriptor's canonical string or its existing structure without running the
miniscript parser again, and returns the same ext/int result type as
parse_and_validate_single; then replace the two calls using
parse_and_validate_single(&singles[0].to_string()) and
parse_and_validate_single(&singles[1].to_string()) with
parse_and_validate_single_descriptor(&singles[0]) and
parse_and_validate_single_descriptor(&singles[1]), removing the unnecessary
stringify+parse step.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc1a287d-f974-4368-82ac-7532ae4be135
📒 Files selected for processing (3)
src/formats.rssrc/lib.rssrc/multisig.rs
notTanveer
force-pushed
the
multisig-desc
branch
from
e23348e to
fe80cad
Compare
Introduces MultisigDescriptorPair and supporting types for parsing watch-only public multisig descriptors before any BDK wallet construction or persistence concerns. Supported input forms: - Single multipath wsh(sortedmulti(k, KEY/<0;1>/*, ...)) — auto-split into external (/0/*) and internal (/1/*) descriptors. - Two-line pair: external descriptor on line 1, internal on line 2. Supported descriptor kinds: wsh(multi(...)), wsh(sortedmulti(...)). Explicitly rejected with typed errors: - Private key material (xprv/tprv) → PrivateKeyMaterialNotAllowed - sh(multi) / sh(sortedmulti) → UnsupportedDescriptorKind - Bare multi / sortedmulti → UnsupportedDescriptorKind - tr(...multi_a / sortedmulti_a...) → UnsupportedDescriptorKind - wsh(thresh(...)) → UnsupportedPolicyDescriptor - Threshold > n → InvalidThreshold - Single non-multipath descriptor → MissingInternalDescriptor - Structural pair mismatch → DescriptorPairMismatch Includes extract_multisig_metadata() for threshold/signer-count extraction from a single descriptor without requiring a full pair (used by tests for spec vectors that are single-descriptor). Tests cover all spec positive vectors (raw pubkeys, xpub-based, Bitcoin Core 2-of-3 tpub pair, multipath split) and all spec negative vectors.
- pub mod multisig in lib.rs so the module is reachable by callers. - Add Format::MultisigDescriptor(MultisigDescriptorPair) variant so the enum covers both single-sig and multisig public descriptors. - Add formats::Error::InvalidMultisigDescriptor(#[from] multisig::Error) so multisig parse failures surface through the top-level error type. - Insert MultisigDescriptorPair detection in Format::try_new_from_str before the existing single-sig Descriptors path: miniscript can parse wsh(sortedmulti(...)) as a plain descriptor, so multisig must be checked first to get the richer typed result.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/formats.rs (1)
152-173: ⚡ Quick winConsider adding a dedicated
formats.rsintegration test forMultisigDescriptor, and fix the pre-existingtest_parse_all_formatsfilter.Two related test-quality gaps:
There is no unit test inside the
testsmodule offormats.rsthat verifiesFormat::try_new_from_strreturnsFormat::MultisigDescriptorfor a multisig descriptor string. The doctest inlib.rscovers the happy path, but an in-module test here would make failures easier to localise and would test the new format consistently with the othertest_parse_*tests.The pre-existing
test_parse_all_formatstest is a no-op.Path::ends_with(".json")in Rust compares whole path components, not string suffixes — sopath.ends_with(".json")is alwaysfalsefor a real file likesparrow-export.json, making!path.ends_with(".json")alwaystrue, and the loop body always hitscontinue. No file is ever read or asserted. The fix is to compare extensions:🛠 Proposed fix for `test_parse_all_formats` filter + new multisig test
fn test_parse_all_formats() { let files = std::fs::read_dir("test/data").unwrap(); for file in files { let file = file.unwrap(); let path = file.path(); - if !path.ends_with(".json") || path.ends_with(".txt") { + let ext = path.extension().and_then(|e| e.to_str()).unwrap_or(""); + if ext != "json" && ext != "txt" { continue; }And add a new test for the multisig path:
+ #[test] + fn test_parse_multisig_descriptor() { + let string = concat!( + "wsh(sortedmulti(2,", + "[6f53d49c/44h/1h/0h]tpubDDjsCRDQ9YzyaAq9rspCfq8RZFrWoBpYnLxK6sS2hS2yukqSczgcYiur8Scx4Hd5AZatxTuzMtJQJhchufv1FRFanLqUP7JHwusSSpfcEp2/<0;1>/*,", + "[e6807791/44h/1h/0h]tpubDDAfvogaaAxaFJ6c15ht7Tq6ZmiqFYfrSmZsHu7tHXBgnjMZSHAeHSwhvjARNA6Qybon4ksPksjRbPDVp7yXA1KjTjSd5x18KHqbppnXP1s/<0;1>/*,", + "[367c9cfa/44h/1h/0h]tpubDDtPnSgWYk8dDnaDwnof4ehcnjuL5VoUt1eW2MoAed1grPHuXPDnkX1fWMvXfcz3NqFxPbhqNZ3QBdYjLz2hABeM9Z2oqMR1Gt2HHYDoCgh/<0;1>/*", + "))" + ); + let format = Format::try_new_from_str(string).expect("multisig parse failed"); + assert!(matches!(format, Format::MultisigDescriptor(_))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formats.rs` around lines 152 - 173, Fix the broken file-filter and add a focused multisig test: in the tests module update test_parse_all_formats to check the file extension via path.extension().and_then(|e| e.to_str()) (e.g. skip unless ext == "json" or ext == "txt") instead of using Path::ends_with, and correct the boolean logic (use && to continue when ext is neither json nor txt). Also add a new unit test (e.g. test_multisig_descriptor) that calls Format::try_new_from_str with a representative multisig descriptor string and asserts the result is Ok(Format::MultisigDescriptor) so parsing of that variant is covered alongside the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/formats.rs`:
- Around line 152-173: Fix the broken file-filter and add a focused multisig
test: in the tests module update test_parse_all_formats to check the file
extension via path.extension().and_then(|e| e.to_str()) (e.g. skip unless ext ==
"json" or ext == "txt") instead of using Path::ends_with, and correct the
boolean logic (use && to continue when ext is neither json nor txt). Also add a
new unit test (e.g. test_multisig_descriptor) that calls
Format::try_new_from_str with a representative multisig descriptor string and
asserts the result is Ok(Format::MultisigDescriptor) so parsing of that variant
is covered alongside the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 230dddc5-0e64-422e-9c6c-1330d26e0db4
📒 Files selected for processing (3)
src/formats.rssrc/lib.rssrc/multisig.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/multisig.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds pubport::multisig a new module that handles parsing and validating wsh(multi(...)) and wsh(sortedmulti(...)) descriptor pairs for watch-only multisig imports.
What it does: takes care of parsing, validation, and returns a typed result (MultisigDescriptorPair). Wallet construction, syncing, and address history are still handled in Cove.
It supports either a single multipath descriptor (/<0;1>/*) or separate external/internal descriptors. It explicitly rejects private keys, bare multi, sh(multi), taproot multi_a, and miniscript policies like wsh(thresh(...)), with clear typed errors. For descriptor pairs, it also checks that the threshold, signer set, and function type match between external and internal.
This is wired into Format::try_new_from_str as Format::MultisigDescriptor, and runs before the single-sig path since miniscript can interpret wsh(sortedmulti(...)) in more than one way.
Summary by CodeRabbit
New Features
Documentation