Add Key Teleport receive flow (#654)#687
Open
pradhyum6144 wants to merge 9 commits intobitcoinppl:masterfrom
Open
Add Key Teleport receive flow (#654)#687pradhyum6144 wants to merge 9 commits intobitcoinppl:masterfrom
pradhyum6144 wants to merge 9 commits intobitcoinppl:masterfrom
Conversation
…xprv) ECDH session key, AES-256-CTR double-encryption, PBKDF2 stretch, BBQr R/S packet encoding, ReceiverSession and SenderSession. 25 tests, all passing.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a complete Key Teleport receive feature: new Rust crate (cove-keyteleport) with protocol/crypto, UniFFI bindings exposing a receiver session, Android Compose UI and navigation for QR display, sender scanning, password entry, payload review/import, and platform routing updates (Android + iOS placeholders). Changes
Sequence DiagramsequenceDiagram
participant User
participant ReceiverPhone as Phone (Receiver)
participant SenderApp as Sender
User->>ReceiverPhone: Open Key Teleport receive
ReceiverPhone->>ReceiverPhone: Create ReceiverSession (privkey + numeric code)
ReceiverPhone->>ReceiverPhone: Generate receiver BBQr + display QR + show code
User->>SenderApp: Share receiver BBQr + numeric code (out-of-band)
SenderApp->>SenderApp: Derive session key + generate teleport password
SenderApp->>SenderApp: Encrypt payload -> SenderPacket -> BBQr
SenderApp->>User: Send Sender BBQr
User->>ReceiverPhone: Scan Sender BBQr
ReceiverPhone->>ReceiverPhone: Parse SenderPacket
User->>ReceiverPhone: Enter teleport password
ReceiverPhone->>ReceiverPhone: ECDH + PBKDF2 + AES-CTR decrypt, verify checksums
ReceiverPhone->>ReceiverPhone: Produce Payload (Mnemonic/XPRV) -> show ReviewImport
User->>ReceiverPhone: Confirm import
ReceiverPhone->>ReceiverPhone: Import wallet / update alerts/navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 implements the Key Teleport receive flow, adding Rust crypto primitives ( Confidence Score: 5/5Safe to merge — all previously flagged P0/P1 issues are resolved; remaining findings are P2 style suggestions. The three prior blocking concerns (modulo bias in numeric code, XPRV silent watch-only import, missing resetRoute after XPRV import) are all resolved. The crypto primitives (ECDH, AES-256-CTR, PBKDF2, rejection sampling) are correct and well-tested. The two remaining findings are P2: the main-thread import call doesn't render its loading indicator, and the XPRV to_bytes has a silent fallback that could corrupt data (theoretical only, since XPRV import is disabled in the UI). rust/crates/cove-keyteleport/src/payload.rs (XPRV unwrap_or_else fallback) and KeyTeleportReceiveImportScreen.kt (synchronous main-thread import). Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Receiver (Android)
participant S as Sender
participant Rust as Rust (cove-keyteleport)
R->>Rust: KeyTeleportReceiverSession::new()
Rust-->>R: session (ephemeral keypair + 8-digit code)
R->>Rust: session.receiverPacketBbqr()
Rust-->>R: B$2R... (encrypted pubkey BBQr)
R->>S: Show QR (B$2R) + numeric code (out-of-band)
S->>Rust: SenderSession::new(r_packet, code)
Note over Rust: Decrypt receiver pubkey, ECDH session key,<br/>generate teleport_password
Rust-->>S: sender_session
S->>Rust: sender_session.encrypt(payload)
Rust-->>S: S packet (B$2S...)
S->>R: Show QR (B$2S) + teleport_password (out-of-band)
R->>Rust: session.decode(senderPacketBbqr, teleport_password)
Note over Rust: ECDH → session_key<br/>AES-CTR outer decrypt + checksum<br/>PBKDF2(session_key, pass) → inner_key<br/>AES-CTR inner decrypt + checksum<br/>Parse type byte → Payload
Rust-->>R: KeyTeleportPayload (Mnemonic | Xprv)
R->>R: ReviewImport screen
R->>Rust: ImportWalletManager.importWallet(words)
Rust-->>R: WalletMetadata
R->>R: resetRoute → SelectedWallet
Reviews (2): Last reviewed commit: "Fix iOS Swift exhaustive switch errors f..." | 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: 14
🧹 Nitpick comments (7)
android/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.kt (1)
418-433: Reusingicon_qr_codeas the Key Teleport leading icon is ambiguous.The first ListItem in this sheet ("QR Code") already uses
R.drawable.icon_qr_code. Giving Key Teleport the same icon makes the two entries visually indistinguishable at a glance. Consider a distinct icon (e.g. a teleport/link/bolt glyph) to aid recognition.Also minor: the supporting text "Receive mnemonic or XPRV wirelessly" lives under the hardware wallet import sheet, but Key Teleport is transport‑agnostic w.r.t. the source device — consider whether placement/copy should be revisited now that it's promoted as a top-level option.
🤖 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/flows/NewWalletFlow/NewWalletSelectScreen.kt` around lines 418 - 433, The Key Teleport ListItem reuses R.drawable.icon_qr_code which makes it visually ambiguous with the QR Code entry; change the leading icon for the Key Teleport ListItem (the ListItem instance that calls onOpenKeyTeleport and toggles showHardwareWalletSheet) to a distinct drawable (e.g., R.drawable.icon_teleport or R.drawable.icon_link/bolt) and update resources if needed; additionally review the supportingContent text ("Receive mnemonic or XPRV wirelessly") in that same ListItem to ensure its copy reflects Key Teleport being transport‑agnostic or move/adjust the string to the appropriate sheet if it is specific to hardware wallet imports.rust/crates/cove-keyteleport/src/lib.rs (1)
103-114: Password-length coverage gap in negative test.
wrong_teleport_password_failsuses an 8-char password"WRONGPAS"that happens to match the probable expected length. Consider also asserting error behavior for wrong-length passwords (e.g. empty, 7, 9, non-ASCII) so regressions in input validation vs. the checksum path are distinguishable — right now only theChecksumMismatchpath is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/lib.rs` around lines 103 - 114, The test wrong_teleport_password_fails only checks a single 8-char wrong password and thus conflates checksum paths with input-validation; extend this test to call receiver.decode(&s_pkt, ...) with additional password variants: empty string, 7-char, 9-char, and a non-ASCII password, and add assertions for each that they return the input-validation error (e.g. Error::InvalidPasswordLength or the crate's appropriate validation error) rather than Error::ChecksumMismatch; use the same ReceiverSession::generate/SenderSession::new flow to produce r_pkt/s_pkt and update the expected Error variant name to match your crate's actual enum if it differs.rust/src/multi_format.rs (1)
110-115: Use a strict case-sensitive prefix check instead of allocating an uppercased copy.The current code allocates a full uppercased string just to check a 4-byte prefix, which is inefficient. Since the BBQr specification defines headers as case-sensitive (with uppercase "B$2S" as the canonical form), the code should use strict case-sensitive matching instead.
♻️ Proposed fix
- // Key Teleport sender packet (BBQr S-type): B$2S... - let upper = string.to_ascii_uppercase(); - if upper.starts_with("B$2S") { - return Ok(Self::KeyTeleportSenderPacket(string.to_string())); - } + // Key Teleport sender packet (BBQr S-type): B$2S... + if string.starts_with("B$2S") { + return Ok(Self::KeyTeleportSenderPacket(string.to_string())); + }This approach is more efficient, avoids unnecessary allocation, and correctly implements the spec-mandated case-sensitive validation.
🤖 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 110 - 115, The code currently uppercases the entire input and checks starts_with("B$2S"), which allocates unnecessarily; change the check in multi_format.rs to a case-sensitive prefix check (e.g., use string.starts_with("B$2S") or an equivalent byte-based starts_with) and remove the to_ascii_uppercase allocation/variable (the branch that returns Ok(Self::KeyTeleportSenderPacket(string.to_string())) should remain unchanged). This enforces the spec's uppercase-only header and avoids the extra allocation.rust/crates/cove-keyteleport/src/payload.rs (1)
50-51: Usemap_err_prefixfor the static error context.This follows the project convention and avoids hand-formatting the same pattern.
Proposed cleanup
+use cove_util::ResultExt as _; @@ - let m = Mnemonic::from_entropy(entropy) - .map_err(|e| Error::InvalidPayload(format!("invalid mnemonic entropy: {e}")))?; + let m = Mnemonic::from_entropy(entropy) + .map_err_prefix(Error::InvalidPayload, "invalid mnemonic entropy")?;As per coding guidelines, “Use
cove_util::ResultExt::map_err_prefixinstead of.map_err(|e| Error::Variant(format!("context: {e}")))when the prefix is a static string”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/payload.rs` around lines 50 - 51, Replace the manual map_err closure on Mnemonic::from_entropy with the project helper map_err_prefix: call .map_err_prefix("invalid mnemonic entropy: ") so the static string is used as the error prefix and the underlying error is appended into Error::InvalidPayload via cove_util::ResultExt::map_err_prefix instead of formatting the error inline; target the Mnemonic::from_entropy(...) call that currently maps to Error::InvalidPayload.rust/crates/cove-keyteleport/src/packet.rs (1)
91-92: Usemap_err_prefixfor sender public-key parse context.This is a static-prefix error conversion and should follow the repository helper. As per coding guidelines, “Use
cove_util::ResultExt::map_err_prefixinstead of.map_err(|e| Error::Variant(format!("context: {e}")))when the prefix is a static string — produces"context: error_message".”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/packet.rs` around lines 91 - 92, Replace the ad-hoc map_err on PublicKey::from_slice with the repository helper: use cove_util::ResultExt::map_err_prefix to convert the error into Error::InvalidSenderPacket with the static prefix "bad sender pubkey: " instead of using .map_err(|e| Error::InvalidSenderPacket(format!("bad sender pubkey: {e}"))). Update the call that sets sender_pubkey (the PublicKey::from_slice(...) expression) to use map_err_prefix so the resulting error message is produced via the helper and still mapped into Error::InvalidSenderPacket.rust/src/key_teleport.rs (1)
28-34: Usemap_err_strfor direct error string conversion.Both mappings only wrap
e.to_string()into a UniFFI error variant. As per coding guidelines, “Usecove_util::ResultExt::map_err_strinstead of.map_err(|e| Error::Variant(e.to_string()))— it's cleaner and equivalent.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/key_teleport.rs` around lines 28 - 34, Replace the explicit .map_err(|e| KeyTeleportError::... (e.to_string())) calls with the provided ResultExt::map_err_str helper; specifically, in the SenderPacket::from_bbqr call that currently maps to KeyTeleportError::InvalidSenderPacket and in the self.0.decode call that maps to KeyTeleportError::DecodeFailed, import/qualify cove_util::ResultExt::map_err_str and call .map_err_str(KeyTeleportError::InvalidSenderPacket) and .map_err_str(KeyTeleportError::DecodeFailed) respectively so the error string conversion is handled by the utility function.rust/crates/cove-keyteleport/src/bbqr.rs (1)
81-84: Usemap_err_prefixfor the Base32 decode context.This is the repository-standard helper for static-prefix error mapping here. As per coding guidelines, “Use
cove_util::ResultExt::map_err_prefixinstead of.map_err(|e| Error::Variant(format!("context: {e}")))when the prefix is a static string — produces"context: error_message".”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/bbqr.rs` around lines 81 - 84, Replace the current manual map_err on the Base32 decode with the repository helper: change the BASE32_NOPAD.decode(b32_data.as_bytes()).map_err(|e| Error::InvalidBbqr(format!("Base32 decode failed: {e}")))? call so it uses cove_util::ResultExt::map_err_prefix to attach the static prefix "Base32 decode failed" while still mapping to Error::InvalidBbqr; locate the decode call that produces the data variable (uses b32_data and BASE32_NOPAD) and call .map_err_prefix("Base32 decode failed").map_err(|msg| Error::InvalidBbqr(msg))? (or the equivalent pattern supported by map_err_prefix) to preserve the static-prefix behavior.
🤖 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 39230-39235: The route enum currently embeds sensitive
mnemonic/XPRV material—remove the `wordsOrXprv` field from the generated
`ReviewImport` data class in the UniFFI route enum (and any similar generated
variants referenced by `NewWalletRoute.KeyTeleportReceive`) so routes carry only
non-sensitive data like `payloadKind`; instead, keep the full
`KeyTeleportPayload` in ephemeral in-memory receive-flow state (e.g., the
Android receive container/viewmodel for the review screen) for the lifetime of
the review, then regenerate the Kotlin bindings from the UniFFI source so
`ReviewImport` contains only a review marker and `payloadKind`, and update
consumers of `NewWalletRoute.KeyTeleportReceive` to retrieve the secret payload
from the receive container/viewmodel rather than from route state.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveImportScreen.kt`:
- Around line 70-75: The ImportWalletManager instance (created in
KeyTeleportReceiveImportScreen) may not be closed if importWallet(...) throws;
wrap its usage in a try/finally: create ImportWalletManager(), call
importWallet(wordGroups) inside the try, store metadata, then in finally always
call manager.close(); after the try/finally call
app.rust.selectWallet(metadata.id) (or call selectWallet inside the try if you
need immediate selection) so resource cleanup (ImportWalletManager.close) always
runs even on failure.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.kt`:
- Around line 68-75: The call to the Rust FFI decoder is being moved to a
background dispatcher via withContext(Dispatchers.Default) inside tryDecode;
remove that wrapper so the synchronous session.decode(senderPacketBbqr,
password) call runs on the current coroutine instead. Locate function tryDecode
and the block using withContext(Dispatchers.Default) and replace it by invoking
session.decode(...) directly (preserving the surrounding scope.launch, try/catch
and isDecoding handling) so the Rust FFI call is not dispatched to a background
thread.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.kt`:
- Around line 83-106: The screen currently only renders the QR (see
androidx.compose.foundation.Image using qrBitmap in
KeyTeleportReceiveShowQrScreen) but needs a copy/share path for the
bbqr/shareable link; add UI actions adjacent to the QR (e.g., row of buttons
under the Image) that 1) derive the shareable link or bbqr string from the same
source used to build qrBitmap and 2) copy that string to the clipboard and 3)
launch a platform share intent/send chooser to share the string; implement
handlers that reference the same data-producing symbol (the value used to create
qrBitmap) so the copied/shared text always matches the QR and provide user
feedback (toast/snackbar) on success/failure.
In `@rust/crates/cove-keyteleport/src/packet.rs`:
- Around line 86-94: The from_bytes constructor currently accepts exactly 33
bytes which yields an empty encrypted_body that later fails decoding; update the
guard in SenderPacket::from_bytes to reject byte slices of length ≤33 by
returning Error::InvalidSenderPacket (e.g. change the conditional from
bytes.len() < 33 to bytes.len() <= 33 and adjust the error text to "too short
(need >33 bytes)"), ensuring encrypted_body is non-empty before creating the
SenderPacket.
In `@rust/crates/cove-keyteleport/src/payload.rs`:
- Around line 9-16: The Payload enum currently derives Debug which can leak
secrets via logs or panics; remove #[derive(Debug)] from the enum and either
implement a custom Debug impl for Payload that redacts sensitive contents (show
only variant names like "Mnemonic" or "Xprv" without revealing the
string/mnemonic) or avoid implementing Debug altogether; also check the Mnemonic
type (used in Payload::Mnemonic) and ensure it does not expose secrets via its
own Debug impl—replace or redact there if necessary.
- Around line 30-37: The XPRV branch currently silently accepts invalid Base58
and arbitrary strings in Payload::Xprv during to_bytes() (uses
bitcoin::base58::decode with a fallback) and from_bytes() re-encodes without
validation; make XPRV handling fallible and validate structure by parsing with
bitcoin::bip32::Xprv::from_str (or the parser that returns Result) before
encoding, reject invalid strings with an error instead of falling back to raw
bytes, ensure the decoded binary is exactly 78 bytes before emitting
TYPE_XPRV-prefixed bytes, and update Payload::Xprv (or to_bytes()/from_bytes())
to store/return a parsed Xprv or return a Result to propagate deserialization
failures so malformed keys are not accepted.
In `@rust/crates/cove-keyteleport/src/receiver.rs`:
- Around line 15-20: Remove the #[derive(Debug)] on ReceiverSession and add a
manual Debug impl for ReceiverSession that does not expose secrets: implement
fmt::Debug for ReceiverSession and format the struct showing the field names
(privkey_bytes and numeric_code) but replace their values with redacted
placeholders (e.g. "<redacted>") so the secret Zeroizing<[u8; 32]> and the
numeric_code are never emitted by debug logging.
- Line 33: Replace the biased modulo-based sampling that assigns numeric_code
with an unbiased bounded sampler: instead of using rand::random::<u32>() %
(MAX_NUMERIC_CODE + 1) assign numeric_code using the rand 0.10 bounded API
(rand::random_range or the equivalent range-based helper) to draw uniformly from
0..=MAX_NUMERIC_CODE (or 0..MAX_NUMERIC_CODE+1); update the numeric_code
assignment in receiver.rs and keep the MAX_NUMERIC_CODE constant unchanged.
In `@rust/crates/cove-keyteleport/src/sender.rs`:
- Around line 31-37: The decrypted receiver pubkey bytes are currently accepted
if they accidentally form a valid secp256k1 key, making wrong numeric-code
detection probabilistic; change this by adding and verifying an authenticated
checksum/MAC before using PublicKey::from_slice. Modify the sender/receiver
packet format so r_packet.encrypted_pubkey() contains an authentication tag
(e.g., HMAC-SHA256 or switch to an AEAD like AES-GCM) and in the code path
around receiver_pubkey_key, aes256ctr, and PublicKey::from_slice verify the
MAC/tag over the ciphertext/plaintext (using the same key derived by
receiver_pubkey_key) and return Error::InvalidReceiverPacket on MAC failure;
only if verification succeeds proceed to parse the bytes into PublicKey and then
derive session keys. Ensure the same change is applied in the other occurrence
referenced (lines 119-125) so both decrypt-and-parse flows perform authenticated
verification first.
- Around line 13-19: Remove the #[derive(Debug)] on SenderSession and replace it
with a manual Debug impl that redacts privkey_bytes, session_key, and
teleport_password (e.g., print "<redacted>"), and change teleport_password's
type from String to Zeroizing<String> so the password is zeroized on drop;
ensure you import zeroize::Zeroizing and update any constructors/uses to wrap
the password in Zeroizing::new(...). Apply the same changes (remove auto Debug,
redact secrets in Debug, and wrap passwords/secret Strings in Zeroizing) to the
other session-like structs noted in the review so all session secrets are
redacted and zeroized.
- Around line 81-99: encrypt currently produces AES-CTR keystreams
deterministically and can be called repeatedly on the same SenderSession,
leaking relationships between plaintexts; fix by introducing per-message
randomness and updating the API: make encrypt take &mut self (or consume self)
and generate a fresh nonce/IV for each call (e.g., a session.counter or random
96/128-bit nonce stored in SenderSession), use that nonce as the IV for
aes256ctr when encrypting both inner and outer layers, and include the nonce in
the returned SenderPacket (add a nonce field to SenderPacket::new and its
struct) so the recipient can decrypt; ensure unique identifying symbols to
change are encrypt(&self) -> encrypt(&mut self) or encrypt(self), SenderSession
(add nonce/counter), aes256ctr invocations, and SenderPacket::new/struct.
In `@rust/src/key_teleport.rs`:
- Around line 40-46: The current derived Debug on KeyTeleportPayload will expose
sensitive wallet material (Mnemonic.words and Xprv.xprv); remove Debug from the
derive and add a manual impl of std::fmt::Debug for KeyTeleportPayload that
prints only the variant names and redacts the inner secret fields (e.g., display
"Mnemonic { words: \"<redacted>\" }" and "Xprv { xprv: \"<redacted>\" }"); keep
the uniffi::Enum attribute and existing Clone implementation unchanged so
bindings remain, and ensure the custom Debug impl covers both
KeyTeleportPayload::Mnemonic and ::Xprv variants.
In `@rust/src/router.rs`:
- Around line 41-60: KeyTeleportReceiveRoute::ReviewImport currently embeds
plaintext secrets in words_or_xprv:String and derives Debug/Hash/Eq/Clone which
leaks secrets into logs, heap and across the UniFFI boundary; fix by removing
the raw String from the route and instead store a reference/opaque handle (e.g.
Arc<ReviewImportState> exposed as a UniFFI Object or a session-managed ID) that
the review screen fetches via a method which holds the secret in a
Zeroize+ZeroizeOnDrop type, or alternately wrap words_or_xprv in a Zeroize type
with redacted Debug and exclude it from Eq/Hash/Clone; also adjust
Route/debugging (to_debug_log) to redact this variant so secrets are never
formatted via {:?}.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.kt`:
- Around line 418-433: The Key Teleport ListItem reuses R.drawable.icon_qr_code
which makes it visually ambiguous with the QR Code entry; change the leading
icon for the Key Teleport ListItem (the ListItem instance that calls
onOpenKeyTeleport and toggles showHardwareWalletSheet) to a distinct drawable
(e.g., R.drawable.icon_teleport or R.drawable.icon_link/bolt) and update
resources if needed; additionally review the supportingContent text ("Receive
mnemonic or XPRV wirelessly") in that same ListItem to ensure its copy reflects
Key Teleport being transport‑agnostic or move/adjust the string to the
appropriate sheet if it is specific to hardware wallet imports.
In `@rust/crates/cove-keyteleport/src/bbqr.rs`:
- Around line 81-84: Replace the current manual map_err on the Base32 decode
with the repository helper: change the
BASE32_NOPAD.decode(b32_data.as_bytes()).map_err(|e|
Error::InvalidBbqr(format!("Base32 decode failed: {e}")))? call so it uses
cove_util::ResultExt::map_err_prefix to attach the static prefix "Base32 decode
failed" while still mapping to Error::InvalidBbqr; locate the decode call that
produces the data variable (uses b32_data and BASE32_NOPAD) and call
.map_err_prefix("Base32 decode failed").map_err(|msg| Error::InvalidBbqr(msg))?
(or the equivalent pattern supported by map_err_prefix) to preserve the
static-prefix behavior.
In `@rust/crates/cove-keyteleport/src/lib.rs`:
- Around line 103-114: The test wrong_teleport_password_fails only checks a
single 8-char wrong password and thus conflates checksum paths with
input-validation; extend this test to call receiver.decode(&s_pkt, ...) with
additional password variants: empty string, 7-char, 9-char, and a non-ASCII
password, and add assertions for each that they return the input-validation
error (e.g. Error::InvalidPasswordLength or the crate's appropriate validation
error) rather than Error::ChecksumMismatch; use the same
ReceiverSession::generate/SenderSession::new flow to produce r_pkt/s_pkt and
update the expected Error variant name to match your crate's actual enum if it
differs.
In `@rust/crates/cove-keyteleport/src/packet.rs`:
- Around line 91-92: Replace the ad-hoc map_err on PublicKey::from_slice with
the repository helper: use cove_util::ResultExt::map_err_prefix to convert the
error into Error::InvalidSenderPacket with the static prefix "bad sender pubkey:
" instead of using .map_err(|e| Error::InvalidSenderPacket(format!("bad sender
pubkey: {e}"))). Update the call that sets sender_pubkey (the
PublicKey::from_slice(...) expression) to use map_err_prefix so the resulting
error message is produced via the helper and still mapped into
Error::InvalidSenderPacket.
In `@rust/crates/cove-keyteleport/src/payload.rs`:
- Around line 50-51: Replace the manual map_err closure on
Mnemonic::from_entropy with the project helper map_err_prefix: call
.map_err_prefix("invalid mnemonic entropy: ") so the static string is used as
the error prefix and the underlying error is appended into Error::InvalidPayload
via cove_util::ResultExt::map_err_prefix instead of formatting the error inline;
target the Mnemonic::from_entropy(...) call that currently maps to
Error::InvalidPayload.
In `@rust/src/key_teleport.rs`:
- Around line 28-34: Replace the explicit .map_err(|e| KeyTeleportError::...
(e.to_string())) calls with the provided ResultExt::map_err_str helper;
specifically, in the SenderPacket::from_bbqr call that currently maps to
KeyTeleportError::InvalidSenderPacket and in the self.0.decode call that maps to
KeyTeleportError::DecodeFailed, import/qualify cove_util::ResultExt::map_err_str
and call .map_err_str(KeyTeleportError::InvalidSenderPacket) and
.map_err_str(KeyTeleportError::DecodeFailed) respectively so the error string
conversion is handled by the utility function.
In `@rust/src/multi_format.rs`:
- Around line 110-115: The code currently uppercases the entire input and checks
starts_with("B$2S"), which allocates unnecessarily; change the check in
multi_format.rs to a case-sensitive prefix check (e.g., use
string.starts_with("B$2S") or an equivalent byte-based starts_with) and remove
the to_ascii_uppercase allocation/variable (the branch that returns
Ok(Self::KeyTeleportSenderPacket(string.to_string())) should remain unchanged).
This enforces the spec's uppercase-only header and avoids the extra allocation.
🪄 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: 035e7fc9-368e-442f-b666-12c518b3b5f7
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveScanSenderScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/Cargo.tomlrust/crates/cove-keyteleport/Cargo.tomlrust/crates/cove-keyteleport/src/bbqr.rsrust/crates/cove-keyteleport/src/crypto.rsrust/crates/cove-keyteleport/src/error.rsrust/crates/cove-keyteleport/src/lib.rsrust/crates/cove-keyteleport/src/packet.rsrust/crates/cove-keyteleport/src/payload.rsrust/crates/cove-keyteleport/src/receiver.rsrust/crates/cove-keyteleport/src/sender.rsrust/src/key_teleport.rsrust/src/lib.rsrust/src/multi_format.rsrust/src/router.rs
Comment on lines
+31
to
+37
| let key = receiver_pubkey_key(numeric_code); | ||
| let pubkey_bytes = aes256ctr(&key, r_packet.encrypted_pubkey()); | ||
| let receiver_pubkey = PublicKey::from_slice(&pubkey_bytes).map_err(|_| { | ||
| Error::InvalidReceiverPacket( | ||
| "decrypted bytes are not a valid pubkey — wrong numeric code?".into(), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Make wrong numeric-code detection deterministic.
A wrong code is accepted whenever the decrypted bytes happen to parse as a secp256k1 public key; the test even notes this is probabilistic. Add an authenticated checksum/MAC to the encrypted receiver pubkey data and verify it here before deriving the session key.
Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 31 - 37, The
decrypted receiver pubkey bytes are currently accepted if they accidentally form
a valid secp256k1 key, making wrong numeric-code detection probabilistic; change
this by adding and verifying an authenticated checksum/MAC before using
PublicKey::from_slice. Modify the sender/receiver packet format so
r_packet.encrypted_pubkey() contains an authentication tag (e.g., HMAC-SHA256 or
switch to an AEAD like AES-GCM) and in the code path around receiver_pubkey_key,
aes256ctr, and PublicKey::from_slice verify the MAC/tag over the
ciphertext/plaintext (using the same key derived by receiver_pubkey_key) and
return Error::InvalidReceiverPacket on MAC failure; only if verification
succeeds proceed to parse the bytes into PublicKey and then derive session keys.
Ensure the same change is applied in the other occurrence referenced (lines
119-125) so both decrypt-and-parse flows perform authenticated verification
first.
Comment on lines
+81
to
+99
| pub fn encrypt(&self, payload: &Payload) -> SenderPacket { | ||
| let secp = Secp256k1::new(); | ||
| let sender_pubkey = self.privkey().public_key(&secp); | ||
|
|
||
| let inner_plain = payload.to_bytes(); | ||
| let inner_cs = checksum(&inner_plain); | ||
| let mut inner_with_cs = inner_plain; | ||
| inner_with_cs.extend_from_slice(&inner_cs); | ||
|
|
||
| let inner_key = pbkdf2_stretch(&self.session_key, self.teleport_password.as_bytes()); | ||
| let layer2 = aes256ctr(&inner_key, &inner_with_cs); | ||
|
|
||
| let outer_cs = checksum(&layer2); | ||
| let mut outer_with_cs = layer2; | ||
| outer_with_cs.extend_from_slice(&outer_cs); | ||
|
|
||
| let body = aes256ctr(&self.session_key, &outer_with_cs); | ||
|
|
||
| SenderPacket::new(sender_pubkey, body) |
There was a problem hiding this comment.
Prevent repeated encryption with the same CTR keystream.
encrypt(&self, ...) can be called multiple times on one SenderSession. Because the protocol uses AES-CTR with deterministic keys/IVs, repeated calls reuse keystreams and leak relationships between plaintexts. Consume the session or add a nonce to the packet format.
Minimal API guard
- pub fn encrypt(&self, payload: &Payload) -> SenderPacket {
+ pub fn encrypt(self, payload: &Payload) -> SenderPacket {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn encrypt(&self, payload: &Payload) -> SenderPacket { | |
| let secp = Secp256k1::new(); | |
| let sender_pubkey = self.privkey().public_key(&secp); | |
| let inner_plain = payload.to_bytes(); | |
| let inner_cs = checksum(&inner_plain); | |
| let mut inner_with_cs = inner_plain; | |
| inner_with_cs.extend_from_slice(&inner_cs); | |
| let inner_key = pbkdf2_stretch(&self.session_key, self.teleport_password.as_bytes()); | |
| let layer2 = aes256ctr(&inner_key, &inner_with_cs); | |
| let outer_cs = checksum(&layer2); | |
| let mut outer_with_cs = layer2; | |
| outer_with_cs.extend_from_slice(&outer_cs); | |
| let body = aes256ctr(&self.session_key, &outer_with_cs); | |
| SenderPacket::new(sender_pubkey, body) | |
| pub fn encrypt(self, payload: &Payload) -> SenderPacket { | |
| let secp = Secp256k1::new(); | |
| let sender_pubkey = self.privkey().public_key(&secp); | |
| let inner_plain = payload.to_bytes(); | |
| let inner_cs = checksum(&inner_plain); | |
| let mut inner_with_cs = inner_plain; | |
| inner_with_cs.extend_from_slice(&inner_cs); | |
| let inner_key = pbkdf2_stretch(&self.session_key, self.teleport_password.as_bytes()); | |
| let layer2 = aes256ctr(&inner_key, &inner_with_cs); | |
| let outer_cs = checksum(&layer2); | |
| let mut outer_with_cs = layer2; | |
| outer_with_cs.extend_from_slice(&outer_cs); | |
| let body = aes256ctr(&self.session_key, &outer_with_cs); | |
| SenderPacket::new(sender_pubkey, body) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 81 - 99, encrypt
currently produces AES-CTR keystreams deterministically and can be called
repeatedly on the same SenderSession, leaking relationships between plaintexts;
fix by introducing per-message randomness and updating the API: make encrypt
take &mut self (or consume self) and generate a fresh nonce/IV for each call
(e.g., a session.counter or random 96/128-bit nonce stored in SenderSession),
use that nonce as the IV for aes256ctr when encrypting both inner and outer
layers, and include the nonce in the returned SenderPacket (add a nonce field to
SenderPacket::new and its struct) so the recipient can decrypt; ensure unique
identifying symbols to change are encrypt(&self) -> encrypt(&mut self) or
encrypt(self), SenderSession (add nonce/counter), aes256ctr invocations, and
SenderPacket::new/struct.
Comment on lines
+40
to
+46
| #[derive(Debug, Clone, uniffi::Enum)] | ||
| pub enum KeyTeleportPayload { | ||
| /// A BIP-39 mnemonic — the word list as a space-separated string. | ||
| Mnemonic { words: String }, | ||
| /// A serialized XPRV (base58). | ||
| Xprv { xprv: String }, | ||
| } |
There was a problem hiding this comment.
Redact debug output for decrypted wallet secrets.
Mnemonic { words } and Xprv { xprv } are private wallet material; derived Debug can put them into logs or crash diagnostics.
🔒 Proposed redacted debug implementation
-#[derive(Debug, Clone, uniffi::Enum)]
+#[derive(Clone, uniffi::Enum)]
pub enum KeyTeleportPayload {
@@
Xprv { xprv: String },
}
+
+impl std::fmt::Debug for KeyTeleportPayload {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::Mnemonic { .. } => f.debug_struct("Mnemonic").field("words", &"<redacted>").finish(),
+ Self::Xprv { .. } => f.debug_struct("Xprv").field("xprv", &"<redacted>").finish(),
+ }
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/key_teleport.rs` around lines 40 - 46, The current derived Debug on
KeyTeleportPayload will expose sensitive wallet material (Mnemonic.words and
Xprv.xprv); remove Debug from the derive and add a manual impl of
std::fmt::Debug for KeyTeleportPayload that prints only the variant names and
redacts the inner secret fields (e.g., display "Mnemonic { words: \"<redacted>\"
}" and "Xprv { xprv: \"<redacted>\" }"); keep the uniffi::Enum attribute and
existing Clone implementation unchanged so bindings remain, and ensure the
custom Debug impl covers both KeyTeleportPayload::Mnemonic and ::Xprv variants.
Android compile errors: - Add KeyTeleportSenderPacket branch to AppManager.handleMultiFormat - Fix KeyTeleportReceiverSession() constructor call (was .new()) - Add missing app/onDismiss params to QrCodeScanView in ScanSenderScreen - Fix QrCodeGenerator import (org.bitcoinppl.cove, not .views) - Add Copy/Share buttons for remote sender use case Security / code quality: - Move decrypted payload out of route state into ephemeral container state (ReviewImport now carries only payloadKind, not wordsOrXprv) - Remove withContext(Dispatchers.Default) around synchronous Rust FFI call - Wrap ImportWalletManager in try/finally so close() always runs - XPRV branch shows unsupported error instead of calling wrong API - Redact Payload::Mnemonic / Payload::Xprv from Debug output - Fix modulo bias in numeric code generation (rejection sampling) - Stricter SenderPacket minimum length check (pubkey + body, not just pubkey) Regenerate bindings: - Kotlin cove.kt regenerated (ReviewImport loses wordsOrXprv field) - Swift cove.swift regenerated (525 lines for new Key Teleport types)
Add keyTeleportSenderPacket case to handleFileOpen and handleMultiFormat switches in CoveMainView.swift (these became non-exhaustive after adding the new MultiFormat variant). Add keyTeleportReceive case to NewWalletContainer.swift switch (placeholder stub — iOS receive flow not yet implemented).
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
rust/crates/cove-keyteleport/src/packet.rs (2)
8-11: Avoid derivingDebugfor packet contents that carry wallet import material.These derives can dump encrypted teleport payload bytes into logs if a packet is formatted with
{:?}. Consider redactedDebugimplementations that only expose lengths/type metadata. As per coding guidelines,rust/**/*.rs: “Check for potential security issues, make sure Bitcoin wallets are handled securely.”🔒 Proposed redacted Debug shape
-#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct ReceiverPacket { /// The AES-encrypted compressed public key (33 bytes). encrypted_pubkey: [u8; 33], } + +impl core::fmt::Debug for ReceiverPacket { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("ReceiverPacket") + .field("encrypted_pubkey_len", &self.encrypted_pubkey.len()) + .finish() + } +} ... -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct SenderPacket { sender_pubkey: PublicKey, encrypted_body: Vec<u8>, } + +impl core::fmt::Debug for SenderPacket { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("SenderPacket") + .field("sender_pubkey", &"<redacted>") + .field("encrypted_body_len", &self.encrypted_body.len()) + .finish() + } +}Also applies to: 51-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/packet.rs` around lines 8 - 11, ReceiverPacket (and the similar packet struct at lines 51-55, e.g., SenderPacket) currently derives Debug which can leak encrypted payload bytes (e.g., the encrypted_pubkey field) into logs; remove Debug from the derive list for these packet types and implement a custom redacted Debug fmt for ReceiverPacket and the other packet struct that only emits safe metadata (type name, lengths or presence flags) — for example, implement std::fmt::Debug for ReceiverPacket to print "ReceiverPacket { encrypted_pubkey: [REDACTED; 33] }" or better just "ReceiverPacket { encrypted_pubkey_len: 33 }" and replicate the same pattern for the other packet struct so no raw bytes are ever included in debug output.
96-97: Usemap_err_prefixfor the static pubkey parse context.This matches the repository error-mapping convention and keeps the error prefixing consistent. As per coding guidelines,
**/*.rs: "Usecove_util::ResultExt::map_err_prefixinstead of.map_err(|e| Error::Variant(format!("context: {e}")))when the prefix is a static string."♻️ Proposed direction
use bitcoin::secp256k1::PublicKey; +use cove_util::ResultExt as _; use crate::bbqr::{self, KtFileType}; use crate::error::Error; ... let sender_pubkey = PublicKey::from_slice(&bytes[..COMPRESSED_PUBKEY_LEN]) - .map_err(|e| Error::InvalidSenderPacket(format!("bad sender pubkey: {e}")))?; + .map_err_prefix("bad sender pubkey", Error::InvalidSenderPacket)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/packet.rs` around lines 96 - 97, The call to PublicKey::from_slice when building sender_pubkey currently uses .map_err(|e| Error::InvalidSenderPacket(format!("bad sender pubkey: {e}"))) which violates the project convention; replace that map_err with the utility map_err_prefix from cove_util::ResultExt so the static prefix "bad sender pubkey" is applied consistently (i.e., call PublicKey::from_slice(&bytes[..COMPRESSED_PUBKEY_LEN]).map_err_prefix("bad sender pubkey", Error::InvalidSenderPacket) or the equivalent API your ResultExt exposes) and keep the same Error::InvalidSenderPacket variant.android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.kt (1)
61-63: Consider caching the BBQr / code in the session-owning container to survive configuration changes.
remember(session)ties the BBQr and numeric code to the session instance; since the session is owned byKeyTeleportReceiveContainerviaDisposableEffect(Unit), this is fine for the happy path. However, sincereceiverPacketBbqr()andnumericCodeDisplay()both cross the FFI on each (re)composition with a new session (e.g., process recreation), you may want torememberSaveablethe BBQr string / code at the container level so the QR doesn't change if the user navigates forward and back. Optional polish.🤖 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/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.kt` around lines 61 - 63, The BBQr and numeric code are being tied to the session instance via remember(session) in KeyTeleportReceiveShowQrScreen which causes FFI calls on process recreation; move caching to the session-owning container (KeyTeleportReceiveContainer) that uses DisposableEffect(Unit) and store the strings with rememberSaveable there (e.g., capture session.receiverPacketBbqr() and session.numericCodeDisplay() once and expose them), then have KeyTeleportReceiveShowQrScreen use those saved values and generate the QR with QrCodeGenerator.generate(savedBbqr, 512) to avoid repeated FFI crossings and to survive configuration changes.android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.kt (1)
60-60: Password is held as an immutableString; consider clearing on disposal.The teleport password is sensitive but stored in an immutable
Stringstate that remains in memory (and inOutlinedTextField's internal buffer) until GC. Since Kotlin Strings can't be zeroized, the best you can do is clearpassword = ""on navigation away (e.g., in aDisposableEffect { onDispose { password = "" } }) and trust the broader app lifecycle. Optional hardening consistent with the PR's "moved decrypted payload out of route state" security posture.🤖 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/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.kt` at line 60, The password state is an immutable String stored via "var password by remember { mutableStateOf(\"\") }" and must be cleared when the composable is disposed; add a DisposableEffect tied to the composable lifecycle (e.g., DisposableEffect(Unit) { onDispose { password = "" } }) so the "password" variable (and reduce trace in OutlinedTextField) is reset on navigation away/disposal; locate the OutlinedTextField using this "password" state and ensure the DisposableEffect's onDispose clears it.android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
341-344: Scanned sender-packet payload is discarded; user must re-scan inside the flow.
multiFormat.v1(the scanned sender BBQr string) is dropped and the flow restarts atShowQr→ScanSender. This matches the iOS behavior inCoveMainView.swift:491so it's consistent, but it's a poor UX for the "scanned outside the flow" case the comment describes. Consider threading the scanned BBQr into the receive flow (e.g., jumping straight toEnterPasswordwith that payload) once sessions can be bootstrapped with an existing packet — or at minimum, document it as known follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt` around lines 341 - 344, The handler for MultiFormat.KeyTeleportSenderPacket currently discards the scanned payload and restarts the receive flow via pushRoute(RouteFactory().keyTeleportReceive()); instead, propagate the scanned BBQr payload (multiFormat.v1) into the receive flow so the user doesn't need to re-scan: modify the branch that matches MultiFormat.KeyTeleportSenderPacket to pass the scanned packet into the route creation (or navigate directly to the EnterPassword step with that payload) by adding an overload or parameter to RouteFactory().keyTeleportReceive(...) or by calling the route/method that accepts the initial packet; if session bootstrapping with an existing packet isn’t yet supported add a short TODO comment documenting this known UX limitation.
🤖 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/flows/KeyTeleportFlow/KeyTeleportReceiveScanSenderScreen.kt`:
- Around line 57-72: In KeyTeleportReceiveScanSenderScreen's onScanned handler,
when the scanned item is invalid (the else branch that currently sets
app.alertState = TaggedItem(AppAlertState.General...)), also dismiss the scanner
like the other screens do: set the scanner controller/state to close (for
example set showQrScanner = false if that state exists, or set the parent
sheetState = null / invoke the same closeScanner method used by
SendFlowHardwareScreen/SendFlowContainer). Update the else branch so it both
sets app.alertState and triggers the same scanner dismissal mechanism to prevent
repeated invalid scans.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.kt`:
- Around line 96-206: The crash is caused by using Spacer(Modifier.weight(1f))
inside the vertically scrollable Column (verticalScroll(rememberScrollState())),
which gives infinite height; remove the weighted Spacer and either (A) move the
Button(onClick = onContinue) out of the scrollable Column so the Column can
scroll and the button sits fixed at the bottom, or (B) replace
Spacer(Modifier.weight(1f)) with a fixed-height spacer (e.g., Spacer with
Modifier.height(...)) so the scrollable Column no longer relies on weight;
locate usages of Column (the outer composable), Modifier.weight, Spacer, and the
Button/onContinue to implement the change.
In `@ios/Cove/CoveMainView.swift`:
- Around line 490-491: The current switch case pushes
RouteFactory().keyTeleportReceive() which leads to the iOS placeholder in
NewWalletContainer (keyTeleportReceive) — instead, change the
.keyTeleportSenderPacket handling to surface an explicit alert (for example use
an existing .invalidFormat alert or add a dedicated KeyTeleport iOS-unsupported
alert) rather than navigating to keyTeleportReceive; update the handling in
CoveMainView.swift (the case .keyTeleportSenderPacket) to call the app alert
presentation (or push a RouteFactory().keyTeleportiOSUnsupported() route you
add) so the user sees a clear message like "Key Teleport is Android-only for
now" until the iOS flow is implemented.
In `@ios/Cove/Flows/NewWalletFlow/Container/NewWalletContainer.swift`:
- Around line 21-22: The current routing stub for NewWalletContainer's case
.keyTeleportReceive lands users on a dead-end Text view; update the behavior so
scans don't produce an unreachable screen: either replace the single-line Text
in NewWalletContainer (case .keyTeleportReceive) with a minimal SwiftUI view
that includes explanatory copy and an explicit dismiss/back affordance (e.g., a
visible "Dismiss" or "Back" button that calls the view's dismiss action), or
change the scan handler (handleMultiFormat in CoveMainView) to gate
.keyTeleportSenderPacket and present an "iOS support coming soon" alert instead
of pushing this route; pick one approach and implement the change by editing
NewWalletContainer.case .keyTeleportReceive or the handleMultiFormat mapping for
.keyTeleportSenderPacket accordingly.
In `@rust/src/router.rs`:
- Around line 41-52: The enum KeyTeleportReceiveRoute currently derives Debug
and stores the full sender_packet_bbqr in the EnterPassword variant, which leads
to sensitive ciphertext being printed where code formats routes with "{:?}"; fix
by preventing that field from being rendered: either implement a custom Debug
for KeyTeleportReceiveRoute that masks or omits sender_packet_bbqr when
formatting EnterPassword, or change the EnterPassword field to a newtype (e.g.,
SensitiveString) whose Debug impl redacts the contents, and update any code that
formats routes with "{:?}" to use the safe Debug/Display; ensure the unique
symbols touched are KeyTeleportReceiveRoute and the EnterPassword {
sender_packet_bbqr: String } variant (and the site that formats routes with
"{:?}") so logs never include the raw packet.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt`:
- Around line 341-344: The handler for MultiFormat.KeyTeleportSenderPacket
currently discards the scanned payload and restarts the receive flow via
pushRoute(RouteFactory().keyTeleportReceive()); instead, propagate the scanned
BBQr payload (multiFormat.v1) into the receive flow so the user doesn't need to
re-scan: modify the branch that matches MultiFormat.KeyTeleportSenderPacket to
pass the scanned packet into the route creation (or navigate directly to the
EnterPassword step with that payload) by adding an overload or parameter to
RouteFactory().keyTeleportReceive(...) or by calling the route/method that
accepts the initial packet; if session bootstrapping with an existing packet
isn’t yet supported add a short TODO comment documenting this known UX
limitation.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.kt`:
- Line 60: The password state is an immutable String stored via "var password by
remember { mutableStateOf(\"\") }" and must be cleared when the composable is
disposed; add a DisposableEffect tied to the composable lifecycle (e.g.,
DisposableEffect(Unit) { onDispose { password = "" } }) so the "password"
variable (and reduce trace in OutlinedTextField) is reset on navigation
away/disposal; locate the OutlinedTextField using this "password" state and
ensure the DisposableEffect's onDispose clears it.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.kt`:
- Around line 61-63: The BBQr and numeric code are being tied to the session
instance via remember(session) in KeyTeleportReceiveShowQrScreen which causes
FFI calls on process recreation; move caching to the session-owning container
(KeyTeleportReceiveContainer) that uses DisposableEffect(Unit) and store the
strings with rememberSaveable there (e.g., capture session.receiverPacketBbqr()
and session.numericCodeDisplay() once and expose them), then have
KeyTeleportReceiveShowQrScreen use those saved values and generate the QR with
QrCodeGenerator.generate(savedBbqr, 512) to avoid repeated FFI crossings and to
survive configuration changes.
In `@rust/crates/cove-keyteleport/src/packet.rs`:
- Around line 8-11: ReceiverPacket (and the similar packet struct at lines
51-55, e.g., SenderPacket) currently derives Debug which can leak encrypted
payload bytes (e.g., the encrypted_pubkey field) into logs; remove Debug from
the derive list for these packet types and implement a custom redacted Debug fmt
for ReceiverPacket and the other packet struct that only emits safe metadata
(type name, lengths or presence flags) — for example, implement std::fmt::Debug
for ReceiverPacket to print "ReceiverPacket { encrypted_pubkey: [REDACTED; 33]
}" or better just "ReceiverPacket { encrypted_pubkey_len: 33 }" and replicate
the same pattern for the other packet struct so no raw bytes are ever included
in debug output.
- Around line 96-97: The call to PublicKey::from_slice when building
sender_pubkey currently uses .map_err(|e|
Error::InvalidSenderPacket(format!("bad sender pubkey: {e}"))) which violates
the project convention; replace that map_err with the utility map_err_prefix
from cove_util::ResultExt so the static prefix "bad sender pubkey" is applied
consistently (i.e., call
PublicKey::from_slice(&bytes[..COMPRESSED_PUBKEY_LEN]).map_err_prefix("bad
sender pubkey", Error::InvalidSenderPacket) or the equivalent API your ResultExt
exposes) and keep the same Error::InvalidSenderPacket variant.
🪄 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: 356566e1-663d-4875-b411-2c2d7f038f69
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (13)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceivePasswordScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveScanSenderScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveShowQrScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/CoveMainView.swiftios/Cove/Flows/NewWalletFlow/Container/NewWalletContainer.swiftrust/crates/cove-keyteleport/src/packet.rsrust/crates/cove-keyteleport/src/payload.rsrust/crates/cove-keyteleport/src/receiver.rsrust/src/router.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveContainer.kt
- android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveImportScreen.kt
- rust/crates/cove-keyteleport/src/receiver.rs
- android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt
Comment on lines
+57
to
+72
| onScanned = { multiFormat -> | ||
| when (multiFormat) { | ||
| is MultiFormat.KeyTeleportSenderPacket -> { | ||
| onScanned(multiFormat.v1) | ||
| } | ||
| else -> { | ||
| app.alertState = TaggedItem( | ||
| AppAlertState.General( | ||
| title = "Wrong QR Code", | ||
| message = "Please scan the sender's Key Teleport QR code (starts with B\$2S)", | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'QrCodeScanView.kt' | head -20
rg -nP -C5 '\bonScanned\b|fun\s+QrCodeScanView' --type=kotlinRepository: bitcoinppl/cove
Length of output: 20485
🏁 Script executed:
cat -n android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.kt | head -200Repository: bitcoinppl/cove
Length of output: 8145
🏁 Script executed:
# Check if there's any state management that stops the scanner after onScanned
rg -n 'scanComplete|scannedData' android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.kt -A2 -B2Repository: bitcoinppl/cove
Length of output: 1666
🏁 Script executed:
# Compare how other screens handle invalid QR codes vs KeyTeleportReceiveScanSenderScreen
rg -n 'onScanned.*{' android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowHardwareScreen.kt -A10 | head -30Repository: bitcoinppl/cove
Length of output: 146
🏁 Script executed:
# Check the full context of how KeyTeleportReceiveScanSenderScreen is used
cat -n android/app/src/main/java/org/bitcoinppl/cove/flows/KeyTeleportFlow/KeyTeleportReceiveScanSenderScreen.kt | head -75Repository: bitcoinppl/cove
Length of output: 3487
Consider dismissing the scanner on invalid QR code, consistent with similar screens.
The scanner continues to stream and re-arm after an invalid QR code (not frozen), but the behavior differs from comparable screens in the app—SendFlowHardwareScreen sets showQrScanner = false, and SendFlowContainer/MainActivity close their scanner sheets via sheetState = null when handling results. Here, the alert alone leaves the scanner active, allowing repeated invalid scans. For consistency and clearer UX, either dismiss the scanner after the alert or add explicit logic to close it.
🤖 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/flows/KeyTeleportFlow/KeyTeleportReceiveScanSenderScreen.kt`
around lines 57 - 72, In KeyTeleportReceiveScanSenderScreen's onScanned handler,
when the scanned item is invalid (the else branch that currently sets
app.alertState = TaggedItem(AppAlertState.General...)), also dismiss the scanner
like the other screens do: set the scanner controller/state to close (for
example set showQrScanner = false if that state exists, or set the parent
sheetState = null / invoke the same closeScanner method used by
SendFlowHardwareScreen/SendFlowContainer). Update the else branch so it both
sets app.alertState and triggers the same scanner dismissal mechanism to prevent
repeated invalid scans.
Security:
- Custom Debug for KeyTeleportReceiveRoute that redacts sender_packet_bbqr
so ciphertext never appears in logs when routes are formatted with {:?}
iOS:
- Replace dead-end Text stub for keyTeleportReceive with a proper
KeyTeleportReceiveUnavailableView that has a Dismiss button
- Show an explicit "iOS not yet supported" alert when a Key Teleport
sender packet is scanned (instead of navigating to the placeholder view)
Android:
- Fix Spacer(weight=1f) crash inside verticalScroll Column in ShowQrScreen:
moved Button outside the scrollable Column into a Box, pinned to bottom
- Dismiss scanner (popRoute) after showing "Wrong QR Code" alert in
ScanSenderScreen so users are not stuck in the scanner after a bad scan
praveenperera
requested changes
Apr 20, 2026
|
|
||
| /// Key Teleport BBQr file type codes. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum KtFileType { |
Contributor
There was a problem hiding this comment.
i don't like abbreviations, especially not in type names, call it KeyTeleportFileType
Comment on lines
+37
to
+87
| pub fn encode(data: &[u8], file_type: KtFileType) -> String { | ||
| let b32 = BASE32_NOPAD.encode(data); | ||
| // num_parts=01 (1 frame total), part_index=00 (first/only frame) | ||
| format!("B$2{}0100{}", file_type.as_char(), b32) | ||
| } | ||
|
|
||
| /// Decode a single-frame BBQr string, returning the file type and binary payload. | ||
| /// Multi-frame packets are rejected — higher-level transport code handles reassembly. | ||
| pub fn decode(s: &str) -> Result<(KtFileType, Vec<u8>), Error> { | ||
| let s = s.trim().to_uppercase(); | ||
|
|
||
| let rest = | ||
| s.strip_prefix("B$").ok_or_else(|| Error::InvalidBbqr("missing 'B$' header".into()))?; | ||
|
|
||
| if rest.len() < 6 { | ||
| return Err(Error::InvalidBbqr("too short to be a valid BBQr packet".into())); | ||
| } | ||
|
|
||
| let mut chars = rest.chars(); | ||
| let encoding = chars.next().unwrap(); | ||
| if encoding != '2' { | ||
| return Err(Error::InvalidBbqr(format!( | ||
| "unsupported encoding '{encoding}' (only Base32/'2' is supported)" | ||
| ))); | ||
| } | ||
|
|
||
| let file_type = KtFileType::from_char(chars.next().unwrap())?; | ||
|
|
||
| // num_parts and part_index are 2 uppercase hex chars each | ||
| let header_tail: String = chars.take(4).collect(); | ||
| if header_tail.len() != 4 { | ||
| return Err(Error::InvalidBbqr("truncated header".into())); | ||
| } | ||
| let num_parts = u8::from_str_radix(&header_tail[0..2], 16) | ||
| .map_err(|_| Error::InvalidBbqr("bad num_parts".into()))?; | ||
| let part_index = u8::from_str_radix(&header_tail[2..4], 16) | ||
| .map_err(|_| Error::InvalidBbqr("bad part_index".into()))?; | ||
|
|
||
| if num_parts != 1 || part_index != 0 { | ||
| return Err(Error::InvalidBbqr(format!( | ||
| "multi-frame BBQr not supported here (num_parts={num_parts}, part_index={part_index})" | ||
| ))); | ||
| } | ||
|
|
||
| let b32_data = &s[8..]; // "B$" + encoding + type + 4 header chars = 8 | ||
| let data = BASE32_NOPAD | ||
| .decode(b32_data.as_bytes()) | ||
| .map_err(|e| Error::InvalidBbqr(format!("Base32 decode failed: {e}")))?; | ||
|
|
||
| Ok((file_type, data)) | ||
| } |
Contributor
There was a problem hiding this comment.
shouldn't we just use the rust bbqr crate for this? we are already using inside cove
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.
Implements the user-facing Key Teleport receive flow for issue #654, building on the
cove-keyteleportRust crate from #653.The receive flow follows the protocol: the receiver generates an ephemeral keypair and shows a BBQr packet and 8-digit verification code. The sender scans the QR, encrypts the payload, and shows their own QR. The receiver scans that back, enters the teleport password the sender shared out-of-band, and decrypts. A review screen shows the decoded mnemonic words or XPRV before committing the import.
On the Rust side,
key_teleport.rsexposesKeyTeleportReceiverSessionvia UniFFI so Android can create sessions, read the BBQr and numeric code, and calldecode(senderPacketBbqr, password)to get aKeyTeleportPayload. The router gets aKeyTeleportReceiveRouteenum (ShowQr → ScanSender → EnterPassword → ReviewImport) andMultiFormatgets aKeyTeleportSenderPacketvariant so the existing QR scanner can routeB$2Spackets into the flow without modification.On the Android side,
KeyTeleportReceiveContainerowns the session for the lifetime of the flow viaDisposableEffectand dispatches to four screens. The entry point is a new item in the hardware wallet import bottom sheet inNewWalletSelectScreen.Closes #654
Summary by CodeRabbit
New Features
Platform