Skip to content

Support SLIP-132 Imports#793

Merged
praveenperera merged 14 commits into
masterfrom
pubport-slip132-imports
Jun 25, 2026
Merged

Support SLIP-132 Imports#793
praveenperera merged 14 commits into
masterfrom
pubport-slip132-imports

Conversation

@praveenperera

Copy link
Copy Markdown
Contributor

Closes: #725

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fa0046f0-107d-4022-986c-b0de6e2a1971

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pubport-slip132-imports

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

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds SLIP-132 support (ypub/zpub prefixed extended public keys) by upgrading pubport from 0.5.0 to 0.6.0 and updating the import logic throughout the wallet stack.

  • wallet.rs introduces preferred_json_descriptors (bip84 > bip49 > bip44 priority) and should_start_json_discovery (only triggers background discovery when a bip84 primary exists alongside other address types), replacing the previous hard requirement for bip84.
  • multi_format.rs adds a UTF-8 fallback path in try_from_data so binary-delivered ypub/zpub strings are parsed the same as their QR/NFC string equivalents.
  • discovery_scanner.rs threads current_address_type into Wallets::try_from_json to avoid re-creating the already-selected wallet type during the background discovery scan.

Confidence Score: 4/5

Safe to merge for the primary SLIP-132 use cases (zpub/ypub single-key imports and full JSON exports with bip84). The bip44+bip49-only JSON export path silently drops the legacy wallet, which may surprise some users but does not corrupt data.

The core import paths are well-exercised by new tests and the logic is straightforward. The one gap is JSON exports that carry bip44 + bip49 but omit bip84 — in that scenario the bip44 descriptor is discarded without any diagnostic, which is an unverified code path with no test. The error-mapping mismatch in xpub.rs and the dead uniffi variant are cosmetic concerns that don't affect runtime correctness.

rust/src/wallet.rs — specifically the interaction between preferred_json_descriptors and should_start_json_discovery for two-type JSON exports (bip44 + bip49, no bip84).

Important Files Changed

Filename Overview
rust/src/wallet.rs Introduces preferred_json_descriptors (bip84>bip49>bip44 fallback) and should_start_json_discovery (NativeSegwit-only); well-tested but bip44 is silently dropped from JSON exports containing only bip44+bip49.
rust/src/discovery_scanner.rs Adds current_address_type guard to try_from_json to avoid re-creating the already-selected wallet during discovery; logic is correct and defensive.
rust/src/xpub.rs Updates pubport 0.6.0 error mapping; InvalidDescriptorInJson variant is now unreachable (kept for uniffi ABI compatibility), and UnsupportedFormat is mapped to InvalidXpub which is a semantic mismatch.
rust/src/multi_format.rs Adds UTF-8 fallback in try_from_data so binary-encoded zpub/ypub strings work; new tests cover all SLIP-132 key types across all entry points.
rust/src/qr_scanner.rs Test refactoring: extracts helper functions and adds new BBQR/plain zpub test cases; no logic changes.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant MultiFormat
    participant pubport
    participant Wallet
    participant DiscoveryScanner

    Caller->>MultiFormat: try_from_data(bytes) / try_from_string(str)
    MultiFormat->>pubport: Format::try_new_from_str(str)
    pubport-->>MultiFormat: "Format::Json(json) | Format::Descriptor(...)"
    MultiFormat-->>Caller: HardwareExport(format)

    Caller->>Wallet: try_new_persisted_from_pubport(format)
    alt Format::Json
        Wallet->>Wallet: preferred_json_descriptors(json)
        Note over Wallet: bip84 > bip49 > bip44
        Wallet->>Wallet: should_start_json_discovery(json, address_type)
        alt NativeSegwit + alternatives exist
            Wallet->>Wallet: "metadata.discovery_state = StartedJson"
        end
    end
    Wallet-->>Caller: persisted wallet

    Note over DiscoveryScanner: Later, background scan
    DiscoveryScanner->>DiscoveryScanner: Wallets::try_from_json(json, network, current_address_type)
    Note over DiscoveryScanner: Skips bip49/bip44 if already selected
    DiscoveryScanner->>DiscoveryScanner: Scan bip49 + bip44 wallets for activity
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant MultiFormat
    participant pubport
    participant Wallet
    participant DiscoveryScanner

    Caller->>MultiFormat: try_from_data(bytes) / try_from_string(str)
    MultiFormat->>pubport: Format::try_new_from_str(str)
    pubport-->>MultiFormat: "Format::Json(json) | Format::Descriptor(...)"
    MultiFormat-->>Caller: HardwareExport(format)

    Caller->>Wallet: try_new_persisted_from_pubport(format)
    alt Format::Json
        Wallet->>Wallet: preferred_json_descriptors(json)
        Note over Wallet: bip84 > bip49 > bip44
        Wallet->>Wallet: should_start_json_discovery(json, address_type)
        alt NativeSegwit + alternatives exist
            Wallet->>Wallet: "metadata.discovery_state = StartedJson"
        end
    end
    Wallet-->>Caller: persisted wallet

    Note over DiscoveryScanner: Later, background scan
    DiscoveryScanner->>DiscoveryScanner: Wallets::try_from_json(json, network, current_address_type)
    Note over DiscoveryScanner: Skips bip49/bip44 if already selected
    DiscoveryScanner->>DiscoveryScanner: Scan bip49 + bip44 wallets for activity
Loading

Comments Outside Diff (1)

  1. rust/src/xpub.rs, line 27 (link)

    P2 InvalidDescriptorInJson is now a dead variant

    With the removal of the Error::InvalidDescriptorInJson => Self::InvalidDescriptorInJson arm in the From<pubport::Error> impl, there is no remaining code path that constructs XpubError::InvalidDescriptorInJson. The variant survives only because removing it from a uniffi::Error enum would be a breaking ABI change for Swift/Kotlin consumers. A #[doc(hidden)] or a comment marking it as deprecated-but-retained-for-ABI-compat would make the intent clear to future maintainers.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Update Pubport for SLIP-132" | Re-trigger Greptile

Comment thread rust/src/wallet.rs
Comment thread rust/src/xpub.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d049b22a86

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rust/src/wallet.rs Outdated
@praveenperera praveenperera force-pushed the pubport-slip132-imports branch from a72e76b to 8b79492 Compare June 24, 2026 18:27
@praveenperera

Copy link
Copy Markdown
Contributor Author

Review loop completed locally.

Status:

  • 2 fresh fix passes completed
  • Codex re-review found no actionable correctness issues
  • CodeRabbit final gate completed with 0 findings
  • just fmt passed
  • just clippy passed
  • cargo test descriptor_address_type --lib passed
  • git diff --check passed

Fixes are currently in the local worktree and have not been pushed in this loop. Modified files:

  • rust/src/wallet.rs
  • rust/src/discovery_scanner.rs
  • rust/src/multi_format.rs

@praveenperera praveenperera added the review-complete Review complete, ready to merge label Jun 24, 2026
@praveenperera praveenperera requested a review from Copilot June 25, 2026 01:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Rust import/parsing pipeline to accept SLIP-132 extended public keys (e.g., zpub/ypub) by upgrading pubport, improving how JSON exports choose a primary descriptor set, and ensuring QR/NFC byte payloads can be interpreted as text hardware exports.

Changes:

  • Bump pubport to 0.6.0 and update error mapping to handle new error variants.
  • Prefer BIP84→BIP49→BIP44 descriptor selection for JSON imports, propagate address_type, and adjust JSON discovery scanning to skip the primary type.
  • Extend MultiFormat / QR scanner coverage so zpub imports work via plain text, BBQR text, and NFC data; plus a small NFC transaction parse safety fix.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rust/src/xpub.rs Updates pubport error mappings; adds a regression test for unsupported-format mapping.
rust/src/wallet.rs Selects preferred descriptor set from JSON exports, sets address_type, and preserves metadata correctly on cold-upgrade.
rust/src/transaction/ffi.rs Avoids a potential panic by bounds-checking the 64-byte prefix strip.
rust/src/qr_scanner.rs Refactors tests and adds coverage for scanning xpub/zpub as HardwareExport (plain + BBQR).
rust/src/multi_format.rs Allows UTF-8 byte payloads to be parsed via the string parser; adds tests for xpub/zpub in bytes/NFC.
rust/src/keys.rs Updates miniscript-version comments to match current dependency majors.
rust/src/discovery_scanner.rs Threads address_type into JSON discovery so alternates exclude the primary type; removes expect in descriptor parsing.
rust/Cargo.toml Bumps pubport dependency to 0.6.0.
rust/Cargo.lock Locks pubport at 0.6.0 with updated checksum.
ios/CoveCore/Sources/CoveCore/generated/cove.swift Regenerated bindings doc comment for InvalidDescriptorInJson.
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt Regenerated bindings doc comment for InvalidDescriptorInJson.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rust/src/xpub.rs
Comment thread rust/src/xpub.rs
@praveenperera praveenperera force-pushed the pubport-slip132-imports branch from 3ca50da to 18596f1 Compare June 25, 2026 15:31
praveenperera and others added 13 commits June 25, 2026 15:02
Accept Pubport JSON exports that only include BIP49 or BIP44 descriptors and set the imported wallet address type from the descriptor selected.

Skip discovery for the already-imported descriptor and keep Pubport error conversion tolerant of newer variants.
Parse UTF-8 byte payloads through the normal hardware export path so data-only NFC and file-like imports can reach Pubport. Only start JSON discovery when a native segwit import includes alternate descriptor sets to scan.
Bump Cove to pubport 0.6.0 so bare SLIP-132 exports are parsed by the released crate.

Add coverage for zpub imports through plain strings, UTF-8 data, NFC data, and BBQr QR scanning, plus ypub/zpub wallet descriptor selection.
Return descriptor conversion failures from discovery instead of panicking, preserve pubport diagnostics in logs, and cover JSON descriptor preference and alternate discovery behavior.
Avoid slicing short byte buffers before transaction parsing so raw non-UTF-8 import data can fall through to the normal unrecognized-format path.
Carry pending import metadata when upgrading an existing watch-only wallet so alternate-address discovery still starts after the upgrade.

Keep completed hardware export scan tests asserting success haptics.
Derive wallet metadata from parsed descriptors and validate supported
address formats so discovery state stays aligned with actual paths.
@praveenperera praveenperera force-pushed the pubport-slip132-imports branch from 18596f1 to afc9a02 Compare June 25, 2026 20:02
Drops `InvalidDescriptorInJson` from `XpubError` in Rust and updates generated Android/Kotlin and iOS/Swift UniFFI bindings accordingly. Error enum tag mappings were reindexed so serialization/deserialization stays consistent with the new variant set.
@praveenperera praveenperera merged commit 46857f6 into master Jun 25, 2026
9 checks passed
@praveenperera praveenperera deleted the pubport-slip132-imports branch June 25, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-complete Review complete, ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants