Add payjoin ohttp relay settings with cloud backup support - #842
Add payjoin ohttp relay settings with cloud backup support#842Sandipmandal25 wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds HTTPS-only OHTTP relay configuration with normalized persistence, backup export/import support, and Android and iOS settings screens accessible through a new settings route. ChangesOHTTP relay configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RelaySettingsScreen
participant GlobalConfigTable
participant BackupService
User->>RelaySettingsScreen: Enter and save relay URL
RelaySettingsScreen->>GlobalConfigTable: Validate and persist URL
GlobalConfigTable-->>RelaySettingsScreen: Return normalized URL or error
BackupService->>GlobalConfigTable: Read configured relay URL
GlobalConfigTable-->>BackupService: Return relay setting for backup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a PayJoin/OHTTP relay settings screen on iOS and Android, backed by a new Rust storage layer that validates HTTPS-only relay URLs, stores them as canonical JSON, re-validates on read, and includes them in cloud backup export/import. It also migrates gracefully from an older single-URL key to a multi-URL list.
Confidence Score: 5/5Safe to merge — all changed paths are well-tested, backup import gracefully skips bad data, and error handling is correctly differentiated in both UI layers. The relay validation, storage, migration, and backup integration all behave correctly for every realistic code path. The one inconsistency found (whitespace-only input to set_ohttp_relay_urls leaves the legacy OhttpRelayUrl key alive) requires the single-URL API to have been used beforehand, which no current UI code does — it has no practical impact today. Files Needing Attention: rust/src/database/global_config.rs — the canonical-empty branch of set_ohttp_relay_urls should also delete OhttpRelayUrl for symmetry with the empty-input branch. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS / Android UI
participant FFI as UniFFI (GlobalConfigTable)
participant DB as redb Database
participant PJ as payjoin::ohttp_relays()
participant Relay as OHTTP Relay
UI->>FFI: setOhttpRelayUrls(urls)
FFI->>FFI: "parse_ohttp_relay_url (HTTPS-only validate & normalize)"
FFI->>DB: set OhttpRelayUrls (JSON array)
DB-->>FFI: Ok / Err
FFI-->>UI: canonical URLs / InvalidOhttpRelayUrl / storage error
UI->>FFI: ohttpRelayUrls()
FFI->>DB: get OhttpRelayUrls
alt OhttpRelayUrls present
DB-->>FFI: JSON array
FFI->>FFI: re-validate each URL
else migrate from single key
FFI->>DB: get OhttpRelayUrl
DB-->>FFI: single URL or None
end
FFI-->>UI: Vec String
Note over PJ: On PayJoin send
PJ->>FFI: ohttpRelayUrls()
alt custom relays set
FFI-->>PJ: custom list (stored order)
else no custom relays
FFI-->>PJ: empty list, use shuffled OHTTP_RELAYS
end
PJ->>Relay: OHTTP request (first relay that succeeds)
Reviews (5): Last reviewed commit: "Redesign OHTTP relay settings with card ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
android/detekt-baseline.xml (1)
1178-1178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-evaluate baseline suppression.
If you choose to log the exceptions in
OhttpRelaySettingsScreen.kt(as suggested in the review for that file), thisSwallowedExceptionsuppression—as well as theTooGenericExceptionCaughtsuppression on line 1237—will no longer be necessary and should be removed to keep the baseline clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/detekt-baseline.xml` at line 1178, Remove the SwallowedException baseline entry for OhttpRelaySettingsScreen.kt and also remove its associated TooGenericExceptionCaught suppression. Only make these removals if the exceptions in OhttpRelaySettingsScreen.kt are now logged, leaving unrelated baseline suppressions unchanged.ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift (1)
77-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove ineffective
isSavingstate and dismiss keyboard on reset.The
isSavingstate is set totrueandfalsesynchronously within a single main-thread execution cycle, because the FFI callconfig.setOhttpRelayUrlis synchronous. SwiftUI coalesces these state changes, meaning the UI never actually re-renders withisSaving = true.Since the save operation completes practically instantly and the button is immediately disabled by the updated URL equality check anyway, the
isSavingstate is unnecessary and can be removed entirely.Additionally, consider dismissing the keyboard in the
reset()function for a consistent user experience.♻️ Proposed refactor
Note: You can also safely remove
@State private var isSaving = false(line 8) and theisSaving ||check from the button's.disabled()modifier (line 58).- private func save() { - guard !isSaving else { return } - - let inputToSave = input - - isSaving = true - isInputFocused = false + private func save() { + let inputToSave = input + isInputFocused = false do { let normalized = try config.setOhttpRelayUrl(url: inputToSave) input = normalized ?? "" Task { `@MainActor` in await dismissAllPopups() try? await Task.sleep(for: .milliseconds(250)) await MiddlePopup(state: .success("Relay saved successfully")) .dismissAfter(2) .present() } } catch DatabaseError.GlobalConfig(.InvalidOhttpRelayUrl) { showInvalidUrlAlert = true } catch { showUpdateFailedAlert = true } - - isSaving = false } private func reset() { + isInputFocused = false do { try config.clearOhttpRelayUrl() input = "" } catch { showUpdateFailedAlert = true } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift` around lines 77 - 113, Remove the isSaving state and its guard, assignments, and button .disabled() dependency from the settings view, since save() is synchronous and URL equality already prevents duplicate saves. Update reset() to dismiss the keyboard by clearing isInputFocused consistently with save().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OhttpRelaySettingsScreen.kt`:
- Line 66: Update the Settings screen’s config initialization around Database
and globalConfig so both the originating Database instance and GlobalConfigTable
are retained and closed via a Compose DisposableEffect when the screen leaves
composition; call close() before releasing references, using the generated
AutoCloseable APIs confirmed by the binding definitions.
In `@rust/src/database/global_config.rs`:
- Around line 405-406: Update the URL parsing error conversion in the
surrounding global configuration flow to use cove_util::ResultExt::map_err_str
instead of the manual closure, ensuring ResultExt is imported if necessary and
preserving GlobalConfigTableError::InvalidOhttpRelayUrl as the error variant.
---
Nitpick comments:
In `@android/detekt-baseline.xml`:
- Line 1178: Remove the SwallowedException baseline entry for
OhttpRelaySettingsScreen.kt and also remove its associated
TooGenericExceptionCaught suppression. Only make these removals if the
exceptions in OhttpRelaySettingsScreen.kt are now logged, leaving unrelated
baseline suppressions unchanged.
In `@ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift`:
- Around line 77-113: Remove the isSaving state and its guard, assignments, and
button .disabled() dependency from the settings view, since save() is
synchronous and URL equality already prevents duplicate saves. Update reset() to
dismiss the keyboard by clearing isInputFocused consistently with save().
🪄 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: 231acc33-6ffd-4d97-87c6-53aaef4170d1
⛔ Files ignored due to path filters (2)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktis excluded by!android/app/src/main/java/org/bitcoinppl/cove_core/**ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**,!ios/CoveCore/Sources/CoveCore/generated/**
📒 Files selected for processing (14)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OhttpRelaySettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SettingsContainer.ktandroid/app/src/main/res/values/strings.xmlandroid/detekt-baseline.xmlios/Cove/Flows/SettingsFlow/MainSettingsSections.swiftios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swiftios/Cove/Flows/SettingsFlow/SettingsContainer.swiftrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/backup/verify.rsrust/src/database/global_config.rsrust/src/router.rs
e2f5d25 to
ebb08b3
Compare
|
@coderabbitai re review |
|
The Comment images show the new iOS PayJoin Relay settings flow. The first screen contains the description of OHTTP relay privacy, the default relays
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rust/src/database/global_config.rs (1)
389-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a blank line after the setup binding.
As per coding guidelines, add blank lines between logical steps in function bodies, specifically after setup/result bindings and before new control flow.
♻️ Proposed fix
let parsed = url::Url::parse(&url).ok()?; + if parsed.scheme() != "https" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/database/global_config.rs` around lines 389 - 390, Insert a blank line between the Url parsing binding and the subsequent scheme-validation conditional in the surrounding function, keeping the existing parsing and control-flow logic unchanged.Source: Coding guidelines
ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift (2)
105-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDismiss keyboard on reset.
Consider dismissing the keyboard when resetting to the default relays, matching the user experience in the
save()method.♻️ Proposed refactor
private func reset() { + isInputFocused = false do { try config.clearOhttpRelayUrl() input = ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift` around lines 105 - 112, Update the reset() method to dismiss the keyboard when resetting succeeds, matching the keyboard dismissal behavior already implemented in save(). Keep the existing input reset and error-alert handling unchanged.
4-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid double instantiation of the database configuration handle.
The
Database().globalConfig()handle is currently instantiated twice: once as an inline property and once inside theinit()method. You can cleanly resolve this by declaring the property type and assigning it entirely withininit().♻️ Proposed refactor
- private let config = Database().globalConfig() + private let config: GlobalConfigTable `@State` private var input: String `@State` private var isSaving = false `@State` private var showInvalidUrlAlert = false `@State` private var showUpdateFailedAlert = false `@FocusState` private var isInputFocused: Bool init() { - let config = Database().globalConfig() - _input = State(initialValue: config.ohttpRelayUrl() ?? "") + let dbConfig = Database().globalConfig() + self.config = dbConfig + _input = State(initialValue: dbConfig.ohttpRelayUrl() ?? "") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift` around lines 4 - 16, Update OhttpRelaySettingsView so the globalConfig handle is initialized only once inside init(). Declare config with an explicit type without an inline initializer, assign it in init(), and use that initialized config to derive _input while preserving the existing state behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swift`:
- Around line 105-112: Update the reset() method to dismiss the keyboard when
resetting succeeds, matching the keyboard dismissal behavior already implemented
in save(). Keep the existing input reset and error-alert handling unchanged.
- Around line 4-16: Update OhttpRelaySettingsView so the globalConfig handle is
initialized only once inside init(). Declare config with an explicit type
without an inline initializer, assign it in init(), and use that initialized
config to derive _input while preserving the existing state behavior.
In `@rust/src/database/global_config.rs`:
- Around line 389-390: Insert a blank line between the Url parsing binding and
the subsequent scheme-validation conditional in the surrounding function,
keeping the existing parsing and control-flow logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41de6019-aff4-4d9f-9414-401780258197
⛔ Files ignored due to path filters (2)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktis excluded by!android/app/src/main/java/org/bitcoinppl/cove_core/**ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**,!ios/CoveCore/Sources/CoveCore/generated/**
📒 Files selected for processing (14)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/OhttpRelaySettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SettingsContainer.ktandroid/app/src/main/res/values/strings.xmlandroid/detekt-baseline.xmlios/Cove/Flows/SettingsFlow/MainSettingsSections.swiftios/Cove/Flows/SettingsFlow/OhttpRelaySettingsView.swiftios/Cove/Flows/SettingsFlow/SettingsContainer.swiftrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/backup/verify.rsrust/src/database/global_config.rsrust/src/router.rs
|
@Sandipmandal25 i like the design it looks good and fits with the rest of the app, do you have what it looks like on android? I'm just wondering if we should start a new sub section called "Advanced" and put that in there, or inside node? How do other apps do this? |
i will attach the photos of android too and i think advanced makes more sense than node like the other apps like sparrow and nunchuk do the same block explorer could move there too! |
|
Where does sparrow and bull put it? |
bull bitcoin does it automatically without a user setting It has an advanced options screen for tor proxy and custom electrum server but the relay is purely non configurable and sparrow doesnt have it v1 only. so so cove would be first. but advanced feels right |
|
@Sandipmandal25 should it be configureable at all? can they use multiple relays? |
ebb08b3 to
fd9aed1
Compare
yes, mainly for self hosted relays. multiple isnt needed since the defaults already rotate.The defaults already rotate across 3 randomly |
|
@Sandipmandal25 the rotation does that happen in the PDK code or ours? And are we upgraded to PDK 1.0? |
rotation is in our code not the pdk we shuffle the list with rand on each call. and were on 0.25.0 not 1.0 yet. |
49091e1 to
3f495bc
Compare
|
|
|
@praveenperera should we have a cap for adding max relay? or its fine? |
|
@Sandipmandal25 i think we can just leave it, this looks good, how it look on android? |
|
|
@Sandipmandal25 looks alright but ios looks much nicer |
Yeah agree ios looks better should i try to match ios design more closely? |
|
can you do some research into what idiomatic android material design 3 should look like for a screen like this? |
Sure i will do that and update accordingly! |
d4c23b6 to
2b52bc5
Compare
|
|
2b52bc5 to
c96ff43
Compare
2 participants
Summary
Adds a new PayJoin Relay settings screen on iOS and Android, allowing users to configure a custom OHTTP relay URL for PayJoin sends. When no custom relay is configured, Cove continues using the built-in public relays (
relay.payjoin.org,ohttp.achow101.com,pj.bobspacebkk.com).Changes
httpsonly)Checklist
UI
Summary by CodeRabbit