feat: add bulk UTXO lock control to Transaction Details (#661)#693
feat: add bulk UTXO lock control to Transaction Details (#661)#693gitsofaryan wants to merge 19 commits intobitcoinppl:masterfrom
Conversation
- Implement 3-state aggregate lock logic (Unlocked, Locked, Mixed) in WalletActor - Add bulk UTXO lock control to Transaction Details Top Bar - Include snackbar feedback and animated transitions for better UX - Exclude already-spent and external outputs from the lock scope - Refresh lock state on screen load and pull-to-refresh
|
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:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a bulk UTXO lock/unlock control to the Transaction Details screen, scoped to wallet-owned unspent outputs of a specific transaction. The Rust backend introduces Confidence Score: 5/5Safe to merge — all findings are P2 style/efficiency suggestions with no correctness or data-integrity impact. The core toggle logic is correct, UTXO scoping via list_unspent() is correct, and FFI wiring is straightforward. The four findings are unused variable, polling-loop staleness, a redundant BDK scan, and unnecessary no-op DB records — none affect correctness or data integrity. No files require special attention for merge safety. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| IconButton(onClick = { | ||
| scope.launch { | ||
| try { | ||
| val previousState = lockState |
There was a problem hiding this comment.
previousState variable
previousState is assigned from lockState but is never read afterward — the snackbar message is derived from newState. This will generate a Kotlin compiler warning and can be removed.
| val previousState = lockState | |
| try { | |
| manager.rust.toggleTransactionLock(txId = txId) |
| let mut record = table.get(key.clone())?.map(|r| r.value()).unwrap_or_else(|| { | ||
| Record::new(OutputRecord { | ||
| ref_: outpoint, | ||
| label: None, | ||
| spendable: true, | ||
| }) | ||
| }); |
There was a problem hiding this comment.
When set_outputs_spendable is called with spendable = true (unlock path) and an outpoint has no existing record, a new OutputRecord is created with spendable: true — which is the default. This writes a semantically empty record to the database, adding unnecessary cloud-backup churn. A small guard would avoid the write:
let existing = table.get(key.clone())?.map(|r| r.value());
let mut record = match existing {
Some(r) => r,
None => {
if spendable { continue; }
Record::new(OutputRecord { ref_: outpoint, label: None, spendable: true })
}
};- Implement 3-state aggregate lock logic (Unlocked, Locked, Mixed) in WalletActor - Add bulk UTXO lock control to Transaction Details Top Bar - Include snackbar feedback and animated transitions for better UX - Exclude already-spent and external outputs from the lock scope - Refresh lock state on screen load and pull-to-refresh
| IconButton(onClick = { | ||
| scope.launch { | ||
| try { | ||
| <<<<<<< HEAD |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt`:
- Around line 349-399: Add a local boolean "isToggling" state and guard the
IconButton so it is disabled while a toggle is in flight: set isToggling = true
immediately before calling manager.rust.toggleTransactionLock(txId = txId) in
the scope.launch block and ensure you set isToggling = false in a finally block
after the network call (both on success and in the catch). Update the IconButton
to pass enabled = !isToggling (or return early from onClick when isToggling),
keep updating lockState from manager.rust.transactionLockState(txId = txId) and
showing snackbars as before; optionally show a small progress indicator when
isToggling is true.
In `@rust/src/database/wallet_data/label.rs`:
- Line 367: The changed region around the `if spendable {` hunk fails rustfmt;
run rustfmt (e.g., cargo fmt) and reformat the block so it conforms to the
project's formatting rules, then update the PR with the formatted code ensuring
the `if spendable {` block and surrounding lines match rustfmt's output.
- Around line 350-392: set_outputs_spendable currently leaves synthetic empty
OutputRecord rows (label: None) behind when unlocking outputs; update it so when
spendable is true and an existing record has label == None you remove the row
instead of updating/inserting it. In the loop inside set_outputs_spendable
(using OutPointKey, OUTPUT_TABLE, Record/OutputRecord and table variable) check
for Some(record) with record.item.label.is_none() && spendable == true and call
table.remove(key) (instead of modifying/inserting), preserving the existing
behavior that continues when there is no record and spendable is true.
- Around line 212-221: The locked_outpoints function currently swallows
iteration errors via filter_map(|r| r.ok()), which can hide unreadable/corrupt
record errors; change the iteration to propagate errors so callers get a
Result::Err on read failures: after read_table(OUTPUT_TABLE)? call table.iter()?
and instead of filter_map(|r| r.ok()) use a fallible mapping that returns Err
when any r is Err (e.g., map(|r| r?) or collect into Result) so the function
returns the first iteration/read error rather than silently dropping records;
keep the existing filtering of !r.item.spendable and mapping to r.item.ref_ but
ensure the final collect produces Result<Vec<bitcoin::OutPoint>, Error> from
locked_outpoints.
In `@rust/src/manager/wallet_manager.rs`:
- Around line 591-596: The current WalletManager::toggle_transaction_lock uses
call!(self.actor.toggle_transaction_lock(Arc::unwrap_or_clone(tx_id))).await but
ignores the inner Result returned by WalletActor::toggle_transaction_lock, so DB
write errors are lost; change the flow to await the call!, map the outer call
failure to Error as you already do, then inspect and propagate the inner Result
from WalletActor::toggle_transaction_lock (e.g. use ? on that inner Result or
map its Err into your uniffi Error) so any actor-level application error is
returned to the caller instead of being treated as success; keep the
Arc::unwrap_or_clone(tx_id) and the call! invocation but ensure both the call!
failure and the actor method’s Result are converted into the method’s Result<(),
Error> and returned.
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 322-346: The hunk in the async method toggle_transaction_lock has
formatting issues flagged by rustfmt; run rustfmt on the file or reformat the
function body to match project style (consistent indentation, spacing around
operators and arrows, and blank lines). Specifically, reformat the
toggle_transaction_lock function (including the let outputs = ..., the if
outputs.is_empty() block, the current_state calculation, the spendable
assignment, the chained DB call to
labels.set_outputs_spendable(...).map_err_str(...), and the
CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(...) call) so it passes
rustfmt.
- Around line 317-371: compute_lock_state currently swallows DB errors by
treating get_output_record errors as None; change its signature to fn
compute_lock_state(&self, outputs: &[OutPoint]) -> Result<TransactionLockState,
Error> and return Err when any self.db.labels.get_output_record(outpoint) fails,
preserving existing semantics for empty outputs; update callers
transaction_lock_state and toggle_transaction_lock to propagate the Result
(i.e., transaction_lock_state should return ActorResult<TransactionLockState> by
returning Produces::ok(...) or an Err on DB failure, and toggle_transaction_lock
should early-return Err on compute_lock_state failure before deciding spendable
and calling self.db.labels.set_outputs_spendable and
CLOUD_BACKUP_MANAGER.handle_wallet_backup_change) so that DB read errors do not
silently mark outputs as unlocked.
- Around line 322-339: toggle_transaction_lock only updates labels but
transaction construction via build_tx (call site: self.wallet.bdk.build_tx())
still uses all UTXOs; ensure locked outputs are excluded by making the
coin-selection path consult the label DB. Change the coin selection wrapper
CoveDefaultCoinSelection (or the build_tx wrapper) to call the label DB method
locked_outpoints() and filter those out of the UTXO set passed into BDK (or set
the spendable flag when constructing the tx request), so BDK's default algorithm
never sees label-marked locked outputs; update references to
wallet.bdk.build_tx() to use the filtered UTXO list or a selection hook that
respects labels.
🪄 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: 1d95d31e-072f-43fa-bc1e-f6395fc8c3ad
📒 Files selected for processing (6)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.ktrust/src/database/wallet_data/label.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/transaction.rsrust/src/transaction/transaction_details.rs
|
Hey @praveenperera the Rust changes are all passing (clippy, rustfmt, tests), but the Android/iOS binding regeneration requires just build-android (Linux + NDK) and just build-ios (macOS), which I can't run from my Windows dev environment. Would it be okay if I set up a CI workflow (or a manual workflow_dispatch) that auto-regenerates the UniFFI bindings when Rust source changes? This would help any contributor who doesn't have the full cross-platform toolchain locally. |
|
@gitsofaryan ya thats sounds good, make it manual and have it commit the generated files after running, thanks |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/regenerate-bindings.yml (1)
52-52: Heads-up: commits pushed withGITHUB_TOKENwill not trigger downstream workflows.By GitHub's default, events triggered by the built-in
GITHUB_TOKENdo not kick off other workflows (no PR checks / CI will re-run on the regenerated-bindings commits). For a manualworkflow_dispatchthat lands on a feature branch this is usually desired, but contributors should be aware the regenerated commit will not re-trigger the standard PR validation. If re-running PR checks on the auto-commit is desired, use a PAT or GitHub App token instead.Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/regenerate-bindings.yml at line 52, The workflow currently uses the built-in GITHUB_TOKEN for the git push step which prevents downstream workflows from being triggered; replace the push authentication to use a repository secret holding a PAT or a GitHub App installation token (e.g., SECRET_PAT) instead of GITHUB_TOKEN, update the git push step that contains "git push" to authenticate with that secret, and ensure the workflow-level permissions include write access to contents (permissions: contents: write) so the PAT/App token can push and trigger downstream workflows again.android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt (1)
363-374: Localize snackbar messages and content descriptions.
"Transaction outputs locked","Transaction outputs unlocked","Remaining outputs locked","Failed to update lock state", and thecontentDescriptionvalues ("Unlock all outputs","Lock remaining outputs","Lock all outputs") are hardcoded English. Content descriptions in particular are read aloud by TalkBack, so they should go throughstringResource(...). Not a regression (file already has some hardcoded strings), but worth adding with this feature while string IDs are still fresh.Also applies to: 397-401
🤖 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/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt` around lines 363 - 374, Replace the hardcoded snackbar messages and TalkBack contentDescription literals with localized string resources: use stringResource(...) for the messages produced in the TransactionLockState when-block (the "Transaction outputs locked", "Transaction outputs unlocked", "Remaining outputs locked" branches) and for the error snackbar in the catch ("Failed to update lock state"), and replace the hardcoded contentDescription values ("Unlock all outputs", "Lock remaining outputs", "Lock all outputs") with stringResource(...) calls; add corresponding entries to strings.xml (e.g., tx_outputs_locked, tx_outputs_unlocked, tx_remaining_locked, tx_lock_update_failed, tx_cd_unlock_all, tx_cd_lock_remaining, tx_cd_lock_all) and ensure you import androidx.compose.ui.res.stringResource and use those stringResource(...) references in the UI (also update the other occurrence mentioned around lines ~397-401).
🤖 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/wallet_manager/actor.rs`:
- Around line 322-341: Replace the explicit closure-based error mappings in
transaction_lock_state and toggle_transaction_lock that use .map_err(|e|
Error::UnknownError(e.to_string())) with the repo utility .map_err_str from
cove_util::ResultExt (ResultExt is already imported in this file); update the
two calls inside functions transaction_lock_state and toggle_transaction_lock so
they call .map_err_str(Error::UnknownError) instead of the closure to follow the
repo convention.
- Around line 197-199: The current code calls
self.db.labels.locked_outpoints().unwrap_or_default() which masks DB errors and
can let locked UTXOs be spent; change both build_tx (the tx construction path
shown) and build_ephemeral_drain_tx to propagate the error instead of
defaulting: call self.db.labels.locked_outpoints()?, map its Err into the
appropriate BuildTxError (or return the existing error type used by
compute_lock_state) and return early on failure so tx_builder.unspendable only
receives a definitive list; mirror the error-handling and return-path used by
compute_lock_state to ensure DB read failures surface to the caller.
- Around line 197-199: The missing helper methods (self.do_build_tx,
self.do_build_manual_tx, self.do_build_ephemeral_tx,
self.do_build_ephemeral_drain_tx, self.do_build_manual_ephemeral_tx) must be
implemented as async functions that create and return the same transaction
builders/results as their callers expect; each must fetch locked_outpoints
exactly like the public path (e.g., let locked_outpoints =
self.db.labels.locked_outpoints().unwrap_or_default()) and apply
tx_builder.unspendable(locked_outpoints) to the builder before finishing, so
ephemeral/manual builds respect locked UTXO filtering and avoid fee/UTXO
disagreements when callers use .await?.
---
Nitpick comments:
In @.github/workflows/regenerate-bindings.yml:
- Line 52: The workflow currently uses the built-in GITHUB_TOKEN for the git
push step which prevents downstream workflows from being triggered; replace the
push authentication to use a repository secret holding a PAT or a GitHub App
installation token (e.g., SECRET_PAT) instead of GITHUB_TOKEN, update the git
push step that contains "git push" to authenticate with that secret, and ensure
the workflow-level permissions include write access to contents (permissions:
contents: write) so the PAT/App token can push and trigger downstream workflows
again.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.kt`:
- Around line 363-374: Replace the hardcoded snackbar messages and TalkBack
contentDescription literals with localized string resources: use
stringResource(...) for the messages produced in the TransactionLockState
when-block (the "Transaction outputs locked", "Transaction outputs unlocked",
"Remaining outputs locked" branches) and for the error snackbar in the catch
("Failed to update lock state"), and replace the hardcoded contentDescription
values ("Unlock all outputs", "Lock remaining outputs", "Lock all outputs") with
stringResource(...) calls; add corresponding entries to strings.xml (e.g.,
tx_outputs_locked, tx_outputs_unlocked, tx_remaining_locked,
tx_lock_update_failed, tx_cd_unlock_all, tx_cd_lock_remaining, tx_cd_lock_all)
and ensure you import androidx.compose.ui.res.stringResource and use those
stringResource(...) references in the UI (also update the other occurrence
mentioned around lines ~397-401).
🪄 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: b39255af-8da2-4197-abee-3a4191fa18f8
📒 Files selected for processing (5)
.github/workflows/regenerate-bindings.ymlandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.ktrust/src/database/wallet_data/label.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/database/wallet_data/label.rs
|
@gitsofaryan open a new PR for the github ci changes |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/src/manager/wallet_manager/actor.rs (1)
351-352: Minor: prefer auseover the fully-qualified path.Resolving
crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGERinline is noisy; a top-of-fileuse(or at least a scoped one) reads more cleanly and matches the rest of the imports in this module. Non-blocking.♻️ Proposed refactor
@@ top of file imports +use super::cloud_backup_manager::CLOUD_BACKUP_MANAGER; @@ toggle_transaction_lock - crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGER - .handle_wallet_backup_change(self.wallet.id.clone()); + CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(self.wallet.id.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 351 - 352, Replace the fully-qualified call to crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(...) with a simple reference by adding a use for CLOUD_BACKUP_MANAGER (e.g. add `use crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGER;` near the top of the module or a scoped use) and then call CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(self.wallet.id.clone()); keep the existing handle_wallet_backup_change and CLOUD_BACKUP_MANAGER identifiers unchanged.
🤖 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/wallet_manager/actor.rs`:
- Line 197: The DB call mapping in build_tx incorrectly maps a failure of
self.db.labels.locked_outpoints() to Error::UnknownError; change the error
mapping to Error::BuildTxError so DB failures during transaction construction
are classified as tx-build errors. Update the map_err_str on the
locked_outpoints() call in build_tx (and the similar mapping at the other
build_tx location around line 220) to return Error::BuildTxError, keeping the
existing error text handling.
---
Nitpick comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 351-352: Replace the fully-qualified call to
crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(...)
with a simple reference by adding a use for CLOUD_BACKUP_MANAGER (e.g. add `use
crate::manager::cloud_backup_manager::CLOUD_BACKUP_MANAGER;` near the top of the
module or a scoped use) and then call
CLOUD_BACKUP_MANAGER.handle_wallet_backup_change(self.wallet.id.clone()); keep
the existing handle_wallet_backup_change and CLOUD_BACKUP_MANAGER identifiers
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: 14d488f9-f446-4846-aaf1-33016b19fc84
📒 Files selected for processing (1)
rust/src/manager/wallet_manager/actor.rs
gitsofaryan
force-pushed
the
bulk-utxo-lock-661
branch
from
4e2ca8a to
6223a02
Compare
|
@praveenperera cc : #704 |
|
@gitsofaryan merge master and resolve conflicts |
|
can you use the new ci action on your fork to regen the bindings now? @gitsofaryan |
|
Just merged master! I'm triggering the Regenerate Bindings workflow on my fork for this branch now. Once the bot commits the fresh bindings, the CI should turn green. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/src/manager/wallet_manager/actor.rs (1)
334-403: Lock-state actor methods look correct.
- 3-state toggle logic at Line 356 (
Locked → unlock; Unlocked/Mixed → lock) matches the acceptance criteria.- Empty-output early return at Lines 348-350 avoids spurious DB writes and cloud-backup notifications.
compute_lock_statenow propagates label-DB read errors viaResult<_, label::Error>(no more fail-open) and the callers map toError::UnknownErrorper guideline.wallet_unspent_outputs_for_txcorrectly scopes to wallet-owned, still-unspent outputs of the given txid, honoring the "external outputs and already-spent outputs are ignored" requirement.- Since the actor serializes messages, the read-in-
compute_lock_state/ write-in-set_outputs_spendablesequence is free from intra-actor TOCTOU.One very minor, optional efficiency note:
compute_lock_stateopens a new read transaction per outpoint viaget_output_record. For typical txs with a handful of outputs this is negligible, but if you ever want to consolidate, a single read-table scan (or reusinglocked_outpoints()as aHashSetlookup) would fold all N lookups into one table open. Not worth changing now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 334 - 403, The review finds no functional issues: the actor methods transaction_lock_state, toggle_transaction_lock, compute_lock_state, and wallet_unspent_outputs_for_tx are correct and follow requirements (3-state toggle, early return on empty outputs, proper error propagation and wallet-scoped UTXO filtering); no code changes required—merge as-is, optional future optimization: consolidate per-outpoint DB reads in compute_lock_state into a single scan or HashSet lookup if you later profile it as a hotspot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 334-403: The review finds no functional issues: the actor methods
transaction_lock_state, toggle_transaction_lock, compute_lock_state, and
wallet_unspent_outputs_for_tx are correct and follow requirements (3-state
toggle, early return on empty outputs, proper error propagation and
wallet-scoped UTXO filtering); no code changes required—merge as-is, optional
future optimization: consolidate per-outpoint DB reads in compute_lock_state
into a single scan or HashSet lookup if you later profile it as a hotspot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 332db88e-aaa3-4c46-908a-233d88c76780
📒 Files selected for processing (2)
rust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rs
|
@praveenperera LGTM! |
2 participants
solves : (#661)
Summary
Implementing a transaction-level bulk lock shortcut in the Transaction Details view. This allows users to quickly secure all unspent outputs created by a specific transaction, preventing them from being automatically spent in future "Send" flows.
Parent Issue
Acceptance Criteria Met
Unlocked,Locked,Mixed).UnlockedorMixedlocks all wallet-owned unspent outputs for that transaction.Lockedunlocks all wallet-owned unspent outputs for that transaction.Technical Changes
Rust Backend
set_outputs_spendablefor bulk updating the BIP329spendableflag inOUTPUT_TABLE.compute_lock_stateandtransaction_lock_statefor aggregate analysis.toggle_transaction_lockwith actor-safe logic (avoids self-async-recursion).list_unspent()to ensure only relevant wallet UTXOs are targeted.RustWalletManagerand Uniffi.Android UI
CenterAlignedTopAppBar.AnimatedContentfor smooth icon transitions.Testing
Platform Coverage
Checklist
Summary by CodeRabbit