Uh oh!
There was an error while loading. Please reload this page.
feat: wallet transaction confirmation lifecycle - #540
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds InstantSend transaction context and propagation across FFI, manager, wallet-checking, account/UTXO state, and test utilities; expands check_core_transaction with an extra update_balance flag and updates call sites and conversions accordingly. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## v0.42-dev #540 +/- ##
=============================================
+ Coverage 66.13% 66.24% +0.10%
=============================================
Files 311 311 Lines 64757 65022 +265 =============================================
+ Hits 42829 43073 +244 - Misses 21928 21949 +21
|
xdustinfaceforce-pushed
the
feat/wallet-confirmation-lifecycle
branch
3 times, most recently
from
March 16, 2026 07:28
5528f98 to
11405bcCompareContributor
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinfaceforce-pushed
the
feat/wallet-confirmation-lifecycle
branch
from
March 17, 2026 02:57
11405bc to
41d957fComparexdustinface
marked this pull request as ready for review
March 17, 2026 02:58
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/managed_account/mod.rs (1)
347-357:⚠️ Potential issue | 🔴 CriticalAdd
#[serde(default)]toUtxo::is_instantlockedfor backward compatibility.The new
is_instantlockedfield inkey-wallet/src/utxo.rs(line 32) is serialized without a default value. Previously persisted wallets—which lack this field entirely—will fail to deserialize after upgrade because Serde expects the field to be present. Add#[serde(default)]to allow loading older wallet files:Fix
// key-wallet/src/utxo.rs#[cfg_attr(feature = "serde", serde(default))]pub is_instantlocked: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 347 - 357, The Utxo struct's new boolean field is_instantlocked needs a Serde default so older serialized wallets missing that field can deserialize; edit the Utxo definition (the is_instantlocked field in key-wallet/src/utxo.rs) and add the serde default attribute (e.g. cfg_attr for the serde feature) on that field so missing values default to false, then run the serde-related tests/deserialize older fixtures to confirm compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 179-191: The example C enum usages in the example
(check_transaction.c) are now out-of-date because the discriminants were
changed: MEMPOOL=0, INSTANT_SEND=1, IN_BLOCK=2, IN_CHAIN_LOCKED_BLOCK=3; update
any literal integer values or switches in the example to use the new enum
constants (MEMPOOL, INSTANT_SEND, IN_BLOCK, IN_CHAIN_LOCKED_BLOCK) or adjust the
numeric literals to match these new discriminants so that old InBlock=1
semantics no longer map to InstantSend.
In `@key-wallet-ffi/src/types.rs`:
- Around line 713-721: The FFI discriminants were shifted by inserting
InstantSend; restore the original discriminants and append the new variant:
change FFITransactionContext to keep Mempool = 0, set InBlock = 1,
InChainLockedBlock = 2, and add InstantSend = 3 as the last variant so existing
consumers keep their numeric values; update the enum declaration for
FFITransactionContext accordingly.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 389-397: The code only updates confirmation metadata when both
block height and block hash are Some, causing a mempool tx to retain stale
confirmation state when TransactionContext::InBlock or ::InChainLockedBlock
provide height but block_hash is None; change the condition so that if
context.block_height() is Some(height) you call tx_record.mark_confirmed(height,
context.block_hash()) (allowing a None hash), set changed = true, and still
update tx_record.timestamp from context.timestamp() if present; update the
branch around tx_record.is_confirmed() accordingly (referencing
tx_record.mark_confirmed, TransactionContext::InBlock/InChainLockedBlock, and
update_utxos).
- Around line 388-399: confirm_transaction() can update the UTXO set for an
account even when that account has no TransactionRecord (get_mut() misses),
leaving spendable outputs without a history entry; change confirm_transaction()
to backfill a per-account TransactionRecord when the tx is absent before calling
update_utxos: detect the missing record (get_mut() returned None and
check_core_transaction() indicated this account should care), create and insert
a TransactionRecord for this account (setting confirmation via
TransactionRecord::mark_confirmed and timestamp from context if present) and
only then call update_utxos so both transaction history and UTXO updates happen
atomically; ensure the insertion and the mark_confirmed/timestamp assignment are
performed together to prevent inconsistent state.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 167-193: check_core_transaction() mutates UTXO spendability in the
InstantSend branch and the normal record/confirm path but never refreshes cached
balances; after you mark UTXOs as IS-locked (inside the InstantSend branch where
account.mark_utxos_instant_send(&txid) is called) and after the normal
record/confirm paths (the code region around lines 200-259), call the wallet's
balance-refresh helper (e.g. self.update_balances() or the existing
update_balance(...) API) to recompute cached balances so update_state=true
semantics are honored for external callers; add the call right after the UTXO
mutations in check_core_transaction().
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 236-244: The mark_instant_send_utxos method updates UTXO lock
state across accounts but does not refresh the wallet's cached balance
(self.balance), which can leave callers seeing stale balances; after the loop,
if any_changed is true call the existing balance refresh/recompute routine
(e.g., a method like recompute_balance(), refresh_balance_cache(), or the
appropriate balance update helper) to recalculate and assign self.balance so the
cache reflects the InstantSend state change; locate mark_instant_send_utxos,
accounts.all_accounts_mut(), and account.mark_utxos_instant_send(txid) to add
this conditional balance recomputation.
---
Outside diff comments:
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 347-357: The Utxo struct's new boolean field is_instantlocked
needs a Serde default so older serialized wallets missing that field can
deserialize; edit the Utxo definition (the is_instantlocked field in
key-wallet/src/utxo.rs) and add the serde default attribute (e.g. cfg_attr for
the serde feature) on that field so missing values default to false, then run
the serde-related tests/deserialize older fixtures to confirm compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6589e0f6-7109-413c-b248-78ed43cc80c0
📒 Files selected for processing (10)
key-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
xdustinface
marked this pull request as draft
March 17, 2026 03:52
QuantumExplorer
commented
Mar 17, 2026
Member
Review from ClaudeIssue 1: FFI ABI Breaking Change — |
| Variant | Before | After |
|---|---|---|
| MEMPOOL | 0 | 0 |
| INSTANT_SEND | — | 1 |
| IN_BLOCK | 1 | 2 |
| IN_CHAIN_LOCKED_BLOCK | 2 | 3 |
Since FFITransactionContext is #[repr(C)], any existing C/Swift consumer that passes 1 meaning "in-block" will now be interpreted as "InstantSend". The transaction won't be marked confirmed, UTXOs will have incorrect state, and coinbase maturity checks could use height 0.
Suggested fix: Append INSTANT_SEND at the end (= 3) instead of inserting in the middle, or bump a version and document the ABI break.
Issue 2: confirm_transaction silently skips when block_hash is None
Severity: High
The guard if let (Some(height), Some(hash)) = (context.block_height(), context.block_hash()) requires BOTH to be Some. But the FFI layer allows block_hash: None for InBlock context. When this happens:
TransactionRecordis NOT marked confirmed (height staysNone,is_confirmed()returnsfalse)update_utxosIS called unconditionally afterward, settingutxo.is_confirmed = true
This creates split-brain state: the transaction record says "unconfirmed" but its UTXOs are spendable. Subsequent IS-lock dedup checks (already_confirmed) will incorrectly think the tx isn't confirmed, causing unnecessary reprocessing.
Suggested fix: Allow confirming with height alone:
ifletSome(height) = context.block_height(){if !tx_record.is_confirmed(){
tx_record.mark_confirmed(height, context.block_hash().unwrap_or_default());
changed = true;}}Add `InstantSend` transaction context and transaction state management so wallet transactions can transition through confirmation stages (mempool → InstantSend → block → chain-locked block). Key changes: - Add `TransactionContext::InstantSend` variant with FFI support - Track known transactions to avoid double-counting - Add `confirm_transaction()` for mempool→block transitions - Add IS lock dedup set to prevent redundant processing - Update balance after any UTXO state change - Add `status_changed` field to `TransactionCheckResult` - Add `TestWalletContext::with_mempool_funding()` helper - Add more relevant tests
…issing accounts Addresses CodeRabbit review comment on PR #540 File: key-wallet/src/managed_account/mod.rs
Addresses CodeRabbit review comment on PR #540 File: key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
…ansaction()` Addresses CodeRabbit review comment on PR #540 File: key-wallet/src/transaction_checking/wallet_checker.rs
xdustinfaceforce-pushed
the
feat/wallet-confirmation-lifecycle
branch
from
March 17, 2026 14:07
41d957f to
698d75aComparexdustinface
marked this pull request as ready for review
March 17, 2026 14:09
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet-ffi/include/key_wallet_ffi.h (1)
175-191:⚠️ Potential issue | 🔴 CriticalPreserve the existing FFI discriminants or version the ABI.
The enum has been reordered, shifting
InBlockfrom1to2andInChainLockedBlockfrom2to3with the insertion ofInstantSend = 1. This is a breaking change for existing C/Swift clients that previously passed1for "in block" transactions—they will now be interpreted as InstantSend.Since this file is auto-generated by cbindgen, fix the Rust enum source in
key-wallet-ffi/src/types.rsby either: (1) restoring the original discriminant values (InBlock = 1,InChainLockedBlock = 2) and appendingInstantSend = 3at the end, or (2) explicitly documenting and versioning the ABI break if breaking compatibility is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/include/key_wallet_ffi.h` around lines 175 - 191, The generated C enum changed discriminants and breaks existing FFI consumers; fix the Rust source in key-wallet-ffi/src/types.rs (the enum that emits MEMPOOL, InstantSend, InBlock, InChainLockedBlock) so the C ABI preserves original values: set InBlock = 1 and InChainLockedBlock = 2 and move/append InstantSend to a new discriminant (e.g., InstantSend = 3), or alternatively add explicit ABI/versioning documentation and a new versioned enum type if the break is intentional; regenerate the header with cbindgen after updating the Rust enum.key-wallet-ffi/src/transaction_checking.rs (1)
144-187:⚠️ Potential issue | 🟠 MajorReject NULL block hashes for confirmed contexts at the FFI boundary.
The FFI function builds
TransactionContext::InBlock { block_hash: None }andInChainLockedBlock { block_hash: None }when the caller omits the hash. This causes a state split inManagedWalletInfo:
- For new transactions:
record_transactionsetsheightfrom the context, makingis_confirmed()returntrue, but leavesblock_hash: Nonein the record.- For existing unconfirmed transactions:
confirm_transactionrequires both(Some(height), Some(hash))to update the record. With a NULL hash,mark_confirmed()is never called, so the transaction record stays unconfirmed. However,update_utxos()still runs and marks UTXOs as confirmed becausecontext.confirmed()is true forInBlockregardless of the hash value.The result is inconsistent persisted state: transaction balance remains unconfirmed while UTXO balance becomes confirmed. Either validate and reject missing hashes for confirmed contexts here, or redesign the record structure to be height-only. As per coding guidelines,
**/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction_checking.rs` around lines 144 - 187, The code currently allows constructing TransactionContext::InBlock and ::InChainLockedBlock with block_hash == None when the incoming FFI pointer is NULL; change the FFI boundary in the match over FFITransactionContext to validate that block_hash is non-null for confirmed contexts and return a failure (propagate an Err/invalid-argument) instead of creating a None hash. Concretely, inside the arms handling FFITransactionContext::InBlock and ::InChainLockedBlock, add an explicit check for block_hash.is_null() and return an error/result early (do not proceed to slice::from_raw_parts or build TransactionContext with block_hash=None); keep the existing conversion using slice::from_raw_parts and dashcore::BlockHash::from_byte_array when non-null so the code still constructs TransactionContext::InBlock/InChainLockedBlock with Some(hash) and timestamp handling as before.
🧹 Nitpick comments (1)
key-wallet/src/test_utils/wallet.rs (1)
76-83: Expose the new balance-refresh switch in the test helper.Hardcoding
update_balance = truehere means tests built onTestWalletContextcan only exercise the eager-refresh path, which leaves the new batching path awkward to cover. Thread the flags through or add a second helper so lifecycle tests can hit both behaviors.As per coding guidelines, "Write unit tests for new functionality in Rust code".♻️ Suggested refactor
- pub async fn check_transaction(- &mut self,- tx: &Transaction,- context: TransactionContext,- ) -> TransactionCheckResult {- self.managed_wallet.check_core_transaction(tx, context, &mut self.wallet, true, true).await- }+ pub async fn check_transaction_with_options(+ &mut self,+ tx: &Transaction,+ context: TransactionContext,+ update_state: bool,+ update_balance: bool,+ ) -> TransactionCheckResult {+ self.managed_wallet+ .check_core_transaction(+ tx,+ context,+ &mut self.wallet,+ update_state,+ update_balance,+ )+ .await+ }++ pub async fn check_transaction(+ &mut self,+ tx: &Transaction,+ context: TransactionContext,+ ) -> TransactionCheckResult {+ self.check_transaction_with_options(tx, context, true, true).await+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/test_utils/wallet.rs` around lines 76 - 83, The test helper check_transaction currently hardcodes the balance-refresh flag by calling managed_wallet.check_core_transaction(..., true, true); update check_transaction signature to accept an update_balance: bool (or add an alternative helper like check_transaction_eager/check_transaction_batched) and forward that boolean into the managed_wallet.check_core_transaction call instead of the hardcoded last true; specifically modify the async fn check_transaction(&mut self, tx: &Transaction, context: TransactionContext, update_balance: bool) -> TransactionCheckResult (or add a second helper) and pass update_balance as the final argument to managed_wallet.check_core_transaction(tx, context, &mut self.wallet, true, update_balance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 218-222: The confirm_transaction path conflates "InBlock with
height but no block_hash" so transactions stay unconfirmed while update_utxos
marks UTXOs confirmed; modify confirm_transaction to treat InBlock { height:
Some(h), block_hash: None } as a valid confirmation (i.e., accept height-only
confirmations) so the transaction record and UTXOs stay in sync: update the
pattern matching/logic in confirm_transaction (and any place that later writes
transaction.status or confirmation fields) to accept height-only InBlock (use
the height when present, and either leave hash empty or substitute a canonical
placeholder) rather than requiring both height AND block_hash, ensuring
update_utxos and context.confirmed() remain consistent.
---
Outside diff comments:
In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 175-191: The generated C enum changed discriminants and breaks
existing FFI consumers; fix the Rust source in key-wallet-ffi/src/types.rs (the
enum that emits MEMPOOL, InstantSend, InBlock, InChainLockedBlock) so the C ABI
preserves original values: set InBlock = 1 and InChainLockedBlock = 2 and
move/append InstantSend to a new discriminant (e.g., InstantSend = 3), or
alternatively add explicit ABI/versioning documentation and a new versioned enum
type if the break is intentional; regenerate the header with cbindgen after
updating the Rust enum.
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 144-187: The code currently allows constructing
TransactionContext::InBlock and ::InChainLockedBlock with block_hash == None
when the incoming FFI pointer is NULL; change the FFI boundary in the match over
FFITransactionContext to validate that block_hash is non-null for confirmed
contexts and return a failure (propagate an Err/invalid-argument) instead of
creating a None hash. Concretely, inside the arms handling
FFITransactionContext::InBlock and ::InChainLockedBlock, add an explicit check
for block_hash.is_null() and return an error/result early (do not proceed to
slice::from_raw_parts or build TransactionContext with block_hash=None); keep
the existing conversion using slice::from_raw_parts and
dashcore::BlockHash::from_byte_array when non-null so the code still constructs
TransactionContext::InBlock/InChainLockedBlock with Some(hash) and timestamp
handling as before.
---
Nitpick comments:
In `@key-wallet/src/test_utils/wallet.rs`:
- Around line 76-83: The test helper check_transaction currently hardcodes the
balance-refresh flag by calling managed_wallet.check_core_transaction(..., true,
true); update check_transaction signature to accept an update_balance: bool (or
add an alternative helper like
check_transaction_eager/check_transaction_batched) and forward that boolean into
the managed_wallet.check_core_transaction call instead of the hardcoded last
true; specifically modify the async fn check_transaction(&mut self, tx:
&Transaction, context: TransactionContext, update_balance: bool) ->
TransactionCheckResult (or add a second helper) and pass update_balance as the
final argument to managed_wallet.check_core_transaction(tx, context, &mut
self.wallet, true, update_balance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7c5240c-6379-43bd-89a8-0854d4b146a3
📒 Files selected for processing (19)
key-wallet-ffi/examples/check_transaction.ckey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/provider.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet/src/transaction_checking/account_checker.rs
- key-wallet/src/wallet/managed_wallet_info/mod.rs
Uh oh!
There was an error while loading. Please reload this page.
ZocoLini
approved these changes
Mar 18, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 24, 2026
3 tasks
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 freeto 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.
Add
InstantSendtransaction context and transaction state managementso wallet transactions can transition through confirmation stages
(mempool → InstantSend → block → chain-locked block).
Key changes:
TransactionContext::InstantSendvariant with FFI supportconfirm_transaction()for mempool→block transitionsstatus_changedfield toTransactionCheckResultTestWalletContext::with_mempool_funding()helperBased on:
TestWalletContext#539snapshot_balances/emit_balance_changeshelpers #542Summary by CodeRabbit
New Features
Tests