Add coin control for Monero#9396
Merged
Merged
Conversation
The flow started at false regardless of the persisted preference. On cold start SendBitcoinViewModel collected that false into a property delegated to the preference setter, silently disabling UTXO expert mode.
The fixed 64dp cell height left no room for the two-line value once RowUniversal's 16dp vertical padding was applied, cutting off the fiat line. Let the cell wrap its content instead.
Mirrors the Bitcoin UTXO expert mode, keyed by key image instead of txHash-outputIndex. Adds unspentOutputs and selected outputs to ISendMoneroAdapter, a Monero UTXO selection screen reusing the Bitcoin list composables, and a send settings page with the shared expert mode switch gating the UTxOs cell. Send is blocked until the wallet is synced, with the reason shown as the disabled button title, and selections referencing outputs that disappeared during sync are pruned automatically. Fee estimation errors surface as a caution; connectivity is checked before estimating because the native fee call blocks un-cancellably while offline.
balanceStateUpdatedFlowable was backed by the subject that fires on balance changes and vice versa, so collectors of either signal were notified of the wrong event.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughChangesMonero coin control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLong-lived Flow collectors added in this PR have no error handling, so a single upstream error silently kills state updates for the rest of the session. Each of these is a new/changed persistent
collect/collectWithin alaunchblock with nocatchoperator or try-catch; the RxJava-bridged balance flowables are the most likely to actually error.
walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt#L119-126: wrapmerge(balanceAdapter.balanceStateUpdatedFlowable.asFlow(), balanceAdapter.balanceUpdatedFlowable.asFlow())with.catch { }(or try-catch insidecollect) so a Flowable error doesn't stop balance/UTXO pruning permanently.walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.kt#L36-43: add.catch { }aroundconnectivityManager.networkAvailabilityFlowbefore.collect, or wrap the collector body in try-catch.walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt#L112-118: add.catch { }aroundlocalStorage.utxoExpertModeEnabledFlowbefore.collect.walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeViewModel.kt#L41-45: replace the Zcash-SDKcollectWithextension withviewModelScope.launch { xRateService.getRateFlow(token.coin.uid).catch { }.collect { ... } }, matching the pattern already used inSendMoneroViewModel, and add error handling.🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt` at line 1, Add error handling to every long-lived Flow collector identified in the Monero send flow: apply catch before collecting localStorage.utxoExpertModeEnabledFlow and the merged balanceAdapter Flowables in SendMoneroViewModel, and connectivityManager.networkAvailabilityFlow in SendMoneroFeeService. In MoneroUtxoExpertModeViewModel, replace the Zcash-SDK collectWith usage with a viewModelScope.launch collecting xRateService.getRateFlow(token.coin.uid) after catch, while preserving the existing collector bodies and state-update behavior.Source: Coding guidelines
🧹 Nitpick comments (3)
walletkit/src/main/java/io/horizontalsystems/walletkit/core/adapters/MoneroAdapter.kt (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMigrate legacy RxJava
Flowableto Kotlin Coroutines.As per coding guidelines, do not use RxJava for reactive programming; migrate legacy types to
Flow. Consider updating theIBalanceAdapterinterface to replace these subjects and flowables withSharedFloworStateFlow.🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/core/adapters/MoneroAdapter.kt` around lines 57 - 60, Replace the RxJava Flowable-based balance update API in MoneroAdapter, including balanceUpdatedFlowable and balanceStateUpdatedFlowable, with Kotlin Coroutines Flow using SharedFlow or StateFlow as appropriate. Update the corresponding IBalanceAdapter contract and migrate the underlying subjects and consumers so the existing balance-update behavior is preserved without RxJava types.Source: Coding guidelines
walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeViewModel.kt (1)
26-27: 🚀 Performance & Scalability | 🔵 TrivialConsider a
SetforselectedUnspentOutputsto avoid O(n·m) lookups.
selectedUnspentOutputsis aList<String>, and.contains()is called against it inside loops overunspentOutputsinsetAvailableBalanceInfo,setUnspentOutputViewItems, andcustomOutputs. For wallets with many outputs (a realistic scenario in Monero due to output splitting), this is quadratic. ALinkedHashSet<String>(or plainSet) preserves the needed semantics with O(1) lookups.♻️ Suggested change
- private var selectedUnspentOutputs = listOf<String>() + private var selectedUnspentOutputs = setOf<String>()(Adjust
+/filter/onUnspentOutputClickedaccordingly to operate on aSet.)Also applies to: 34-35, 57-70, 90-94
🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeViewModel.kt` around lines 26 - 27, Change selectedUnspentOutputs from a List<String> to a Set<String>, preferably LinkedHashSet to preserve insertion order, and update the related +, filter, and onUnspentOutputClicked logic to use set operations. Ensure contains checks in setAvailableBalanceInfo, setUnspentOutputViewItems, and customOutputs use O(1) lookups while preserving selection and deselection behavior.walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.kt (1)
89-89: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify
Log.ehere can't leak sensitive tx details in release builds.Per path instructions, any
Log.e/Log.d/etc. that can contain sensitive material must be guarded so it doesn't ship unguarded.ehere wrapsadapter.estimateFee(amount, address.hex, memo)failures, so its message/stacktrace could echo address/amount/memo depending on the native error surface. Recommend gating withBuildConfig.DEBUG(or scrubbing the message) to be safe.As per path instructions, "Logging: any Log.d/v/i/w/e or println that can contain secret material must be guarded by BuildConfig.DEBUG and verified not to ship."
🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.kt` at line 89, Guard the Log.e call in the fee-estimation retry handling around adapter.estimateFee with BuildConfig.DEBUG so exception messages and stack traces cannot ship in release builds. Preserve the existing retry behavior and debug failure context without logging this potentially sensitive exception unguarded.Source: Path instructions
🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/core/Interfaces.kt`:
- Around line 506-509: The synchronous unspentOutputs property performs JNI and
transaction-history I/O on the caller’s thread. In
walletkit/src/main/java/io/horizontalsystems/walletkit/core/Interfaces.kt:506-509,
replace it with suspend fun getUnspentOutputs(): List<MoneroUnspentOutput>; in
walletkit/src/main/java/io/horizontalsystems/walletkit/core/adapters/MoneroAdapter.kt:111-125,
implement getUnspentOutputs and execute the existing retrieval and
timestamp-mapping work inside withContext(Dispatchers.IO).
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/amount/SendAmountService.kt`:
- Around line 73-80: Update SendAmountService.setAvailableBalance to be a
suspend function and wrap the availableBalance assignment, validateAmount(), and
emitState() calls in mutex.withLock, preserving their existing order and
behavior.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.kt`:
- Around line 21-24: Synchronize all read, cancellation, and reassignment
operations on estimateFeeJob across refreshFeeAndEmitState and every existing
trigger, including the connectivity networkAvailabilityFlow collector and
setAmount/setAddress/setMemo callers. Make the job-management sequence atomic so
concurrent refreshes cannot lose cancellation or leave multiple estimate
coroutines running, while preserving the existing latest-request behavior.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt`:
- Around line 107-126: Update the persistent collectors in the ViewModel
initialization around utxoExpertModeEnabledFlow and the merged balanceAdapter
flows to handle upstream exceptions with the established flow error-handling
approach. Ensure errors do not silently terminate these lifetime collectors,
while preserving the existing state updates for successful emissions.
---
Outside diff comments:
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.kt`:
- Line 1: Add error handling to every long-lived Flow collector identified in
the Monero send flow: apply catch before collecting
localStorage.utxoExpertModeEnabledFlow and the merged balanceAdapter Flowables
in SendMoneroViewModel, and connectivityManager.networkAvailabilityFlow in
SendMoneroFeeService. In MoneroUtxoExpertModeViewModel, replace the Zcash-SDK
collectWith usage with a viewModelScope.launch collecting
xRateService.getRateFlow(token.coin.uid) after catch, while preserving the
existing collector bodies and state-update behavior.
---
Nitpick comments:
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/core/adapters/MoneroAdapter.kt`:
- Around line 57-60: Replace the RxJava Flowable-based balance update API in
MoneroAdapter, including balanceUpdatedFlowable and balanceStateUpdatedFlowable,
with Kotlin Coroutines Flow using SharedFlow or StateFlow as appropriate. Update
the corresponding IBalanceAdapter contract and migrate the underlying subjects
and consumers so the existing balance-update behavior is preserved without
RxJava types.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.kt`:
- Line 89: Guard the Log.e call in the fee-estimation retry handling around
adapter.estimateFee with BuildConfig.DEBUG so exception messages and stack
traces cannot ship in release builds. Preserve the existing retry behavior and
debug failure context without logging this potentially sensitive exception
unguarded.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeViewModel.kt`:
- Around line 26-27: Change selectedUnspentOutputs from a List<String> to a
Set<String>, preferably LinkedHashSet to preserve insertion order, and update
the related +, filter, and onUnspentOutputClicked logic to use set operations.
Ensure contains checks in setAvailableBalanceInfo, setUnspentOutputViewItems,
and customOutputs use O(1) lookups while preserving selection and deselection
behavior.
🪄 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 Plus
Run ID: 84d82854-8686-4b27-bde7-88323346c3a5
📒 Files selected for processing (14)
gradle/libs.versions.tomlwalletkit/src/main/java/io/horizontalsystems/walletkit/core/Interfaces.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/core/adapters/MoneroAdapter.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/LocalStorageManager.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/amount/SendAmountService.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/bitcoin/SendBitcoinScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/bitcoin/utxoexpert/UtxoExpertModeScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroAdvancedSettingsScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroFeeService.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroModule.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/SendMoneroViewModel.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/send/monero/utxoexpert/MoneroUtxoExpertModeViewModel.kt
Comment on lines
+73
to
+80
| fun setAvailableBalance(availableBalance: BigDecimal) { | ||
| this.availableBalance = availableBalance | ||
|
|
||
| validateAmount() | ||
|
|
||
| emitState() | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use mutex.withLock for thread-safe state updates.
As per coding guidelines for Services (**/*Service.kt), use mutex.withLock for thread-safe state updates. Since this method modifies the internal availableBalance state and triggers a new state emission, it should be protected against concurrent modifications.
Consider changing this method to a suspend fun and wrapping the internal logic in a mutex.withLock { ... } block to comply with the architectural rules.
🤖 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
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/amount/SendAmountService.kt`
around lines 73 - 80, Update SendAmountService.setAvailableBalance to be a
suspend function and wrap the availableBalance assignment, validateAmount(), and
emitState() calls in mutex.withLock, preserving their existing order and
behavior.
Source: Coding guidelines
rafaelekol
previously approved these changes
Jul 19, 2026
The synchronous unspentOutputs property ran JNI under the wallet2 mutex, which the background refresh can hold for seconds; the UTXO selection view model read it on the main thread during construction. Replace it with suspend getUnspentOutputs() on Dispatchers.IO and load the selection screen list asynchronously.
setAvailableBalance is called from background collectors while setAmount arrives from the UI thread; all three mutators share the same fields, so synchronize them together rather than locking one.
refreshFeeAndEmitState is triggered concurrently from the view model collectors and the connectivity collector; unsynchronized cancel and reassignment of estimateFeeJob could leave two estimate coroutines running. One monitor across the setters and the refresh keeps a single job alive - the latest - and gives the job happens-before visibility of the input fields.
handleBalanceAdapterUpdate reaches JNI via getUnspentOutputs; a single native error would silently kill the lifetime collector and freeze sync gating and selection pruning for the rest of the screen. Guard the body per emission and the upstream with catch, rethrowing cancellation.
rafaelekol
approved these changes
Jul 20, 2026
abdrasulov
approved these changes
Jul 20, 2026
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
3 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.
#9216
Summary by CodeRabbit
New Features
Bug Fixes