Add SATSCARD Rust core for tap-card integration#698
Open
guptapratykshh wants to merge 1 commit intobitcoinppl:masterfrom
Open
Add SATSCARD Rust core for tap-card integration#698guptapratykshh wants to merge 1 commit intobitcoinppl:masterfrom
guptapratykshh wants to merge 1 commit 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:
📝 WalkthroughWalkthroughAdds SATSCARD support across the stack: parser tests, MultiFormat SatsCard variant and error, a UniFFI SatsCardReader module (status/unseal/dump/errors), router enum for SatsCard routes, and Android/iOS scan handling. No existing public APIs removed. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant SatsCardReader
participant Transport
participant Card
Client->>Router: parse/select SatsCard URL
Router->>SatsCardReader: create_sats_card_reader(transport, cmd)
SatsCardReader->>Transport: init / open connection
Transport->>Card: establish link
Card-->>Transport: card info
Transport-->>SatsCardReader: initialized
alt auth delay present
SatsCardReader->>Transport: wait_if_needed / poll
Transport->>Card: check auth delay
Card-->>Transport: delay status
end
Client->>SatsCardReader: run(cmd) or status()
alt status
SatsCardReader->>Card: read slot/status
Card-->>SatsCardReader: SatsCardStatus
else unseal
SatsCardReader->>SatsCardReader: validate CVC
SatsCardReader->>Card: unseal active slot
Card-->>SatsCardReader: SatsCardUnsealData
else dump
SatsCardReader->>SatsCardReader: validate optional CVC
SatsCardReader->>Card: dump slot data
Card-->>SatsCardReader: SatsCardSlotDump
end
SatsCardReader-->>Client: SatsCardResponse or SatsCardReaderError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 adds the Rust core scaffolding for SATSCARD integration: a
Confidence Score: 4/5Safe to merge after verifying the mut/non-mut lock guard in dump; all other changes are well-structured scaffolding One P1 finding: the immutable lock guard in dump is inconsistent with every other NFC operation in the codebase and will cause a compile error if SatsCard::dump requires &mut self. All remaining code — parsing, routing, scan handlers, and generated bindings — is clean. rust/src/tap_card/sats_card_reader.rs — specifically the dump method's lock guard mutability Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant SM as ScanManager
participant MF as MultiFormat
participant SC as SatsCardReader (FFI)
participant Card as rust_cktap::SatsCard
UI->>SM: scan NFC / QR
SM->>MF: try_from_string(url)
MF-->>SM: MultiFormat::SatsCard
SM->>UI: alert("SATSCARD Detected – coming soon")
note over SC,Card: Future flow (not yet wired to UI)
UI->>SC: create_sats_card_reader(transport, cmd)
SC->>Card: to_cktap() — identify card type
SC->>Card: wait_if_needed() — drain auth delay
UI->>SC: run()
alt Status
SC->>Card: reads fields via &self
else Unseal
SC->>Card: unseal(slot, cvc) — &mut self
else Dump
SC->>Card: dump(slot, cvc?) — &mut self?
end
SC-->>UI: SatsCardResponse
Reviews (2): Last reviewed commit: "Add SATSCARD Rust core for tap-card inte..." | Re-trigger Greptile |
![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: 4
🧹 Nitpick comments (1)
rust/src/multi_format.rs (1)
160-182: Minor:InvalidUrlis misleading for card-type crosspaths.When
tapsigner.com/startparses to aSatsCard(or vice-versa), the URL itself is well-formed — the failure is that the endpoint didn't match the card type. SurfacingTapCardParseError::InvalidUrl(string)will make error logs and UI text harder to interpret (e.g., users/debuggers may hunt for URL encoding problems).Consider introducing a dedicated variant (e.g.,
TapCardParseError::WrongCardType { expected, got }) incove-tap-card, or at minimum using a distinctMultiFormatErrorvariant for the crosspath case. Non-blocking.🤖 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 160 - 182, The current error uses TapCardParseError::InvalidUrl when the parsed card type doesn't match the endpoint (e.g., cove_tap_card::TapCard::SatsCard parsed from a tapsigner.com/start URL), which is misleading; update the handling in the parsing branch that returns MultiFormatError::InvalidTapSigner and MultiFormatError::InvalidSatsCard so they emit a clear mismatch error instead: either add a new TapCardParseError::WrongCardType { expected, got } in cove-tap-card and map the crosspath cases to that, or add a distinct MultiFormatError variant (e.g., MultiFormatError::CardTypeMismatch { expected, got, url }) and use it in the match arms where cove_tap_card::TapCard::SatsCard vs cove_tap_card::TapCard::TapSigner are rejected, replacing the current use of TapCardParseError::InvalidUrl(string.to_string()).
🤖 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/tap_card/sats_card_reader.rs`:
- Around line 95-123: SatsCardUnsealData (privkey, master_pk, chain_code) and
SatsCardSlotDump::privkey contain sensitive key bytes and must be zeroized
rather than left as plain Vec<u8>; update handling so these fields are not kept
in long‑lived state such as SatsCardRoute::UnsealSuccess, and after passing the
bytes over FFI explicitly call zeroize::Zeroize::zeroize() (or wrap in
zeroize::Zeroizing where possible) to scrub the buffers on the Rust side;
because uniffi::Record currently prevents directly deriving ZeroizeOnDrop for
those structs, remove/avoid cloning these fields, avoid storing them in
persistent enums or global state, and add doc comments on SatsCardUnsealData and
SatsCardSlotDump::privkey instructing Swift/Kotlin callers that the fields
contain raw private key material and must be handled/cleared securely.
- Around line 247-259: The dump method currently forwards the raw Option<String>
CVC to reader.dump, skipping local validation; before calling reader.dump in
async fn dump, if cvc.is_some() call the existing validate_cvc helper (the same
used by unseal) and return an appropriate error on invalid input so malformed
CVCs are rejected locally instead of consumed by the card. Update the dump
implementation to validate the provided CVC (using validate_cvc on the inner
string) and only pass it to reader.dump when validation succeeds.
- Around line 35-36: The NonNumericCvc variant on SatsCardReaderError currently
stores the raw CVC string (NonNumericCvc(String)) and exposes it via the
#[error("CVC must be numeric only, found {0}")] Display message; remove the
sensitive payload by changing the variant to NonNumericCvc without a String
payload and update its #[error(...)] message to something that does not include
the value (e.g., #[error("CVC must be numeric only")]); also update every place
constructing NonNumericCvc in the codebase (calls creating
SatsCardReaderError::NonNumericCvc(...)) to use the new no-arg variant so no raw
CVC is captured or logged.
- Around line 214-228: In wait_if_needed, move transport.set_message(message) to
before awaiting reader.wait so the UI is updated prior to blocking, and
consolidate reader.lock() calls by taking a single scoped mutex guard each
iteration to read auth_delay, build the message, call transport.set_message on
that delay, then release the guard before calling reader.wait(None). After the
loop, acquire the lock once to call set_auth_delay(None). Use scoped blocks to
ensure the lock is dropped before awaiting reader.wait and avoid multiple lock
acquisitions per iteration.
---
Nitpick comments:
In `@rust/src/multi_format.rs`:
- Around line 160-182: The current error uses TapCardParseError::InvalidUrl when
the parsed card type doesn't match the endpoint (e.g.,
cove_tap_card::TapCard::SatsCard parsed from a tapsigner.com/start URL), which
is misleading; update the handling in the parsing branch that returns
MultiFormatError::InvalidTapSigner and MultiFormatError::InvalidSatsCard so they
emit a clear mismatch error instead: either add a new
TapCardParseError::WrongCardType { expected, got } in cove-tap-card and map the
crosspath cases to that, or add a distinct MultiFormatError variant (e.g.,
MultiFormatError::CardTypeMismatch { expected, got, url }) and use it in the
match arms where cove_tap_card::TapCard::SatsCard vs
cove_tap_card::TapCard::TapSigner are rejected, replacing the current use of
TapCardParseError::InvalidUrl(string.to_string()).
🪄 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: 07daf4bf-c860-4508-b332-129afcdbf556
📒 Files selected for processing (5)
rust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rsrust/src/router.rsrust/src/tap_card.rsrust/src/tap_card/sats_card_reader.rs
guptapratykshh
force-pushed
the
feat/satscard-rust-core
branch
3 times, most recently
from
1a34da8 to
7eb4be5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
47-56000:⚠️ Potential issue | 🟡 MinorAuto-generated UniFFI bindings — review the Rust source instead.
This entire file is generated by UniFFI from the Rust side (
rust/src/tap_card/sats_card_reader.rs,rust/src/router.rs,rust/src/multi_format.rs). Hand-edits here will be overwritten on the next build, and static-analysis warnings (e.g., the detekt "empty default constructor" hits around lines 43095–43132) are generator artifacts rather than defects in this PR.A couple of things worth confirming before merge:
- Generated file is in sync with the Rust source. The checksum constants (e.g.,
uniffi_cove_checksum_func_create_sats_card_reader() != 46505at line 3636, and the five newsatscardreader*/satscardresponse*checksums) are only valid for the exact Rust signatures in this PR. If anyone tweaks the Rust API (param names, types, ordering) after this file was committed, the runtime will throw "UniFFI API checksum mismatch" on load. Please regenerate and re-commitcove.ktas the final step before merging, and keep an eye on this if the Rust side changes during review.- Check the file is actually committed as generator output, not hand-edited. A quick way: re-run the UniFFI Kotlin bindgen against the current Rust code and diff against the checked-in
cove.kt. Any diff indicates drift.MultiFormatdiscriminants. Hunk 24 assignsSatsCard = 8and shiftsSignedPsbtto9. SinceMultiFormatis serialized across the FFI boundary by integer tag, make sure no persisted/cachedMultiFormatvalues exist anywhere (Rust-side DB, Kotlin-side state) that were encoded with the old ordering — otherwise old values will be mis-decoded asSatsCard. Given this is only surfaced in transient scan flows it's likely safe, but worth a sanity check.#!/bin/bash # 1. Confirm cove.kt is generated (look for a generator marker / build script). rg -nP -C2 '(WARNING|Autogenerated|auto-generated|DO NOT EDIT|uniffi-bindgen)' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | head -n 40 # 2. Find where/how the Kotlin bindings are generated. fd -t f 'build.gradle|build.gradle.kts|settings.gradle|uniffi.toml' | xargs rg -nP -l 'uniffi|bindgen' 2>/dev/null rg -nP -C3 'uniffi-bindgen|generate.*kotlin|uniffi_bindgen' --type-add 'gradle:*.{gradle,gradle.kts,kts}' -tgradle -ttoml 2>/dev/null | head -n 60 # 3. Check MultiFormat discriminant ordering in the Rust source to confirm Kotlin matches. rg -nP -C2 '^\s*(pub\s+)?enum\s+MultiFormat\b' rust/ | head -n 30 ast-grep --pattern $'pub enum MultiFormat { $$$ }' # 4. Look for any persisted MultiFormat values that could be affected by the reorder. rg -nP -C3 '\bMultiFormat\b' --type=rust -g '!**/target/**' | head -n 80🤖 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_core/cove.kt` around lines 47 - 56000, The provided file is entirely auto-generated from Rust source via UniFFI and should not be manually edited. Ensure that this Kotlin file is regenerated each time the Rust FFI definitions are updated so the checksum constants remain in sync, preventing runtime errors. Confirm that the MultiFormat enum discriminant reordering (SatsCard changed to 8, SignedPsbt to 9) is consistent in Rust and no persisted data exists using the old ordering to avoid deserialization errors. Verify this file is committed as generated output by diffing against a freshly generated version from the current Rust codebase.
🤖 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/tap_card/sats_card_reader.rs`:
- Around line 56-66: SatsCardCmd currently derives Clone+Hash+Eq and stores raw
CVCs which causes CVC bytes to be included in SatsCardReader comparisons and
hashing; remove Hash and Eq from the SatsCardCmd enum (keep
Debug/Clone/uniffi::Enum as needed) and modify SatsCardReader’s Hash and
PartialEq/ Eq implementations to only use the reader’s unique id (the Nanoid
field referenced in SatsCardReader::hash / eq) so CVCs are never hashed/compared
or cloned across FFI; ensure any tests or uses that relied on SatsCardCmd impls
are updated to compare readers by id only.
- Line 158: The debug line debug!("sats_card_from_status: {:?}", card) leaks
sensitive CkTapCard fields (card_ident, pubkey, addr, auth_delay, birth data);
replace it with a redacted/log-safe message that only emits the card variant or
a minimal, non-identifying summary: inside sats_card_from_status (or wherever
CkTapCard is being logged) match on the CkTapCard enum/struct to log a fixed tag
like "card type: SatsCard" or "card type: Unknown" and, if you must include an
identifier, log only a hashed/fingerprint or truncated/redacted value (not full
pubkey/address), removing any direct printing of card_ident, pubkey, addr,
auth_delay, or birth data.
---
Outside diff comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 47-56000: The provided file is entirely auto-generated from Rust
source via UniFFI and should not be manually edited. Ensure that this Kotlin
file is regenerated each time the Rust FFI definitions are updated so the
checksum constants remain in sync, preventing runtime errors. Confirm that the
MultiFormat enum discriminant reordering (SatsCard changed to 8, SignedPsbt to
9) is consistent in Rust and no persisted data exists using the old ordering to
avoid deserialization errors. Verify this file is committed as generated output
by diffing against a freshly generated version from the current Rust codebase.
🪄 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: 1d71ccec-14e3-4073-bf92-0b86bc89e415
📒 Files selected for processing (6)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rsrust/src/router.rsrust/src/tap_card.rsrust/src/tap_card/sats_card_reader.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/tap_card.rs
- rust/crates/cove-tap-card/src/parse.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/router.rs
- rust/src/multi_format.rs
guptapratykshh
force-pushed
the
feat/satscard-rust-core
branch
from
7eb4be5 to
e794e57
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
rust/src/tap_card/sats_card_reader.rs (3)
56-66:⚠️ Potential issue | 🟠 Major
SatsCardReaderHash/Eq still mix in the raw CVC.Previously flagged and not addressed.
SatsCardCmd::Unseal { cvc }/Dump { cvc }still deriveHash + Eqand are incorporated intoSatsCardReader::hash(line 346) andPartialEq(line 353). CVC bytes therefore participate in every reader hash/compare and areClone'd across FFI. Hash/eqSatsCardReaderbyidalone (the Nanoid is already unique), and consider droppingHash/EqfromSatsCardCmd.🔒 Proposed fix
-#[derive(Debug, Clone, Hash, PartialEq, Eq, uniffi::Enum)] +#[derive(Debug, Clone, uniffi::Enum)] pub enum SatsCardCmd {impl std::hash::Hash for SatsCardReader { fn hash<H: Hasher>(&self, state: &mut H) { self.id.hash(state); - self.cmd.read().as_ref().hash(state); } } impl Eq for SatsCardReader {} impl PartialEq for SatsCardReader { fn eq(&self, other: &Self) -> bool { - self.id == other.id && self.cmd.read().as_ref() == other.cmd.read().as_ref() + self.id == other.id } }As per coding guidelines: "Check for potential security issues, make sure Bitcoin wallets are handled securely."
Also applies to: 343-355
158-158:⚠️ Potential issue | 🟠 MajorDebug-logging full
CkTapCardstill leaks card identifiers.This line was flagged previously and does not appear to be addressed —
debug!("sats_card_from_status: {:?}", card)still dumps the entireCkTapCardDebug representation (card_ident,pubkey,addr,auth_delay, birth data) into tracing sinks/crash reports before user consent. Log only the variant tag or a redacted summary.As per coding guidelines: "Check for potential security issues, make sure Bitcoin wallets are handled securely."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` at line 158, The debug call in sats_card_from_status currently prints the full CkTapCard (debug!("{:?}", card)), leaking sensitive fields; change it to log only a non-sensitive variant tag or a redacted summary instead: update the debug line in sats_card_from_status to match on CkTapCard (or call a redaction helper) and emit only the enum variant name or a constructed minimal summary that omits/obfuscates card_ident, pubkey, addr, auth_delay, and birth data; add a redacted_display/redact method on CkTapCard if needed and use that here so tracing never receives raw sensitive fields.
105-137:⚠️ Potential issue | 🟡 MinorSensitive key material still not zeroized.
Doc comments were added (good), but
SatsCardUnsealData(privkey,master_pk,chain_code) andSatsCardSlotDump::privkeyremain plainVec<u8>withClone+Hash+Eqderived. DerivingHashon raw private key bytes is particularly risky — any incidental hashing (e.g. put into aHashSet/HashMapfor dedup) silently mixes secret material into a non-cryptographic hasher.At minimum, drop
Hash/Eqfrom these secret-bearing records so they can't be accidentally hashed, and explicitlyzeroize()the buffers on the Rust side once the FFI call returns.As per coding guidelines: "Check for potential security issues, make sure Bitcoin wallets are handled securely."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` around lines 105 - 137, The SatsCardUnsealData record (fields privkey, master_pk, chain_code) and SatsCardSlotDump::privkey expose secret material and are still deriving Hash/Eq and using plain Vec<u8>; remove the Hash and Eq derives from SatsCardUnsealData and from any record/type that contains secret bytes to prevent accidental hashing/comparison, and ensure secret buffers are zeroized after use — e.g. wrap secret Vec<u8> fields in zeroize::Zeroizing (or call zeroize() on the Vecs) on the Rust side when the FFI call returns, and/or implement Drop to zeroize fields so privkey/master_pk/chain_code are cleared from memory immediately after use; update uniffi record definitions (SatsCardUnsealData and SatsCardSlotDump) accordingly so FFI still exposes required data but without Hash/Eq derives and with explicit zeroization.
🤖 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_core/cove.kt`:
- Around line 43461-43465: SatsCardRoute.UnsealSuccess currently embeds
SatsCardUnsealData (raw private key/master key/chain code) which stores
sensitive key material in navigation state; change the route to carry a
non-sensitive reference (e.g., unsealResultId, token or slot id) instead of
SatsCardUnsealData, implement a short-lived in-memory handoff/cache keyed by
that id (accessed by the sweep UI) to deliver the SatsCardUnsealData, and ensure
the handoff entry is zeroed/cleared immediately after the sweep UI consumes it;
update all references to SatsCardRoute.UnsealSuccess and any callers that
construct it to use the id/token and the new in-memory accessor API rather than
passing SatsCardUnsealData directly.
In `@ios/Cove/ScanManager.swift`:
- Around line 32-37: handleFileOpen currently switches on MultiFormat but does
not handle the .satsCard case while handleMultiFormat does (see
handleMultiFormat's .satsCard branch), leaving a file-based SATSCARD import
unhandled; either add a .satsCard case to handleFileOpen that mirrors the
behavior in handleMultiFormat (logging the detection and setting app.alertState
with the same message/slot handling) or add a clear comment/docstring in
handleFileOpen explaining that .satsCard is intentionally unsupported for file
imports so the missing case is explicit.
In `@rust/src/tap_card/sats_card_reader.rs`:
- Around line 38-45: SatsCardReaderError defines variants WrongSlot, NoAddress,
and NoRemainingSlots that are never constructed; update the caller-path (e.g.,
the dump function) to perform local validation and emit these errors instead of
calling the card blind: check that the provided slot equals the reader’s
active_slot and return SatsCardReaderError::WrongSlot { slot, active:
active_slot } if not; verify an active address exists and return
SatsCardReaderError::NoAddress when missing; and check remaining_slots (or
equivalent local capacity state) and return
SatsCardReaderError::NoRemainingSlots before attempting the card operation —
alternatively, if you prefer a minimal FFI surface, remove those three variants
from SatsCardReaderError and any references to them.
---
Duplicate comments:
In `@rust/src/tap_card/sats_card_reader.rs`:
- Line 158: The debug call in sats_card_from_status currently prints the full
CkTapCard (debug!("{:?}", card)), leaking sensitive fields; change it to log
only a non-sensitive variant tag or a redacted summary instead: update the debug
line in sats_card_from_status to match on CkTapCard (or call a redaction helper)
and emit only the enum variant name or a constructed minimal summary that
omits/obfuscates card_ident, pubkey, addr, auth_delay, and birth data; add a
redacted_display/redact method on CkTapCard if needed and use that here so
tracing never receives raw sensitive fields.
- Around line 105-137: The SatsCardUnsealData record (fields privkey, master_pk,
chain_code) and SatsCardSlotDump::privkey expose secret material and are still
deriving Hash/Eq and using plain Vec<u8>; remove the Hash and Eq derives from
SatsCardUnsealData and from any record/type that contains secret bytes to
prevent accidental hashing/comparison, and ensure secret buffers are zeroized
after use — e.g. wrap secret Vec<u8> fields in zeroize::Zeroizing (or call
zeroize() on the Vecs) on the Rust side when the FFI call returns, and/or
implement Drop to zeroize fields so privkey/master_pk/chain_code are cleared
from memory immediately after use; update uniffi record definitions
(SatsCardUnsealData and SatsCardSlotDump) accordingly so FFI still exposes
required data but without Hash/Eq derives and with explicit zeroization.
🪄 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: 643286f4-47a9-4d8c-ab96-c1355e2faab4
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/ScanManager.swiftrust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rsrust/src/router.rsrust/src/tap_card.rsrust/src/tap_card/sats_card_reader.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/tap_card.rs
- rust/crates/cove-tap-card/src/parse.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/multi_format.rs
- rust/src/router.rs
praveenperera
requested changes
Apr 23, 2026
Contributor
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
ios isnt compiling
guptapratykshh
force-pushed
the
feat/satscard-rust-core
branch
from
e794e57 to
9853301
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rust/src/tap_card/sats_card_reader.rs (2)
190-209:run()is single-shot by design — worth documenting.
self.cmd.write().take()consumes the queued command, so a second call torun()always returnsNoCommand. That's a reasonable contract (readers are usually one-shot per NFC tap), but it isn't documented on the UniFFI surface and callers on Swift/Kotlin won't see it. Consider a short doc comment onrunclarifying that the reader is single-use, or reject the second call with a more descriptive variant than the genericNoCommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` around lines 190 - 209, The run() method currently consumes the queued command via self.cmd.write().take() so subsequent calls return the generic SatsCardReaderError::NoCommand; document that behavior on the UniFFI surface and/or provide a clearer error variant: add a doc comment to the run method explaining the reader is single-use (one NFC tap per instance), and optionally introduce a new error variant (e.g. SatsCardReaderError::AlreadyConsumed or SatsCardReaderError::ReaderSingleUse) and return that instead of NoCommand when self.cmd.write().take() is None; update references to run, self.cmd.write().take(), and SatsCardReaderError to implement the chosen approach.
248-251: Consider adding a comment explaining whyset_auth_delay(None)is called after the loop guaranteesauth_delayis alreadyNone.The loop exits only when
auth_delayisNone(line 235), making the finalset_auth_delay(None)call technically redundant. This defensive pattern mirrorstap_signer_readerand is safe, but if intentional for defensive coding, a one-line comment would clarify the intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` around lines 248 - 251, Add a one-line comment above the defensive call that clarifies why set_auth_delay(None) is invoked after the loop: even though the loop exits when auth_delay is observed as None, we still explicitly call reader.set_auth_delay(None) after acquiring the lock to ensure the reader's internal state is consistent (mirrors the defensive pattern used in tap_signer_reader). Place this comment immediately before the block that acquires the lock (the reader.lock().await block containing set_auth_delay(None)) and reference set_auth_delay and tap_signer_reader so future maintainers understand the intentional redundancy.android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
43077-43143: Generated-code nits from detekt are not actionable here; address upstream if desired.detekt flags empty default constructors on
NoCommand,NonNumericCvc,NoAddress,NoRemainingSlots(lines 43095–43096, 43109–43110, 43125–43126, 43131–43132). This is UniFFI-emitted boilerplate for unit error variants and will reappear on every regeneration. If these warnings are noisy in CI, consider suppressing theEmptyDefaultConstructorrule for UniFFI-generated files (cove.kt) in the detekt config rather than editing the generated file.🤖 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_core/cove.kt` around lines 43077 - 43143, The detekt warnings for empty default constructors in the UniFFI-generated error variants (NoCommand, NonNumericCvc, NoAddress, NoRemainingSlots in SatsCardReaderException) are noise; do not edit cove.kt—instead update the detekt configuration to suppress the EmptyDefaultConstructor rule for UniFFI-generated files (e.g., add a path/include/exclude filter or a rule-set override that excludes files named cove.kt or the generated sources directory), so those classes remain untouched and CI no longer reports these warnings.
🤖 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/router.rs`:
- Around line 160-161: SatsCardRoute::UnsealSuccess currently stores
SatsCardUnsealData (privkey/master_pk/chain_code) which violates the type's
guidance and leaks sensitive key bytes into Clone/Hash/Eq and stableHash; change
the route to carry only non-sensitive data needed by the sweep UI (e.g., derived
address and metadata) or a short-lived opaque token/ID, and move the full
SatsCardUnsealData into a side-channel store (map) keyed by that token which the
sweep flow consumes and immediately drops/zeroizes; alternatively wrap any
remaining in-memory secret in an Arc<ZeroizeOnDrop> that is not Hash/Eq/Clone
and ensure you remove Hash/Eq/Clone derivations from the route variant so raw
key bytes never participate in stableHash or get cloned through the router.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 43077-43143: The detekt warnings for empty default constructors in
the UniFFI-generated error variants (NoCommand, NonNumericCvc, NoAddress,
NoRemainingSlots in SatsCardReaderException) are noise; do not edit
cove.kt—instead update the detekt configuration to suppress the
EmptyDefaultConstructor rule for UniFFI-generated files (e.g., add a
path/include/exclude filter or a rule-set override that excludes files named
cove.kt or the generated sources directory), so those classes remain untouched
and CI no longer reports these warnings.
In `@rust/src/tap_card/sats_card_reader.rs`:
- Around line 190-209: The run() method currently consumes the queued command
via self.cmd.write().take() so subsequent calls return the generic
SatsCardReaderError::NoCommand; document that behavior on the UniFFI surface
and/or provide a clearer error variant: add a doc comment to the run method
explaining the reader is single-use (one NFC tap per instance), and optionally
introduce a new error variant (e.g. SatsCardReaderError::AlreadyConsumed or
SatsCardReaderError::ReaderSingleUse) and return that instead of NoCommand when
self.cmd.write().take() is None; update references to run,
self.cmd.write().take(), and SatsCardReaderError to implement the chosen
approach.
- Around line 248-251: Add a one-line comment above the defensive call that
clarifies why set_auth_delay(None) is invoked after the loop: even though the
loop exits when auth_delay is observed as None, we still explicitly call
reader.set_auth_delay(None) after acquiring the lock to ensure the reader's
internal state is consistent (mirrors the defensive pattern used in
tap_signer_reader). Place this comment immediately before the block that
acquires the lock (the reader.lock().await block containing
set_auth_delay(None)) and reference set_auth_delay and tap_signer_reader so
future maintainers understand the intentional redundancy.
🪄 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: 5190fcc5-a602-4a16-93cc-039e32d7b330
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/ScanManager.swiftrust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rsrust/src/router.rsrust/src/tap_card.rsrust/src/tap_card/sats_card_reader.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/tap_card.rs
- rust/crates/cove-tap-card/src/parse.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/org/bitcoinppl/cove/ScanManager.kt
- ios/Cove/ScanManager.swift
- rust/src/multi_format.rs
guptapratykshh
force-pushed
the
feat/satscard-rust-core
branch
2 times, most recently
from
dedd7cc to
3e32207
Compare
guptapratykshh
force-pushed
the
feat/satscard-rust-core
branch
from
3e32207 to
fc4f6d3
Compare
Add SatsCardReader with status, unseal, and dump commands, SatsCardRoute enum for mobile navigation, MultiFormat detection for getsatscard.com URLs, and seven new parse tests. Replace unreachable! panics with proper error returns for crafted card-type mismatch payloads.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/multi_format.rs (1)
155-187:⚠️ Potential issue | 🟡 MinorUse exact card URL prefixes instead of substring matching.
The new
getsatscard.comdispatch currently matches any string containing that substring, so lookalike hosts/paths can be classified asInvalidSatsCardbefore the exact parser/fallback path runs.Suggested tightening
+ let card_url = string.trim_start_matches(|p: char| !p.is_ascii_alphabetic()); + let card_url = card_url + .strip_prefix("https://") + .or_else(|| card_url.strip_prefix("http://")) + .unwrap_or(card_url); + - if string.contains("tapsigner.com/start") { + if card_url.starts_with("tapsigner.com/start#") { let tap_card = cove_tap_card::TapCard::parse(string) .map_err(|e| MultiFormatError::InvalidTapSigner(e.into()))?; match tap_card { cove_tap_card::TapCard::TapSigner(card) => { @@ } } - if string.contains("getsatscard.com/start") { + if card_url.starts_with("getsatscard.com/start#") { let tap_card = cove_tap_card::TapCard::parse(string) .map_err(|e| MultiFormatError::InvalidSatsCard(e.into()))?;🤖 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 155 - 187, The substring checks using string.contains("tapsigner.com/start") and string.contains("getsatscard.com/start") are too permissive; update the dispatch in multi_format.rs to match exact card URL prefixes or validate the host/path via URL parsing instead of contains. Specifically, replace the contains(...) guards with either string.starts_with("https://tapsigner.com/start") / string.starts_with("https://getsatscard.com/start") or parse the input with url::Url and verify url.host_str() == Some("tapsigner.com") && url.path().starts_with("/start") (and likewise for getsatscard.com) before calling cove_tap_card::TapCard::parse; keep the existing MultiFormatError::InvalidTapSigner / InvalidSatsCard branches and returns (Self::from(card) and Self::SatsCard(card)) unchanged.
🧹 Nitpick comments (2)
rust/src/tap_card/sats_card_reader.rs (2)
41-42: UnusedUnknownerror variant.
SatsCardReaderError::Unknown(String)is never constructed anywhere in this module — all error paths funnel throughSatsCardError(TransportError),UnknownCardType, CVC validation,SlotOutOfRange, orNoCommand. Either wire it into an actual fallback path or drop it to keep the FFI surface minimal (same rationale as the prior cleanup ofWrongSlot/NoAddress/NoRemainingSlots).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` around lines 41 - 42, The SatsCardReaderError::Unknown(String) variant is unused; remove the Unknown(String) enum variant from SatsCardReaderError (and any matching Display/From impls) to minimize the FFI surface, or alternatively wire it as a true fallback by constructing Unknown(error.to_string()) in the catch-all path inside functions that currently map all errors to SatsCardError(TransportError) (e.g., where errors are converted into SatsCardError or UnknownCardType); prefer removing the variant if no real fallback is needed and update pattern matches and conversions that reference Unknown accordingly.
23-24: Nit:SatsCardErroris a misleading name for a transport-error passthrough.The variant wraps
TransportError(NFC / CkTap errors), not a SatsCard-specific condition. A name likeTransportorTapreads more accurately at call sites (Err(SatsCardReaderError::Transport(e))) and matches howis_auth_error/is_rate_limiteddestructure it. Purely a readability/FFI-surface nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card/sats_card_reader.rs` around lines 23 - 24, Rename the misleading enum variant SatsCardError to a clearer name like Transport (or Tap) since it merely wraps TransportError; update the enum variant in the declaration (replace SatsCardError(#[from] TransportError) with Transport(#[from] TransportError)), then update all call sites and pattern matches that reference SatsCardError (e.g. in is_auth_error and is_rate_limited and any Err(SatsCardReaderError::SatsCardError(...)) occurrences) to use the new variant name so compilation and pattern destructuring remain correct.
🤖 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/tap_card/sats_card_reader.rs`:
- Around line 228-267: The dump() method is passing slot as usize to
reader.dump(slot as usize, cvc) while unseal() passes a u8 directly; both
SatsCard::dump and SatsCard::unseal expect a u8, so remove the unnecessary cast
and call reader.dump(slot, cvc) (update the reader.dump invocation in dump()) to
keep parameter types consistent with unseal() and the SatsCard API.
---
Outside diff comments:
In `@rust/src/multi_format.rs`:
- Around line 155-187: The substring checks using
string.contains("tapsigner.com/start") and
string.contains("getsatscard.com/start") are too permissive; update the dispatch
in multi_format.rs to match exact card URL prefixes or validate the host/path
via URL parsing instead of contains. Specifically, replace the contains(...)
guards with either string.starts_with("https://tapsigner.com/start") /
string.starts_with("https://getsatscard.com/start") or parse the input with
url::Url and verify url.host_str() == Some("tapsigner.com") &&
url.path().starts_with("/start") (and likewise for getsatscard.com) before
calling cove_tap_card::TapCard::parse; keep the existing
MultiFormatError::InvalidTapSigner / InvalidSatsCard branches and returns
(Self::from(card) and Self::SatsCard(card)) unchanged.
---
Nitpick comments:
In `@rust/src/tap_card/sats_card_reader.rs`:
- Around line 41-42: The SatsCardReaderError::Unknown(String) variant is unused;
remove the Unknown(String) enum variant from SatsCardReaderError (and any
matching Display/From impls) to minimize the FFI surface, or alternatively wire
it as a true fallback by constructing Unknown(error.to_string()) in the
catch-all path inside functions that currently map all errors to
SatsCardError(TransportError) (e.g., where errors are converted into
SatsCardError or UnknownCardType); prefer removing the variant if no real
fallback is needed and update pattern matches and conversions that reference
Unknown accordingly.
- Around line 23-24: Rename the misleading enum variant SatsCardError to a
clearer name like Transport (or Tap) since it merely wraps TransportError;
update the enum variant in the declaration (replace SatsCardError(#[from]
TransportError) with Transport(#[from] TransportError)), then update all call
sites and pattern matches that reference SatsCardError (e.g. in is_auth_error
and is_rate_limited and any Err(SatsCardReaderError::SatsCardError(...))
occurrences) to use the new variant name so compilation and pattern
destructuring remain correct.
🪄 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: 2da2368a-802b-46ae-ad52-b5051e866548
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/ScanManager.swiftrust/crates/cove-tap-card/src/parse.rsrust/src/multi_format.rsrust/src/router.rsrust/src/tap_card.rsrust/src/tap_card/sats_card_reader.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/tap_card.rs
- rust/crates/cove-tap-card/src/parse.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/router.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
2 participants
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.
Add SatsCardReader with status, unseal, and dump commands, SatsCardRoute enum for mobile navigation, MultiFormat detection for getsatscard.com URLs, and seven new parse tests. Replace unreachable! panics with proper error returns for crafted card-type mismatch payloads.
Summary
Testing
Platform Coverage
Checklist
Summary by CodeRabbit
New Features
Tests