refactor(key-wallet): split ManagedCoreAccount into funds + keys variants#711
Open
QuantumExplorer wants to merge 3 commits intov0.42-devfrom
Open
refactor(key-wallet): split ManagedCoreAccount into funds + keys variants#711QuantumExplorer wants to merge 3 commits intov0.42-devfrom
QuantumExplorer wants to merge 3 commits intov0.42-devfrom
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis PR refactors the monolithic ChangesAccount Type Refactoring & Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 58 minutes and 54 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/IMPLEMENTATION_SUMMARY.md`:
- Around line 183-186: The example uses the old method
get_next_receive_address() which no longer exists on ManagedCoreFundsAccount;
update the call to the renamed API (replace get_next_receive_address() with the
current method on ManagedCoreFundsAccount, e.g., get_receive_address()) and add
the necessary import for ManagedCoreFundsAccount so the snippet is
copy-pasteable; ensure you call ManagedCoreFundsAccount::from_account(&account)
followed by the new instance method name instead of get_next_receive_address().
In `@key-wallet/src/managed_account/managed_core_funds_account.rs`:
- Around line 421-452: confirm_transaction currently only updates context and
UTXOs for existing TransactionRecord, so changes in account_match from gap-limit
rescans leave net_amount/direction and input/output roles stale; modify
confirm_transaction (and use TransactionRecord/record_transaction helpers) to
detect when the stored match differs from the new account_match and refresh the
stored TransactionRecord's derived metadata (net_amount, direction, input/output
role flags) by re-running the same computation used in record_transaction (or by
replacing the entry with a newly constructed TransactionRecord while preserving
transaction_type and context), then continue to update UTXOs and return changed
appropriately; refer to confirm_transaction, TransactionRecord,
record_transaction, update_utxos, and account_match to locate code to change.
- Around line 296-304: The pool-mutating helpers (e.g., mark_address_used, batch
derivation helpers, non-standard next_address/_with_info paths, provider-key
helpers) must bump the monitor revision just like
next_receive_address/next_change_address do; update each of these functions
(including mark_address_used and the batch/alternate derivation functions
referenced in the review) to call the same monitor_revision invalidation logic
(increment the metadata.monitor_revision or call the existing helper that bumps
it and sets last_used) immediately when they extend or mutate the watched
address pools, before returning, so the mempool/filter consumers will refresh
their watched set; locate calls to managed_account_type.mark_address_used, the
batch derivation routines, and any provider-key derivation helpers and add the
identical revision-bump step there.
In `@key-wallet/src/managed_account/managed_core_keys_account.rs`:
- Around line 257-261: The account mutation methods must bump the
monitor_revision when they actually change address/pool state: update
mark_address_used (and similarly next_address, next_address_with_info, and any
provider-key helper that mutates pool state) to increment or set
self.metadata.monitor_revision (e.g., to Self::current_timestamp() or
self.metadata.monitor_revision.wrapping_add(1)) only when the underlying
mutation returned true/succeeded; locate the mutation by the methods
managed_account_type.mark_address_used, next_address, next_address_with_info,
and provider-key helper calls, and add the monitor_revision update immediately
after a successful mutation return path so the monitor sees the new address set.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 19-27: Keep a compatibility alias named ManagedCoreAccount
pointing to ManagedCoreFundsAccount and mark it deprecated for one release
cycle: add a public alias (pub type ManagedCoreAccount =
ManagedCoreFundsAccount) with a #[deprecated(...)] attribute and a short note
steering callers to ManagedCoreFundsAccount; place this alongside the existing
pub use ManagedCoreFundsAccount export in mod.rs so existing imports continue to
work while signalling the new name.
In `@key-wallet/src/tests/spent_outpoints_tests.rs`:
- Around line 90-95: The test only checks serde round-trips but not that the
private spent_outpoints set is actually rebuilt; change one of the
post-deserialization assertions to exercise behavior that depends on
spent_outpoints (e.g., attempt to apply/insert a duplicate spend and assert it
is rejected or not considered spendable). After deserializing into
ManagedCoreFundsAccount, call the public API that records or validates spends
(use the existing methods on ManagedCoreFundsAccount such as the
transaction-add/apply method or a spendability check) to try to spend the same
outpoint twice and assert the second spend fails or reports unspendable; this
will prove spent_outpoints was reconstructed. Ensure you replace one of the
existing serde assertions (like the transactions.len() check) with this
duplicate-spend/spendability assertion and leave the other serde checks intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06e6eb8a-3905-4949-aa3f-e042afddfa66
📒 Files selected for processing (23)
dash-spv/src/sync/mempool/sync_manager.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/utxo_tests.rskey-wallet/CLAUDE.mdkey-wallet/IMPLEMENTATION_SUMMARY.mdkey-wallet/README.mdkey-wallet/src/account/mod.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/managed_account/managed_core_keys_account.rskey-wallet/src/managed_account/managed_platform_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/account.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/helpers.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rs
Comment on lines
+296
to
+304
| /// Mark an address as used | ||
| pub fn mark_address_used(&mut self, address: &Address) -> bool { | ||
| // Update metadata timestamp | ||
| self.metadata.last_used = Some(Self::current_timestamp()); | ||
|
|
||
| // Use the account type's mark_address_used method | ||
| // The address pools already track gap limits internally | ||
| self.managed_account_type.mark_address_used(address) | ||
| } |
Contributor
There was a problem hiding this comment.
Pool-mutating derivation helpers need the same monitor_revision invalidation as the standard single-address helpers.
next_receive_address/next_change_address already bump the revision, but the batch helpers, non-standard next_address(_with_info) paths, provider-key helpers, and mark_address_used do not. If any of those extend the watched set, mempool/filter consumers will keep monitoring the old address set until some later mutation happens to bump the counter.
Also applies to: 709-788, 793-1068
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
296 - 304, The pool-mutating helpers (e.g., mark_address_used, batch derivation
helpers, non-standard next_address/_with_info paths, provider-key helpers) must
bump the monitor revision just like next_receive_address/next_change_address do;
update each of these functions (including mark_address_used and the
batch/alternate derivation functions referenced in the review) to call the same
monitor_revision invalidation logic (increment the metadata.monitor_revision or
call the existing helper that bumps it and sets last_used) immediately when they
extend or mutate the watched address pools, before returning, so the
mempool/filter consumers will refresh their watched set; locate calls to
managed_account_type.mark_address_used, the batch derivation routines, and any
provider-key derivation helpers and add the identical revision-bump step there.
Member
Author
There was a problem hiding this comment.
Tracked as #713 — agreed it's a real gap, just out of scope for this structural-only PR.
Contributor
There was a problem hiding this comment.
@QuantumExplorer, understood — thanks for the confirmation! I'll keep the comment open until #713 is resolved. The structural refactor looks clean as-is.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 711
File: key-wallet/src/managed_account/mod.rs:19-27
Timestamp: 2026-05-03T16:28:16.898Z
Learning: In the `key-wallet` crate, when reviewing the `managed_account` module for type renames/splits (e.g., `ManagedCoreAccount` -> `ManagedCoreFundsAccount`, plus separate “funds-free” variants), avoid recommending deprecated compatibility type aliases for the old Rust types. This code is intended for in-tree Rust use only (no external Rust consumers), and callsites are expected to be migrated in the same PR. Backward compatibility is handled separately at the C/FFI layer, which should remain unchanged by these Rust-only renames.
Comment on lines
+421
to
+452
| pub(crate) fn confirm_transaction( | ||
| &mut self, | ||
| tx: &Transaction, | ||
| account_match: &AccountMatch, | ||
| context: TransactionContext, | ||
| transaction_type: TransactionType, | ||
| ) -> bool { | ||
| if !self.transactions.contains_key(&tx.txid()) { | ||
| self.record_transaction(tx, account_match, context, transaction_type); | ||
| return true; | ||
| } | ||
|
|
||
| let mut changed = false; | ||
| if let Some(tx_record) = self.transactions.get_mut(&tx.txid()) { | ||
| debug_assert_eq!( | ||
| tx_record.transaction_type, | ||
| transaction_type, | ||
| "transaction_type changed between recordings for {}", | ||
| tx.txid() | ||
| ); | ||
| if tx_record.context != context { | ||
| let was_confirmed = tx_record.context.confirmed(); | ||
| tx_record.update_context(context.clone()); | ||
| // Only signal a change when confirmation status actually changes, | ||
| // not for upgrades within the confirmed state (e.g. InBlock → InChainLockedBlock). | ||
| // TODO: emit a change event for InBlock → InChainLockedBlock once chainlock | ||
| // wallet transaction events are properly handled | ||
| changed = !was_confirmed; | ||
| } | ||
| } | ||
| self.update_utxos(tx, account_match, context); | ||
| changed |
Contributor
There was a problem hiding this comment.
Refresh the stored TransactionRecord when rescans change account_match.
This method claims to handle “potentially new address matches from gap limit rescans”, but for an already-recorded tx it only updates context and UTXOs. net_amount, direction, and the input/output roles stay frozen from the first recording, so a tx confirmed after a gap-limit rescan can still show stale wallet metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
421 - 452, confirm_transaction currently only updates context and UTXOs for
existing TransactionRecord, so changes in account_match from gap-limit rescans
leave net_amount/direction and input/output roles stale; modify
confirm_transaction (and use TransactionRecord/record_transaction helpers) to
detect when the stored match differs from the new account_match and refresh the
stored TransactionRecord's derived metadata (net_amount, direction, input/output
role flags) by re-running the same computation used in record_transaction (or by
replacing the entry with a newly constructed TransactionRecord while preserving
transaction_type and context), then continue to update UTXOs and return changed
appropriately; refer to confirm_transaction, TransactionRecord,
record_transaction, update_utxos, and account_match to locate code to change.
Member
Author
There was a problem hiding this comment.
Tracked as #714 — agreed, but out of scope for this structural-only PR (the function is moved verbatim here).
Contributor
There was a problem hiding this comment.
@QuantumExplorer, understood — thanks for tracking it in #714. Since this is a verbatim move with no behavioral changes, deferring the fix is the right call for this structural PR.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 711
File: key-wallet/src/managed_account/mod.rs:19-27
Timestamp: 2026-05-03T16:28:16.898Z
Learning: In the `key-wallet` crate, when reviewing the `managed_account` module for type renames/splits (e.g., `ManagedCoreAccount` -> `ManagedCoreFundsAccount`, plus separate “funds-free” variants), avoid recommending deprecated compatibility type aliases for the old Rust types. This code is intended for in-tree Rust use only (no external Rust consumers), and callsites are expected to be migrated in the same PR. Backward compatibility is handled separately at the C/FFI layer, which should remain unchanged by these Rust-only renames.
Comment on lines
+257
to
+261
| /// Mark an address as used | ||
| pub fn mark_address_used(&mut self, address: &Address) -> bool { | ||
| self.metadata.last_used = Some(Self::current_timestamp()); | ||
| self.managed_account_type.mark_address_used(address) | ||
| } |
Contributor
There was a problem hiding this comment.
Bump monitor_revision on successful key/address-pool mutations.
These paths can grow or otherwise change the monitored address set, but none of them invalidate monitor_revision. That means a keys-only account can derive fresh identity/provider addresses and still keep watching the old filter until some unrelated code bumps the revision manually.
Suggested direction
pub fn mark_address_used(&mut self, address: &Address) -> bool {
self.metadata.last_used = Some(Self::current_timestamp());
- self.managed_account_type.mark_address_used(address)
+ let changed = self.managed_account_type.mark_address_used(address);
+ if changed {
+ self.bump_monitor_revision();
+ }
+ changed
}Apply the same pattern to next_address, next_address_with_info, and the provider-key helpers whenever they actually mutate pool state.
Also applies to: 283-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_keys_account.rs` around lines 257
- 261, The account mutation methods must bump the monitor_revision when they
actually change address/pool state: update mark_address_used (and similarly
next_address, next_address_with_info, and any provider-key helper that mutates
pool state) to increment or set self.metadata.monitor_revision (e.g., to
Self::current_timestamp() or self.metadata.monitor_revision.wrapping_add(1))
only when the underlying mutation returned true/succeeded; locate the mutation
by the methods managed_account_type.mark_address_used, next_address,
next_address_with_info, and provider-key helper calls, and add the
monitor_revision update immediately after a successful mutation return path so
the monitor sees the new address set.
Member
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
@QuantumExplorer, understood — that's a sensible sequencing. Keeping the keys variant's behavior in sync with the funds variant once #713 lands (and before this struct is wired into the collection) is cleaner than a partial fix now. I'll track this against #715.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 711
File: key-wallet/src/managed_account/mod.rs:19-27
Timestamp: 2026-05-03T16:28:16.898Z
Learning: In the `key-wallet` crate, when reviewing the `managed_account` module for type renames/splits (e.g., `ManagedCoreAccount` -> `ManagedCoreFundsAccount`, plus separate “funds-free” variants), avoid recommending deprecated compatibility type aliases for the old Rust types. This code is intended for in-tree Rust use only (no external Rust consumers), and callsites are expected to be migrated in the same PR. Backward compatibility is handled separately at the C/FFI layer, which should remain unchanged by these Rust-only renames.
Comment on lines
90
to
95
| // Verify the rebuilt set by serializing again and comparing transactions | ||
| // (spent_outpoints is private, so we test behavior through a second round-trip | ||
| // to confirm stability) | ||
| let json2 = serde_json::to_string(&deserialized).unwrap(); | ||
| let deserialized2: ManagedCoreAccount = serde_json::from_str(&json2).unwrap(); | ||
| let deserialized2: ManagedCoreFundsAccount = serde_json::from_str(&json2).unwrap(); | ||
| assert_eq!(deserialized2.transactions.len(), 1); |
Contributor
There was a problem hiding this comment.
Exercise the rebuilt spent set directly.
These assertions only prove that transaction inputs survive serde; they would still pass if spent_outpoints stopped being reconstructed. Please switch one of the checks to behavior that depends on the private spent set (e.g. a duplicate-spend/spendability assertion).
Also applies to: 137-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/tests/spent_outpoints_tests.rs` around lines 90 - 95, The test
only checks serde round-trips but not that the private spent_outpoints set is
actually rebuilt; change one of the post-deserialization assertions to exercise
behavior that depends on spent_outpoints (e.g., attempt to apply/insert a
duplicate spend and assert it is rejected or not considered spendable). After
deserializing into ManagedCoreFundsAccount, call the public API that records or
validates spends (use the existing methods on ManagedCoreFundsAccount such as
the transaction-add/apply method or a spendability check) to try to spend the
same outpoint twice and assert the second spend fails or reports unspendable;
this will prove spent_outpoints was reconstructed. Ensure you replace one of the
existing serde assertions (like the transactions.len() check) with this
duplicate-spend/spendability assertion and leave the other serde checks intact.
This was referenced May 3, 2026
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
…ants Move the existing struct out of `managed_account/mod.rs` into a dedicated `managed_core_funds_account.rs` and rename it to `ManagedCoreFundsAccount` to make room for a sibling `ManagedCoreKeysAccount` that omits funds-tracking state (`balance`, `utxos`, `spent_outpoints`). The keys variant is intended for accounts that derive special-purpose keys (identity registration, asset locks, masternode provider keys) but is not yet wired into `ManagedAccountCollection` — that migration follows in a subsequent PR. This change is purely structural: `ManagedAccountCollection` continues to store `ManagedCoreFundsAccount` for every account type and behavior is unchanged. The C ABI is preserved — `FFIManagedCoreAccount` and the `managed_core_account_*` C functions keep their original names and now wrap the renamed Rust struct internally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Account - Sort imports and rewrap long signatures (rustfmt fixes flagged by pre-commit) - Qualify the `[\`ManagedCoreFundsAccount\`]` intra-doc links in `managed_core_keys_account.rs` to `[\`crate::managed_account::ManagedCoreFundsAccount\`]` so rustdoc can resolve them under `-D warnings` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…N_SUMMARY The example called `get_next_receive_address()`, which is not a method on this struct. Replace with the real `next_receive_address(...)` signature so the snippet compiles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/adoring-booth-151626
branch
from
2b9fbad to
425bc19
Compare
github-actions
Bot
removed
the
merge-conflict
6 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Structural refactor of
key-wallet's managed-account types in preparation for distinguishing accounts that hold and spend funds from accounts that exist only to derive special-purpose keys.ManagedCoreAccountstruct out ofmanaged_account/mod.rsinto a dedicatedmanaged_core_funds_account.rsand renamed it toManagedCoreFundsAccount.ManagedCoreKeysAccountinmanaged_core_keys_account.rsthat mirrors the funds variant but omitsbalance,utxos, andspent_outpoints. Intended for accounts that primarily derive keys for special-purpose flows (identity registration, asset locks, masternode provider keys).mod.rsis now a thin module file containing onlypub moddeclarations and re-exports.Scope
This PR is purely structural — no behavior change.
ManagedAccountCollectioncontinues to storeManagedCoreFundsAccountfor every field. WiringManagedCoreKeysAccountinto the identity / asset-lock / provider fields (and the resulting trait/accessor/transaction-checking changes) is queued for a follow-up PR.FFIManagedCoreAccount,FFIManagedCoreAccountResult,FFIManagedCoreAccountCollection) and themanaged_core_account_*C function names are unchanged. They now wrap the renamed Rust struct internally — invisible to FFI consumers.ManagedCoreAccount→ManagedCoreFundsAccountrename is applied acrosskey-wallet,key-wallet-ffi,key-wallet-manager,dash-spv,dash-spv-ffi, plus the README / CLAUDE.md / IMPLEMENTATION_SUMMARY.md doc files inkey-wallet.Test plan
cargo check --workspace --all-targets— cleancargo test -p key-wallet --lib— 461 passed, 0 failed🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation