detect silent payment addresses and surface a clear error#644
detect silent payment addresses and surface a clear error#644guptapratykshh wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughRejects BIP352 silent-payment addresses (sp1/tsp1) across Rust core, FFI, QR parsing, and Android/iOS UI. Adds a SilentPaymentNotSupported error variant, surfaces an "Unsupported Address" alert, and focuses the address input when the alert is dismissed. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as "Android/iOS Presenter"
participant QR as "QR Scanner"
participant FFI as FFI Layer
participant Rust as "Rust Core"
User->>UI: Paste or scan address (sp1... / tsp1...)
UI->>QR: (if scanned) deliver scanned string
QR->>UI: return scanned string
UI->>FFI: request parse/validate address
FFI->>Rust: call AddressWithNetwork::try_new / Address::from_string
Rust->>Rust: is_silent_payment_address() detects sp/tsp bech32m
Rust-->>FFI: return SilentPaymentNotSupported error
FFI-->>UI: deserialize as SilentPaymentNotSupported
UI->>UI: present "Unsupported Address" alert and set focus to address field on dismiss
UI-->>User: show alert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 SummaryDetects BIP352 silent payment addresses ( Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions that do not affect correctness or the happy path. The implementation is correct end-to-end: detection logic matches BIP352, the error propagates cleanly through UniFFI, lock handling is correct, and No files require special attention beyond the P2 suggestions on the alert title and test coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User pastes address string] --> B[handle_entering_address_changed]
B --> C[Address::from_string]
C --> D{is_silent_payment_address?}
D -- yes --> E[Err: AddressError::SilentPaymentNotSupported]
D -- no --> F[BDK address parse]
F -- invalid --> G[Err: AddressError::InvalidAddress]
F -- valid --> H[Ok: Address]
E --> I[SendFlowError::SilentPaymentNotSupported stored in state.address_error]
G --> J[SendFlowError::InvalidAddress stored in state.address_error]
H --> K[state.address = Some, state.address_error = None]
I --> L[validate_address called with display_alert=true]
J --> L
K --> M[Validation passes, send flow continues]
L --> N[SetAlert with stored address_error]
N --> O[iOS/Android: errorAlertTitle = 'Invalid Address', errorAlertMessage = SP-specific message]
Reviews (1): Last reviewed commit: "detect silent payment addresses and surf..." | 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: 1
🧹 Nitpick comments (1)
rust/src/manager/send_flow_manager.rs (1)
422-427: Prefer scoped lock extraction over explicitdrop(state).Line 426 uses manual
drop(state)just to release the mutex before sending. This can be expressed more idiomatically with a scoped block and avoids explicit-drop style drift.♻️ Suggested refactor
- let state = self.state.lock(); - let error = state.address_error.clone().unwrap_or_else(|| { - SendFlowError::InvalidAddress(state.entering_address.clone()) - }); - drop(state); + let error = { + let state = self.state.lock(); + state.address_error.clone().unwrap_or_else(|| { + SendFlowError::InvalidAddress(state.entering_address.clone()) + }) + }; self.reconciler.send(Message::SetAlert(error.into()));As per coding guidelines: "Prefer scoped blocks to release locks or borrows instead of explicit
drop(...)unless explicitdropis actually needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/send_flow_manager.rs` around lines 422 - 427, The mutex lock is released explicitly with drop(state); prefer a scoped block to limit the lock's lifetime: create the error value inside a short scope that acquires self.state.lock() (e.g., let error = { let state = self.state.lock(); state.address_error.clone().unwrap_or_else(|| SendFlowError::InvalidAddress(state.entering_address.clone())) };), then call self.reconciler.send(Message::SetAlert(error.into())); this removes the explicit drop and keeps the lock confined to the inner scope while preserving use of SendFlowError, state.entering_address, and Message::SetAlert.
🤖 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/crates/cove-types/src/address.rs`:
- Around line 513-532: Add assertions that Address::from_string rejects
silent-payment addresses the same way as AddressWithNetwork::try_new by calling
Address::from_string with the same mainnet, testnet, and bitcoin: URI SP strings
and asserting Err(AddressError::SilentPaymentNotSupported); update or add a test
(alongside test_silent_payment_address_detection) to call Address::from_string
for the three example inputs and assert the SilentPaymentNotSupported error so
the send-flow path is covered.
---
Nitpick comments:
In `@rust/src/manager/send_flow_manager.rs`:
- Around line 422-427: The mutex lock is released explicitly with drop(state);
prefer a scoped block to limit the lock's lifetime: create the error value
inside a short scope that acquires self.state.lock() (e.g., let error = { let
state = self.state.lock(); state.address_error.clone().unwrap_or_else(||
SendFlowError::InvalidAddress(state.entering_address.clone())) };), then call
self.reconciler.send(Message::SetAlert(error.into())); this removes the explicit
drop and keeps the lock confined to the inner scope while preserving use of
SendFlowError, state.entering_address, and Message::SetAlert.
🪄 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: 518e7684-e7fd-415c-b119-1bc9fe0b34c0
📒 Files selected for processing (6)
android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.ktios/Cove/Flows/SendFlow/SendFlowPresenter.swiftrust/crates/cove-types/src/address.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/error.rsrust/src/manager/send_flow_manager/state.rs
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
also read CONTRIBUTING.md and follow the commit message format
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
2 times, most recently
from
2ce66ac to
54253d7
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
- Regenerate UniFFI bindings for the new send-flow error, read
CONTRIBUTING.md - Use the URI-aware parser when surfacing silent-payment alerts
send_flow_manager.rs:426-428If the user pastes a BIP21 URI such as bitcoin:sp1... into the send field and then submits, this path reparses with Address::from_string, which only accepts raw addresses. In that scenario the new SilentPaymentNotSupported branch is never reached and the app still shows the generic invalid-address alert, even though AddressWithNetwork::try_new already recognizes silent-payment URIs and has tests for them.
praveenperera
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
54253d7 to
7f78fb8
Compare
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
d4d1862 to
bae3f32
Compare
praveenperera
force-pushed
the
fix/617-silent-payment-address-detection
branch
2 times, most recently
from
f8a5844 to
edfa409
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust/crates/cove-types/src/address.rs (1)
292-294:⚠️ Potential issue | 🟡 MinorHandle
bitcoin:silent-payment URIs inAddress::from_stringtoo.
is_silent_payment_address("bitcoin:sp1...")is false, so this constructor still returnsInvalidAddressfor a BIP21 silent-payment URI. If this FFI constructor oraddress_is_valid_for_networksees raw user input, the clear unsupported message is lost for URI-form inputs.🐛 Proposed fix
- if is_silent_payment_address(address) { + let is_silent_payment = parse_bitcoin_uri(address) + .map(|(parsed_address, _)| is_silent_payment_address(&parsed_address)) + .unwrap_or_else(|_| is_silent_payment_address(address)); + + if is_silent_payment { return Err(Error::SilentPaymentNotSupported); }As per coding guidelines, “Verify correct functionality and logic consistency.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-types/src/address.rs` around lines 292 - 294, Address::from_string currently misses BIP21 silent-payment URIs (e.g., "bitcoin:sp...") because is_silent_payment_address only recognizes raw addresses, so add a check that treats URI-form inputs as silent-payment too: in Address::from_string (and before calling address_is_valid_for_network) detect if the input starts with "bitcoin:sp" (or parse a bitcoin: URI and inspect its path) and if so return Err(Error::SilentPaymentNotSupported) so URI-form silent-payment addresses get the same clear error as raw ones; reference is_silent_payment_address, Address::from_string, and address_is_valid_for_network when locating the change.
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
43641-43642: Remove the empty default constructor.This keeps the no-arg constructor while satisfying detekt.
Proposed cleanup
- class SilentPaymentNotSupported( - ) : SendFlowException() { + class SilentPaymentNotSupported : SendFlowException() {🤖 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 43641 - 43642, The class SilentPaymentNotSupported currently declares an explicit empty primary constructor; remove the redundant empty constructor declaration so the class is declared as a no-arg class inheriting SendFlowException (i.e., change the declaration to omit the empty parentheses after the class name), ensuring you keep the inheritance to SendFlowException and satisfy detekt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rust/crates/cove-types/src/address.rs`:
- Around line 292-294: Address::from_string currently misses BIP21
silent-payment URIs (e.g., "bitcoin:sp...") because is_silent_payment_address
only recognizes raw addresses, so add a check that treats URI-form inputs as
silent-payment too: in Address::from_string (and before calling
address_is_valid_for_network) detect if the input starts with "bitcoin:sp" (or
parse a bitcoin: URI and inspect its path) and if so return
Err(Error::SilentPaymentNotSupported) so URI-form silent-payment addresses get
the same clear error as raw ones; reference is_silent_payment_address,
Address::from_string, and address_is_valid_for_network when locating the change.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 43641-43642: The class SilentPaymentNotSupported currently
declares an explicit empty primary constructor; remove the redundant empty
constructor declaration so the class is declared as a no-arg class inheriting
SendFlowException (i.e., change the declaration to omit the empty parentheses
after the class name), ensuring you keep the inheritance to SendFlowException
and satisfy detekt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa7e281c-e24c-491c-858d-4822fb93dcab
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/Flows/SendFlow/SendFlowPresenter.swiftrust/crates/cove-types/src/address.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/error.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rust/src/manager/send_flow_manager/error.rs
- ios/Cove/Flows/SendFlow/SendFlowPresenter.swift
- android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.kt
- rust/src/manager/send_flow_manager.rs
There was a problem hiding this comment.
@guptapratykshh Validate silent-payment candidates before surfacing unsupported crates/cove-types/src/address.rs
Any string that merely starts with sp1q or tsp1q is currently classified as SilentPaymentNotSupported, even if it is truncated, has a bad checksum, or is otherwise not a real silent-payment address. In those cases, users will see the new unsupported-address path instead of the existing invalid-address path, which is a user-visible validation regression. Please only return SilentPaymentNotSupported after confirming the input is a valid silent-payment encoding.
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
edfa409 to
6414386
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
43296-43297: Drop the empty constructor.This trips detekt and adds no value; Kotlin can declare the payload-free variant directly.
♻️ Proposed cleanup
- class SilentPaymentNotSupported( - ) : SendFlowException() { + class SilentPaymentNotSupported : SendFlowException() {🤖 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 43296 - 43297, The class SilentPaymentNotSupported currently declares an empty primary constructor; remove the empty parentheses and declare it as "class SilentPaymentNotSupported : SendFlowException" (i.e., drop the redundant constructor) to satisfy detekt and simplify the declaration.
🤖 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 43357-43360: The discriminant mapping for the Rust enum variants
is off: change the numeric tag used to construct
SendFlowException.SilentPaymentNotSupported from 15 to 14 so it matches the Rust
discriminant order (so SilentPaymentNotSupported is handled without attempting
to read a string via FfiConverterString.read), and update the subsequent variant
tags accordingly (the block constructing
SendFlowException.UnableToGetFeeDetails,
SendFlowException.SilentPaymentNotSupported, and the following variant mappings
around the same switch/when that includes the code near FfiConverterString.read)
so all variant indices align with the Rust definition.
In `@rust/crates/cove-types/src/address.rs`:
- Around line 539-560: Run rustfmt on the updated tests and reformat the long
assert_eq! statements in the test_silent_payment_address_from_string and the
surrounding assertions (references: AddressWithNetwork::try_new,
Address::from_string, test_silent_payment_address_from_string) so lines fit the
project's rustfmt rules (wrap long arguments/expected values across lines).
After formatting, re-run cargo fmt / rustfmt to ensure CI passes.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 43296-43297: The class SilentPaymentNotSupported currently
declares an empty primary constructor; remove the empty parentheses and declare
it as "class SilentPaymentNotSupported : SendFlowException" (i.e., drop the
redundant constructor) to satisfy detekt and simplify the declaration.
🪄 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: fe7e40a3-7a7b-449b-87db-f970b50a5123
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/Flows/SendFlow/SendFlowPresenter.swiftrust/crates/cove-types/src/address.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/error.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rust/src/manager/send_flow_manager/error.rs
- ios/Cove/Flows/SendFlow/SendFlowPresenter.swift
- rust/src/manager/send_flow_manager.rs
- android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.kt
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
6414386 to
0291efb
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
rust/src/multi_format.rs:113-120 does not handle the new AddressError::SilentPaymentNotSupported. The shared scanner still goes through MultiFormat::try_from_string() (rust/src/qr_scanner.rs:218, :276, :399), so an sp1... QR now fails as a generic parse error before Send Flow reaches handle_scan_code_changed() (rust/src/manager/send_flow_manager.rs:1493-1497). On iOS that lands in the generic QR error path in ios/Cove/Flows/SendFlow/SetAmountScreen/QrCodeAddressView.swift:99-111, and on Android Send Flow only accepts MultiFormat. Address in android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowContainer.kt:289-307. The new unsupported-address copy therefore works for manual entry, but not for scanned silent-payment addresses.
Please thread the new error through the shared scanner path, or otherwise let Send Flow see the raw scanned string.rust/crates/cove-types/src/address.rs:217-225 uses bitcoin::bech32::decode() as the validator.
That API accepts either Bech32 or Bech32m checksums, so a checksummed-but-non-Bech32m sp1.../tsp1... string will be reported as SilentPaymentNotSupported instead of InvalidAddress. Silent payment detection needs a Bech32m-only checksum check here.I
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
0291efb to
79b59d8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.kt (1)
494-498: Silence detekt SwallowedException (or log the exception).detekt flags this catch because the bound
eis never used. Since the exception type itself carries the semantic meaning, you can either drop the binding or log it for diagnostics.♻️ Proposed fix
- } catch (e: MultiQrException.SilentPaymentNotSupported) { + } catch (_: MultiQrException.SilentPaymentNotSupported) { onError( "Unsupported Address", "Sending to silent payment addresses (sp1...) is not yet supported. Support is coming soon.", )🤖 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/QrCodeScanView.kt` around lines 494 - 498, The catch block for MultiQrException.SilentPaymentNotSupported in QrCodeScanView.kt binds the exception to e but never uses it, triggering detekt's SwallowedException; fix this by either removing the binding (change to catch ( _: MultiQrException.SilentPaymentNotSupported ) { ... }) or log the exception for diagnostics before calling onError (e.g., use your existing logger or Android Log.e/Timber.e to record e), keeping the rest of the onError call unchanged so the user-facing message remains the same.rust/src/qr_scanner.rs (1)
63-64: LGTM — new variant andmap_multi_format_errcentralize the SP translation correctly.All three sites (
parse_ursingle-part, BBQrUnicodeText/Json, andscan_firstplain-string) now funnel throughmap_multi_format_err, so a silent-payment address arriving via any transport surfaces asMultiQrError::SilentPaymentNotSupportedrather than a genericParseError.Optional follow-up (not part of this PR): other structured
MultiFormatErrorvariants —PsbtNotSigned,TaprootNotSupported,UnsupportedNetworkAddress,InvalidSeedQr— are still collapsed intoMultiQrError::ParseError(other.to_string())and lose their identity at the FFI boundary. Since this function is now the single mapping point, it would be a good place to propagate those as distinct variants the next time the UI needs to branch on them.Also applies to: 220-220, 278-278, 283-289, 408-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/qr_scanner.rs` around lines 63 - 64, The mapping function map_multi_format_err currently converts MultiFormatError variants into a generic MultiQrError::ParseError except for SilentPaymentNotSupported; update map_multi_format_err to map additional structured MultiFormatError variants (PsbtNotSigned, TaprootNotSupported, UnsupportedNetworkAddress, InvalidSeedQr) to new distinct MultiQrError variants (add those variants to the MultiQrError enum if missing) so the FFI boundary preserves their identity; change the match arm in map_multi_format_err to return the specific MultiQrError::<Variant> for each corresponding MultiFormatError::<Variant> and keep ParseError as the fallback for truly unknown cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.kt`:
- Around line 494-498: The catch block for
MultiQrException.SilentPaymentNotSupported in QrCodeScanView.kt binds the
exception to e but never uses it, triggering detekt's SwallowedException; fix
this by either removing the binding (change to catch ( _:
MultiQrException.SilentPaymentNotSupported ) { ... }) or log the exception for
diagnostics before calling onError (e.g., use your existing logger or Android
Log.e/Timber.e to record e), keeping the rest of the onError call unchanged so
the user-facing message remains the same.
In `@rust/src/qr_scanner.rs`:
- Around line 63-64: The mapping function map_multi_format_err currently
converts MultiFormatError variants into a generic MultiQrError::ParseError
except for SilentPaymentNotSupported; update map_multi_format_err to map
additional structured MultiFormatError variants (PsbtNotSigned,
TaprootNotSupported, UnsupportedNetworkAddress, InvalidSeedQr) to new distinct
MultiQrError variants (add those variants to the MultiQrError enum if missing)
so the FFI boundary preserves their identity; change the match arm in
map_multi_format_err to return the specific MultiQrError::<Variant> for each
corresponding MultiFormatError::<Variant> and keep ParseError as the fallback
for truly unknown cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ff249e9-7c29-4b33-a9e8-f86f09d48e8c
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (11)
android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/Flows/SendFlow/SendFlowPresenter.swiftios/Cove/Flows/SendFlow/SetAmountScreen/QrCodeAddressView.swiftrust/crates/cove-types/src/address.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/error.rsrust/src/multi_format.rsrust/src/qr_scanner.rs
✅ Files skipped from review due to trivial changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- rust/src/manager/send_flow_manager/error.rs
- ios/Cove/Flows/SendFlow/SendFlowPresenter.swift
- rust/src/manager/send_flow_manager.rs
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
looks good, but need some small changes
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
79b59d8 to
8102b8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manager/send_flow_manager.rs`:
- Around line 422-426: The code currently treats any successful
AddressWithNetwork::try_new(&entering) result as InvalidAddress; instead, in
send_flow_manager.rs where you handle let entering =
self.state.lock().entering_address.clone() and match
AddressWithNetwork::try_new(&entering), add a network equality check on the
Ok(path) branch: compare the returned AddressWithNetwork's network (e.g.,
addr.network or AddressWithNetwork::network()) against the expected network for
the current flow (derive expected network from the state or wallet context used
elsewhere in SendFlowManager), and if they differ return the appropriate
SendFlowError variant for wrong network (use the existing SendFlowError variant
for wrong-network addresses) otherwise proceed as a valid address; keep the
Err(e) branch using SendFlowError::from_address_error(e, entering) unchanged.
🪄 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: b7d7f907-7f45-422d-b32b-4c2a154dba0d
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (11)
android/app/src/main/java/org/bitcoinppl/cove/QrCodeScanView.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowPresenter.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/Flows/SendFlow/SendFlowPresenter.swiftios/Cove/Flows/SendFlow/SetAmountScreen/QrCodeAddressView.swiftrust/crates/cove-types/src/address.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/error.rsrust/src/multi_format.rsrust/src/qr_scanner.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- ios/Cove/Flows/SendFlow/SetAmountScreen/QrCodeAddressView.swift
- rust/src/manager/send_flow_manager/error.rs
- ios/Cove/Flows/SendFlow/SendFlowPresenter.swift
- rust/src/multi_format.rs
Pasting or scanning a BIP352 silent payment address (sp1... / tsp1...) into the send field surfaces a generic "Invalid Address" error today, which is confusing because the string looks like valid bech32m. Detect the SP prefixes in both AddressWithNetwork::try_new and Address::from_string and raise a dedicated SilentPaymentNotSupported error. Validation requires a successful bech32m-only decode (via CheckedHrpstring::new::<Bech32m>) with the "sp" or "tsp" HRP, so truncated strings and strings with a bech32 (not bech32m) checksum fall through to the existing InvalidAddress path. The QR scanner shares MultiFormat::try_from_string, so propagate the new error through MultiFormatError and MultiQrError. Both iOS QrCodeAddressView and Android QrCodeScanView catch the typed error and show the same "Unsupported Address" alert as manual entry, including for bitcoin:sp1... URIs and scanned QR codes. Regenerated UniFFI bindings for the new error variants on both iOS and Android. Refs bitcoinppl#617
guptapratykshh
force-pushed
the
fix/617-silent-payment-address-detection
branch
from
8102b8e to
6fc6ce1
Compare
2 participants
Fixes #617
blocks sending to silent payment addresses with a clear "not yet supported" message instead of a confusing "Invalid Address" error.
Summary by CodeRabbit
New Features
Bug Fixes