refactor: cleanup WalletBalance
#335
Conversation
|
Warning Rate limit exceeded@xdustinface has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughWalletBalance was refactored into an immutable value with private fields and accessor methods (spendable(), unconfirmed(), locked(), total()); mutating APIs and BalanceError were removed. Callsites, FFI conversions, tests, and a dash-spv snapshot log were updated to use the new accessors and constructor. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
force-pushed
the
refactor/wallet-balance
branch
from
9f3e1c2 to
aa61e72
Compare
xdustinface
force-pushed
the
refactor/wallet-balance
branch
from
aa61e72 to
da1bc6b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
key-wallet/src/wallet/balance.rs (1)
47-51: Add test coverage for the u64::MAX clamping behavior.The
total()method correctly prevents overflow by usingu128for intermediate calculation and clamping tou64::MAX. However, this silent clamping behavior lacks test coverage, which could lead to surprises if users encounter edge cases where individual components sum beyondu64::MAX.🔎 Suggested test for overflow edge case
Add this test to verify the clamping behavior:
#[test] fn test_balance_total_overflow_clamping() { // Create a balance where components sum beyond u64::MAX let balance = WalletBalance::new(u64::MAX, u64::MAX, u64::MAX); // Should clamp to u64::MAX rather than overflow assert_eq!(balance.total(), u64::MAX); // Individual components should still be accessible assert_eq!(balance.spendable(), u64::MAX); assert_eq!(balance.unconfirmed(), u64::MAX); assert_eq!(balance.locked(), u64::MAX); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
dash-spv/src/main.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/lib.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/balance.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
💤 Files with no reviewable changes (2)
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet/src/wallet/managed_wallet_info/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- key-wallet-ffi/src/wallet_manager.rs
- key-wallet-manager/tests/integration_test.rs
- key-wallet-ffi/src/types.rs
- key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet-manager/examples/wallet_creation.rs
- key-wallet-ffi/src/managed_wallet.rs
🧰 Additional context used
📓 Path-based instructions (7)
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Separate immutable structures (Account,Wallet) containing only identity information from mutable wrappers (ManagedAccount,ManagedWalletInfo) with state management
Never serialize or log private keys in production; use public keys or key fingerprints for identification instead
Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations
UseBTreeMapfor ordered data (accounts, transactions) andHashMapfor lookups (address mappings); apply memory management strategies for old transaction data
Apply atomic state updates when managing watch-only wallets: validate that external signatures match expected pubkeys and never attempt signing operations
Use the?operator for error propagation, provide context in error messages, never panic in library code, and returnResult<T>for all fallible operations
Files:
key-wallet/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Files:
key-wallet/src/managed_account/mod.rs
**/*.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/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.rsdash-spv/src/main.rs
key-wallet/**/transaction_checking/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement transaction classification and routing through
TransactionRouterto avoid checking all accounts for every transaction
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/**/tests/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/tests/**/*.rs: Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Use deterministic testing with known test vectors and fixed seeds for reproducible results
Use property-based testing for complex invariants such as gap limit constraints
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/main.rs
🧠 Learnings (21)
📓 Common learnings
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
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
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/**/account/**/*.rs : Use enum-based type system for `AccountType` with specific variants (Standard, IdentityAuthentication, IdentityEncryption, MasternodeOperator, etc.) to provide compile-time safety and clear semantics
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 the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
📚 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/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rsdash-spv/src/main.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/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.rsdash-spv/src/main.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/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.rsdash-spv/src/main.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/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.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/**/account/**/*.rs : Use enum-based type system for `AccountType` with specific variants (Standard, IdentityAuthentication, IdentityEncryption, MasternodeOperator, etc.) to provide compile-time safety and clear semantics
Applied to files:
key-wallet/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.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 the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Applied to files:
key-wallet/src/managed_account/mod.rskey-wallet/src/wallet/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.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/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store them in pools; only derive on-demand when the pool is exhausted
Applied to files:
key-wallet/src/managed_account/mod.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/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet/src/wallet/mod.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/balance.rskey-wallet/src/lib.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/**/address_pool/**/*.rs : Use staged gap limit management with `GapLimitStage` tracking `last_used_index` and `used_indices` to enable efficient address discovery without loading entire chains into memory
Applied to files:
key-wallet/src/wallet/mod.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/**/transaction_checking/**/*.rs : Implement transaction classification and routing through `TransactionRouter` to avoid checking all accounts for every transaction
Applied to files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/src/transaction_checking/transaction_router/tests/routing.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 : Never serialize or log private keys in production; use public keys or key fingerprints for identification instead
Applied to files:
key-wallet/src/lib.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/main.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:
dash-spv/src/main.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/main.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/src/main.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/main.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/src/main.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/main.rs
🧬 Code graph analysis (5)
key-wallet/src/managed_account/mod.rs (3)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
update_balance(80-80)update_balance(204-223)key-wallet/src/wallet/balance.rs (3)
unconfirmed(37-39)locked(42-44)new(23-29)key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
new(55-66)
key-wallet/src/wallet/mod.rs (2)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (3)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)key-wallet/src/wallet/balance.rs (1)
spendable(32-34)
key-wallet/src/lib.rs (2)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)
dash-spv/src/main.rs (1)
key-wallet-manager/src/wallet_manager/mod.rs (1)
get_wallet_balance(887-894)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: Address Sanitizer
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: Thread Sanitizer
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
🔇 Additional comments (11)
key-wallet-manager/src/wallet_manager/mod.rs (1)
883-883: LGTM! Correct usage of accessor method.The change from field access to the
total()accessor method is correct and aligns with the WalletBalance API refactor.key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (1)
172-174: LGTM! Correct usage of spendable accessor.The test correctly uses the
spendable()accessor method to verify that the balance remains unchanged whenupdate_stateis false.key-wallet/src/wallet/mod.rs (1)
19-19: LGTM! Intentional public API change.The removal of
BalanceErrorfrom the public exports is consistent with the PR's goal to simplify the WalletBalance API by removing error handling from balance operations.dash-spv/src/main.rs (1)
454-473: LGTM! Simplified balance logging.The refactor to use a single
WalletBalancevalue instead of multiple individual fields is cleaner and leverages theDisplayimplementation. Usingunwrap_or_default()is appropriate in this logging context.key-wallet/src/managed_account/mod.rs (1)
268-270: LGTM! Simplified balance update.The removal of the
Resultreturn type and direct construction viaWalletBalance::new()simplifies the API by making balance updates infallible. The metadata timestamp update is correctly preserved.key-wallet/src/lib.rs (1)
68-68: LGTM! BalanceError removal aligns with the refactored API.The removal of
BalanceErrorfrom the public re-exports is consistent with the refactoredWalletBalanceAPI, where the constructor no longer returns aResultand error handling has been simplified.key-wallet/src/wallet/balance.rs (5)
12-19: LGTM! Private fields properly encapsulate balance state.Making the fields private and providing accessor methods follows Rust best practices and aligns with the guideline to use immutable structures for value types.
23-29: LGTM! Constructor simplification improves API usability.The simplified constructor that returns
Selfdirectly (rather thanResult) makes the API more ergonomic and addresses the past review feedback about removing error handling from balance operations.
31-44: LGTM! Clean accessor implementations.The getter methods provide clean, idiomatic access to the private balance fields.
54-65: LGTM! Clear and comprehensive Display implementation.The
Displayimplementation provides a well-formatted view of all balance components, including the computed total.
71-88: LGTM! Tests properly updated for the refactored API.The tests correctly exercise the new constructor and accessor-based API. The test coverage for basic functionality is solid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
key-wallet/src/wallet/balance.rs (1)
47-51: Silent capping at u64::MAX could hide overflow issues.The
total()method silently caps the sum atu64::MAXwhen the actual total exceeds this limit. This creates several problems:
- Information loss without notification: Users have no way to detect when capping occurs
- Incorrect balance reporting:
total()may return a value less than the actual sum of components- Inconsistent state:
spendable() + unconfirmed() + locked()may not equaltotal()Based on the past review comment, a reviewer previously questioned this approach and suggested using
u128to avoid forcing users to handle overflow errors. The current implementation still usesu64for the return type but caps instead of erroring.Consider these alternatives:
- Option 1 (recommended): Return
u128fromtotal()to accurately represent any possible sum- Option 2: Return
Option<u64>whereNoneindicates overflow- Option 3: Return
Result<u64, OverflowError>to explicitly signal overflow🔎 Recommended fix: Use u128 for total()
/// Get the total balance. - pub fn total(&self) -> u64 { - // For safety restrict the total balance to u64::MAX, in practice this should never be relevant. - let total = self.spendable as u128 + self.unconfirmed as u128 + self.locked as u128; - total.min(u64::MAX as u128) as u64 + pub fn total(&self) -> u128 { + self.spendable as u128 + self.unconfirmed as u128 + self.locked as u128 }Note: This change would require updating all call sites that use
total()to handleu128values.
🧹 Nitpick comments (1)
key-wallet-ffi/src/managed_wallet.rs (1)
579-582: Update documentation to reflect the semantic change from confirmed to spendable balance.The FFI parameter is named
confirmed_outand the documentation (lines 534, 539) refers to "confirmed balance", but the implementation now returnsbalance.spendable()instead of the previousbalance.confirmed.If "confirmed" and "spendable" are semantically equivalent (likely, given this is a clarity refactor), consider updating the function documentation to clarify that
confirmed_outreturns the spendable balance. This helps prevent confusion for FFI clients.If they represent different concepts, this could be a breaking semantic change for FFI consumers.
Suggested documentation update
/// Get wallet balance from managed wallet info /// -/// Returns the balance breakdown including confirmed, unconfirmed, locked, and total amounts. +/// Returns the balance breakdown including spendable (confirmed and unlocked), unconfirmed, locked, and total amounts. /// /// # Safety /// /// - `managed_wallet` must be a valid pointer to an FFIManagedWalletInfo -/// - `confirmed_out` must be a valid pointer to store the confirmed balance +/// - `confirmed_out` must be a valid pointer to store the spendable balance /// - `unconfirmed_out` must be a valid pointer to store the unconfirmed balance
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
dash-spv/src/main.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/lib.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/balance.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
💤 Files with no reviewable changes (2)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- key-wallet-manager/src/wallet_manager/mod.rs
- key-wallet/src/wallet/mod.rs
- key-wallet-ffi/src/types.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-ffi/src/wallet_manager.rs
- key-wallet/src/lib.rs
- key-wallet-manager/examples/wallet_creation.rs
- key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
- key-wallet-ffi/src/managed_account.rs
- dash-spv/src/main.rs
- key-wallet-manager/tests/integration_test.rs
🧰 Additional context used
📓 Path-based instructions (6)
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Separate immutable structures (Account,Wallet) containing only identity information from mutable wrappers (ManagedAccount,ManagedWalletInfo) with state management
Never serialize or log private keys in production; use public keys or key fingerprints for identification instead
Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations
UseBTreeMapfor ordered data (accounts, transactions) andHashMapfor lookups (address mappings); apply memory management strategies for old transaction data
Apply atomic state updates when managing watch-only wallets: validate that external signatures match expected pubkeys and never attempt signing operations
Use the?operator for error propagation, provide context in error messages, never panic in library code, and returnResult<T>for all fallible operations
Files:
key-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Files:
key-wallet/src/managed_account/mod.rs
**/*.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/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.rs
key-wallet/**/transaction_checking/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement transaction classification and routing through
TransactionRouterto avoid checking all accounts for every transaction
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/**/tests/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/tests/**/*.rs: Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Use deterministic testing with known test vectors and fixed seeds for reproducible results
Use property-based testing for complex invariants such as gap limit constraints
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
🧠 Learnings (13)
📓 Common learnings
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
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
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
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
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
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/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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/**/account/**/*.rs : Use enum-based type system for `AccountType` with specific variants (Standard, IdentityAuthentication, IdentityEncryption, MasternodeOperator, etc.) to provide compile-time safety and clear semantics
Applied to files:
key-wallet/src/managed_account/mod.rskey-wallet-ffi/src/managed_wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/wallet/balance.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 the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Applied to files:
key-wallet/src/managed_account/mod.rskey-wallet/src/wallet/balance.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/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store them in pools; only derive on-demand when the pool is exhausted
Applied to files:
key-wallet/src/managed_account/mod.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/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet-ffi/src/managed_wallet.rskey-wallet/src/wallet/balance.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/managed_wallet.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/**/transaction_checking/**/*.rs : Implement transaction classification and routing through `TransactionRouter` to avoid checking all accounts for every transaction
Applied to files:
key-wallet/src/transaction_checking/transaction_router/tests/routing.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/src/transaction_checking/transaction_router/tests/routing.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/src/transaction_checking/transaction_router/tests/routing.rs
🧬 Code graph analysis (4)
key-wallet/src/managed_account/mod.rs (1)
key-wallet/src/wallet/balance.rs (3)
unconfirmed(37-39)locked(42-44)new(23-29)
key-wallet-ffi/src/managed_wallet.rs (1)
key-wallet/src/wallet/balance.rs (1)
new(23-29)
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (4)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)key-wallet/src/managed_account/managed_account_trait.rs (1)
balance(36-36)key-wallet/src/wallet/balance.rs (1)
spendable(32-34)
key-wallet/src/wallet/balance.rs (5)
key-wallet/src/mnemonic.rs (1)
fmt(282-284)key-wallet/src/managed_account/mod.rs (2)
new(63-73)balance(845-847)key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
new(55-66)key-wallet/src/utxo.rs (2)
spendable(205-207)default(302-304)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Windows / wallet
- GitHub Check: Ubuntu ARM / spv
- GitHub Check: Ubuntu / spv
- GitHub Check: Windows / tools
- GitHub Check: Windows / core
- GitHub Check: Windows / ffi
- GitHub Check: Ubuntu ARM / wallet
- GitHub Check: Windows / spv
- GitHub Check: Ubuntu ARM / ffi
- GitHub Check: Ubuntu / wallet
- GitHub Check: macOS / core
- GitHub Check: macOS / spv
- GitHub Check: Ubuntu ARM / core
- GitHub Check: Ubuntu / core
- GitHub Check: macOS / rpc
- GitHub Check: macOS / wallet
- GitHub Check: Address Sanitizer
- GitHub Check: Thread Sanitizer
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (windows-latest)
🔇 Additional comments (7)
key-wallet-ffi/src/managed_wallet.rs (1)
1045-1045: LGTM! Correct usage of the new WalletBalance constructor.The test correctly uses the new
WalletBalance::new(spendable, unconfirmed, locked)constructor API, replacing the previous struct literal initialization. The test assertions (lines 1068-1071) properly validate that the balance components are retrieved correctly through the accessor methods.key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (1)
172-173: LGTM! Test correctly updated for new WalletBalance API.The test now uses the
spendable()accessor method instead of direct field access, aligning with the refactoredWalletBalancestructure that makes fields private.key-wallet/src/wallet/balance.rs (5)
12-19: Well-structured encapsulation.Making the balance fields private and providing accessor methods is a solid refactoring that improves API control and maintainability.
23-29: Constructor is clean and straightforward.The simple constructor properly initializes all balance components without unnecessary complexity.
31-44: Accessor methods are well-implemented.The getter methods provide clean, read-only access to the private balance fields.
54-65: Display implementation is clear and informative.The formatted output provides all balance components in a readable format, making it useful for debugging and logging.
72-88: Tests provide good coverage of the new API.The tests validate both the accessor methods and the Display implementation, ensuring the refactored interface works correctly.
xdustinface
force-pushed
the
refactor/wallet-balance
branch
from
da1bc6b to
e2c7ade
Compare
- some renaming of the fields - make the fields private - drop overflow checks and restrict total to `u64::MAX` - implement, test and use `core::fmt::Display` - simplify tests - drop unused/test-only code
xdustinface
force-pushed
the
refactor/wallet-balance
branch
from
e2c7ade to
bad5230
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/src/wallet_manager.rs (1)
659-677: Documentation and parameter naming inconsistency.The function comment (line 661) states "confirmed and unconfirmed balance" and the parameter is named
confirmed_out(line 675), but the code now retrievesspendablebalance (line 706). This creates confusion about what is actually being returned.Update the documentation and parameter name to reflect that this returns
spendablebalance, notconfirmedbalance (though they may be semantically equivalent in practice).🔎 Suggested fix
/// Get wallet balance /// -/// Returns the confirmed and unconfirmed balance for a specific wallet +/// Returns the spendable and unconfirmed balance for a specific wallet /// /// # Safety /// /// - `manager` must be a valid pointer to an FFIWalletManager instance /// - `wallet_id` must be a valid pointer to a 32-byte wallet ID -/// - `confirmed_out` must be a valid pointer to a u64 (maps to C uint64_t) +/// - `spendable_out` must be a valid pointer to a u64 (maps to C uint64_t) /// - `unconfirmed_out` must be a valid pointer to a u64 (maps to C uint64_t) /// - `error` must be a valid pointer to an FFIError structure or null /// - The caller must ensure all pointers remain valid for the duration of this call #[no_mangle] pub unsafe extern "C" fn wallet_manager_get_wallet_balance( manager: *const FFIWalletManager, wallet_id: *const u8, - confirmed_out: *mut u64, + spendable_out: *mut u64, unconfirmed_out: *mut u64, error: *mut FFIError, ) -> bool { if manager.is_null() || wallet_id.is_null() - || confirmed_out.is_null() + || spendable_out.is_null() || unconfirmed_out.is_null()key-wallet-ffi/src/types.rs (1)
54-61: Update documentation to reflect actual field semantics.The documentation comments no longer accurately describe the field values after the implementation changes:
- Line 54-55:
confirmedis documented as "Confirmed balance" but now contains spendable balance (Line 67:balance.spendable()).- Line 60:
totalis documented as "confirmed + unconfirmed" but should include the immature/locked component as well.🔎 Suggested documentation fix
- /// Confirmed balance in duffs + /// Spendable balance in duffs (confirmed and unlocked) pub confirmed: u64, /// Unconfirmed balance in duffs pub unconfirmed: u64, /// Immature balance in duffs (e.g., mining rewards) pub immature: u64, - /// Total balance (confirmed + unconfirmed) in duffs + /// Total balance (spendable + unconfirmed + immature) in duffs pub total: u64,
🧹 Nitpick comments (1)
key-wallet/src/wallet/balance.rs (1)
46-51: Consider logging when clamping occurs.The silent clamping to
u64::MAXis pragmatic since this exceeds Dash's max supply (≈184M Dash vs 18.9M max supply). However, if clamping ever occurs, it likely indicates a bug (e.g., inflated balance calculation).Consider adding a debug log or trace when the sum exceeds
u64::MAXto aid in debugging such issues, though this is optional given the practical impossibility of reaching this limit.🔎 Optional enhancement to add debug logging
pub fn total(&self) -> u64 { // For safety restrict the total balance to u64::MAX, in practice this should never be relevant. let total = self.spendable as u128 + self.unconfirmed as u128 + self.locked as u128; + if total > u64::MAX as u128 { + #[cfg(feature = "std")] + eprintln!("Warning: Balance total exceeds u64::MAX, clamping to u64::MAX"); + } total.min(u64::MAX as u128) as u64 }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
dash-spv/src/main.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/lib.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/balance.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
💤 Files with no reviewable changes (1)
- key-wallet/src/wallet/managed_wallet_info/mod.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- key-wallet-manager/src/wallet_manager/mod.rs
- key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
- key-wallet/src/wallet/mod.rs
- dash-spv/src/main.rs
- key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-ffi/src/managed_wallet.rs
- key-wallet/src/lib.rs
- key-wallet-manager/examples/wallet_creation.rs
🧰 Additional context used
📓 Path-based instructions (6)
key-wallet/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
key-wallet/**/*.rs: Separate immutable structures (Account,Wallet) containing only identity information from mutable wrappers (ManagedAccount,ManagedWalletInfo) with state management
Never serialize or log private keys in production; use public keys or key fingerprints for identification instead
Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations
UseBTreeMapfor ordered data (accounts, transactions) andHashMapfor lookups (address mappings); apply memory management strategies for old transaction data
Apply atomic state updates when managing watch-only wallets: validate that external signatures match expected pubkeys and never attempt signing operations
Use the?operator for error propagation, provide context in error messages, never panic in library code, and returnResult<T>for all fallible operations
Files:
key-wallet/src/managed_account/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rs
key-wallet/**/managed_account/**/*.rs
📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)
Implement atomic state updates when processing transactions: update transactions, UTXOs, balances, and address usage state together
Files:
key-wallet/src/managed_account/mod.rs
**/*.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/src/managed_account/mod.rskey-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
key-wallet-manager/tests/integration_test.rs
**/*integration*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write integration tests for network operations
Files:
key-wallet-manager/tests/integration_test.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-manager/tests/integration_test.rs
🧠 Learnings (17)
📓 Common learnings
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
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.
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
📚 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/src/managed_account/mod.rskey-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.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/src/managed_account/mod.rskey-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.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/src/managed_account/mod.rskey-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.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/src/managed_account/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.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/**/account/**/*.rs : Use enum-based type system for `AccountType` with specific variants (Standard, IdentityAuthentication, IdentityEncryption, MasternodeOperator, etc.) to provide compile-time safety and clear semantics
Applied to files:
key-wallet/src/managed_account/mod.rskey-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
key-wallet/src/managed_account/mod.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/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store them in pools; only derive on-demand when the pool is exhausted
Applied to files:
key-wallet/src/managed_account/mod.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.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/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet-ffi/src/types.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-manager/tests/integration_test.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/src/wallet/managed_wallet_info/wallet_info_interface.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 the `?` operator for error propagation, provide context in error messages, never panic in library code, and return `Result<T>` for all fallible operations
Applied to files:
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.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/**/transaction_checking/**/*.rs : Implement transaction classification and routing through `TransactionRouter` to avoid checking all accounts for every transaction
Applied to files:
key-wallet/src/wallet/balance.rskey-wallet-ffi/src/wallet_manager.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/wallet_manager.rskey-wallet-manager/tests/integration_test.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-manager/tests/integration_test.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-manager/tests/integration_test.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-manager/tests/integration_test.rs
🧬 Code graph analysis (6)
key-wallet/src/managed_account/mod.rs (1)
key-wallet/src/wallet/balance.rs (4)
spendable(32-34)unconfirmed(37-39)locked(42-44)new(23-29)
key-wallet-ffi/src/types.rs (3)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)key-wallet/src/managed_account/managed_account_trait.rs (1)
balance(36-36)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
key-wallet/src/wallet/balance.rs (4)
spendable(32-34)unconfirmed(37-39)new(23-29)locked(42-44)
key-wallet/src/wallet/balance.rs (2)
key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
new(55-66)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)
key-wallet-ffi/src/wallet_manager.rs (3)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)key-wallet/src/managed_account/managed_account_trait.rs (1)
balance(36-36)
key-wallet-manager/tests/integration_test.rs (5)
key-wallet/src/managed_account/mod.rs (1)
balance(845-847)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
balance(77-77)balance(200-202)key-wallet/src/managed_account/managed_account_trait.rs (1)
balance(36-36)key-wallet/src/wallet/balance.rs (1)
total(47-51)dash-spv/src/types.rs (1)
total(696-698)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Ubuntu ARM / rpc
- GitHub Check: Windows / spv
- GitHub Check: macOS / tools
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Address Sanitizer
- GitHub Check: Thread Sanitizer
🔇 Additional comments (10)
key-wallet/src/wallet/balance.rs (4)
12-29: LGTM: Clean encapsulation with private fields.The refactor to private fields with a simple constructor aligns well with the immutable value pattern. The constructor is straightforward without unnecessary error handling, which is appropriate for this internal type.
Based on learnings, this follows the pattern of separating immutable structures from mutable wrappers.
31-44: LGTM: Clean getter methods.The getter methods provide straightforward read-only access to the private fields with consistent naming.
54-65: LGTM: Clean Display implementation.The Display implementation provides a clear, human-readable format for debugging and logging purposes.
67-89: LGTM: Tests cover basic functionality.The tests adequately verify the constructor, getter methods, and Display implementation for typical use cases.
key-wallet-manager/tests/integration_test.rs (2)
129-129: LGTM: Correctly updated to use total() method.The change from field access to method call aligns with the new WalletBalance API.
148-148: LGTM: Correctly updated to use total() method.The change from field access to method call aligns with the new WalletBalance API.
key-wallet-ffi/src/wallet_manager.rs (1)
704-710: LGTM: Correctly updated to use spendable() accessor.The code correctly uses
balance.spendable()andbalance.unconfirmed()accessor methods, aligning with the new WalletBalance API.key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
204-223: LGTM: Consistent semantic shift from confirmed to spendable.The renaming from
confirmedtospendableimproves clarity while maintaining the same logic. UTXOs that are confirmed and not locked are correctly accumulated as spendable balance. The constructor call aligns with the newWalletBalance::new(spendable, unconfirmed, locked)signature.key-wallet/src/managed_account/mod.rs (1)
267-271: LGTM: Parameter naming now consistent with WalletBalance API.The parameter name change from
confirmedtospendablecorrectly aligns with theWalletBalance::new(spendable, unconfirmed, locked)constructor signature. This addresses the inconsistency noted in previous review feedback.The removal of error handling is appropriate since the new
WalletBalanceconstructor no longer returns aResult.key-wallet-ffi/src/types.rs (1)
67-70: The identified mapping issue in line 69 is dead code and does not affect actual FFI behavior.The
FFIBalance::from()implementation (lines 64-72) that mapslocked → immatureis never invoked anywhere in the codebase. The only FFI function that returns balance information—managed_account_get_balance()—manually constructsFFIBalanceand setsimmature = 0, not tolocked.The
confirmedfield mapping tobalance.spendable()is intentional and semantically correct:spendablerepresents "confirmed and mature balance" per theWalletBalancedocumentation, making this an appropriate semantic choice rather than a breaking change. All FFI functions consistently use this mapping.The unused
Fromimplementation with the "for now" comment suggests incomplete refactoring, but it poses no runtime impact since it's never called.Likely an incorrect or invalid review comment.
| pub fn total(&self) -> u64 { | ||
| // For safety restrict the total balance to u64::MAX, in practice this should never be relevant. | ||
| let total = self.spendable as u128 + self.unconfirmed as u128 + self.locked as u128; | ||
| total.min(u64::MAX as u128) as u64 |
There was a problem hiding this comment.
Srry to insist, but I want to make sure we get this right and I took some time to do some math
The theoretical max supply is ~18.9M DASH. Multiplied by 100,000,000 duffs, that is 1,890,000,000,000,000.
u64::MAX is 18,446,744,073,709,551,615, which means u64 can represent ~184.4 billion DASH (roughly 9,760× the maximum possible supply). Given this margin, a simple addition of the balances cannot overflow in any valid Dash scenario. The current implementation clamps the result to u64::MAX, which in practice hides potential bugs instead of surfacing them
I understand the intent of defensive programming, but if the goal is to support arbitrary or unbounded values, wouldn’t an explicit big-integer representation be more appropriate than silently saturating the result??
There was a problem hiding this comment.
The big integer would just move the problem up to the boundaries where we want u64 again like in format_balance were we print the formatted total balance. In cases like this you need to bring it down to u64 again somehow. Not that this case is very important and it's anyway buggy at the moment but generally mixing up integer types for balances just seems weird to me. Ideally we would use Amount type for all balances at some point.
I would rather bring the error handling back or just let it panic since its so unlikely to ever come up but then again i wonder, whats the potential bug we would hide with clamping it to to u64::MAX that we would not catch opposed to when we would have u128? Either way in practice we should never come anywhere near the limit so in the case of a bug we would have some huge number in there which will cause attention. Im not sure if mixing up the balance types just for the sake of knowing its u64::MAX or u64::MAX + n really brings any benefit here.
u64::MAXcore::fmt::DisplaySummary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.