fix: correct confirmations for UTXOs in wallet FFI #306
Conversation
📝 WalkthroughWalkthroughThe changes introduce dynamic UTXO confirmation calculation based on blockchain height instead of hard-coded values. Confirmation count is now computed as Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
xdustinface
changed the title
xdustinface
force-pushed
the
fix/ffi-utxo-confirmations
branch
from
a3d1136 to
ae4114a
Compare
xdustinface
force-pushed
the
feat/wallet-sync-height
branch
from
7316f44 to
0505ed2
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/wallet-sync-height
branch
from
0505ed2 to
5c38c9d
Compare
xdustinface
force-pushed
the
fix/ffi-utxo-confirmations
branch
from
ae4114a to
8c6e2b9
Compare
github-actions
bot
removed
the
merge-conflict
![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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
key-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
key-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
key-wallet-ffi/src/utxo_tests.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Applied to files:
key-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Apply atomic state updates when managing watch-only wallets: validate that external signatures match expected pubkeys and never attempt signing operations
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use property-based testing for complex invariants such as gap limit constraints
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Applied to files:
key-wallet-ffi/src/utxo_tests.rs
🧬 Code graph analysis (2)
key-wallet-ffi/src/utxo.rs (2)
key-wallet-manager/src/wallet_manager/mod.rs (1)
current_height(934-936)key-wallet/src/wallet/immature_transaction.rs (1)
confirmations(121-127)
key-wallet-ffi/src/utxo_tests.rs (1)
key-wallet/src/wallet/immature_transaction.rs (1)
confirmations(121-127)
🔇 Additional comments (1)
key-wallet-ffi/src/utxo_tests.rs (1)
237-237: Approve the confirmation calculation update at line 237; other tests don't require synced_height since they test unconfirmed UTXOs.The test correctly sets
synced_height(300)to validate confirmation calculations for confirmed UTXOs. However, the other test functions (test_managed_wallet_get_utxos_multiple_accountsandtest_managed_wallet_get_utxos) intentionally create UTXOs withis_confirmed = false, which means confirmations will be 0 regardless of synced_height. These tests validate the basic UTXO retrieval functionality rather than confirmation counting, so they won't fail. Consider adding explicit test coverage for unconfirmed UTXO scenarios and edge cases like height = 0 to ensure comprehensive coverage of the confirmation logic.
| assert_eq!(utxos[0].confirmations, 201); | ||
|
|
||
| // Check second UTXO | ||
| assert_eq!(utxos[1].txid[0], 1); | ||
| assert_eq!(utxos[1].vout, 1); | ||
| assert_eq!(utxos[1].amount, 100000); | ||
| assert_eq!(utxos[1].height, 101); | ||
| assert_eq!(utxos[1].confirmations, 200); | ||
|
|
||
| // Check third UTXO | ||
| assert_eq!(utxos[2].txid[0], 2); | ||
| assert_eq!(utxos[2].vout, 2); | ||
| assert_eq!(utxos[2].amount, 150000); | ||
| assert_eq!(utxos[2].height, 102); | ||
| assert_eq!(utxos[2].confirmations, 199); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for edge cases in confirmation calculation.
The updated confirmation values (201, 200, 199) correctly reflect the calculation based on synced_height(300) and UTXO heights (100, 101, 102). However, the test suite is missing coverage for important edge cases:
- Unconfirmed UTXOs (
is_confirmed = false) should return 0 confirmations - UTXOs with
height = 0should return 0 confirmations - When
current_height < utxo.height(e.g., during sync or reorg) should return 0 confirmations - Boundary case:
current_height = utxo.heightshould return 1 confirmation
Based on coding guidelines, consider adding property-based tests for confirmation invariants and covering mainnet/testnet configurations.
🤖 Prompt for AI Agents
In key-wallet-ffi/src/utxo_tests.rs around lines 256 to 270, the existing
assertions check normal confirmation counts but lack edge-case coverage; add
unit tests that (1) assert confirmations == 0 when is_confirmed == false, (2)
assert confirmations == 0 when utxo.height == 0, (3) assert confirmations == 0
when current_height < utxo.height (simulate reorg/sync), and (4) assert
confirmations == 1 when current_height == utxo.height; additionally add a
property-based test to validate confirmation invariants across random heights
and a small matrix to run the same cases under mainnet and testnet
configurations to ensure behavior is consistent.
| let current_height = managed_info.inner().synced_height(); | ||
| let confirmations = if utxo.is_confirmed && utxo.height > 0 { | ||
| current_height.saturating_sub(utxo.height) + 1 | ||
| } else { | ||
| 0 | ||
| }; |
There was a problem hiding this comment.
Critical bug: Incorrect confirmation count when current_height < utxo.height.
The calculation current_height.saturating_sub(utxo.height) + 1 returns 1 confirmation when current_height < utxo.height, which is incorrect. When the synced height hasn't caught up to the UTXO's block height (e.g., during initial sync or reorg), the code should return 0 confirmations, not 1.
The existing pattern in immature_transaction.rs:120-126 correctly checks current_height >= self.height before adding 1.
🔎 Proposed fix to match the established pattern
let current_height = managed_info.inner().synced_height();
-let confirmations = if utxo.is_confirmed && utxo.height > 0 {
- current_height.saturating_sub(utxo.height) + 1
+let confirmations = if utxo.is_confirmed && utxo.height > 0 && current_height >= utxo.height {
+ (current_height - utxo.height) + 1
} else {
0
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let current_height = managed_info.inner().synced_height(); | |
| let confirmations = if utxo.is_confirmed && utxo.height > 0 { | |
| current_height.saturating_sub(utxo.height) + 1 | |
| } else { | |
| 0 | |
| }; | |
| let current_height = managed_info.inner().synced_height(); | |
| let confirmations = if utxo.is_confirmed && utxo.height > 0 && current_height >= utxo.height { | |
| (current_height - utxo.height) + 1 | |
| } else { | |
| 0 | |
| }; |
🤖 Prompt for AI Agents
In key-wallet-ffi/src/utxo.rs around lines 122 to 127, the confirmation
calculation uses current_height.saturating_sub(utxo.height) + 1 which yields 1
when current_height < utxo.height; change the logic to first require
current_height >= utxo.height before computing confirmations (e.g., if
utxo.is_confirmed && current_height >= utxo.height { current_height -
utxo.height + 1 } else { 0 }) so that heights not yet reached return 0
confirmations.
managed_wallet_get_utxoscurrently just returns 1 confirmation for a UTXO inFFIUTXO. This makes use of thesynced_heightto get the actual confirmations.Based on:
synced_heightperManagedWalletInfo#305Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.