Skip to content

feat(key-wallet): track internal address pools for CoinJoin accounts#821

Merged
xdustinface merged 1 commit into
devfrom
feat/coinjoin-external
Jun 26, 2026
Merged

feat(key-wallet): track internal address pools for CoinJoin accounts#821
xdustinface merged 1 commit into
devfrom
feat/coinjoin-external

Conversation

@ZocoLini

@ZocoLini ZocoLini commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features

    • CoinJoin accounts now track separate external and internal address pools, matching how addresses are used in practice.
    • Address selection, routing, and transaction checks now recognize both receive and change branches for CoinJoin accounts.
  • Bug Fixes

    • Improved address classification so CoinJoin addresses can be identified as external, internal, or other more accurately.
    • FFI access to address pools now returns the correct pool for more account types and scenarios.

@ZocoLini
ZocoLini requested a review from xdustinface June 24, 2026 10:17
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@ZocoLini, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 34 minutes and 42 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f929f732-d621-401e-abcf-ccf82c1fc59e

📥 Commits

Reviewing files that changed from the base of the PR and between 2f6e1f7 and 23d0260.

📒 Files selected for processing (10)
  • key-wallet-ffi/src/managed_account.rs
  • key-wallet-ffi/src/utxo_tests.rs
  • key-wallet/src/managed_account/managed_account_collection.rs
  • key-wallet/src/managed_account/managed_account_trait.rs
  • key-wallet/src/managed_account/managed_account_type.rs
  • key-wallet/src/tests/account_tests.rs
  • key-wallet/src/transaction_checking/account_checker.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/conversions.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
  • key-wallet/src/transaction_checking/wallet_checker.rs
📝 Walkthrough

Walkthrough

ManagedAccountType::CoinJoin is changed from holding a single addresses pool to holding separate external_addresses (child index 0) and internal_addresses (child index 1) pools. This struct change is propagated through pool accessors, trait methods, address classification, the FFI address-pool selector, and all affected tests.

Changes

CoinJoin Dual-Pool Refactor

Layer / File(s) Summary
CoinJoin variant: dual-pool struct definition and pool accessors
key-wallet/src/managed_account/managed_account_type.rs
Redefines ManagedAccountType::CoinJoin with external_addresses and internal_addresses fields. Updates address_pools, address_pools_mut, and from_account_type to derive two pools from child indices 0 and 1 and return both.
Trait methods and collection derivation updated for dual pools
key-wallet/src/managed_account/managed_account_collection.rs, key-wallet/src/managed_account/managed_account_trait.rs
ManagedAccountCollection::build_managed_account_type now derives both pools for CoinJoin. Four ManagedAccountTrait default-method match arms (get_next_address_index, next_address, next_address_with_info, gap_limit) bind addresses from external_addresses.
Address classification and FFI pool selection for CoinJoin
key-wallet/src/transaction_checking/account_checker.rs, key-wallet-ffi/src/managed_account.rs
classify_address merges the CoinJoin match arm with Standard, enabling Internal classification. FFI managed_core_account_get_address_pool now serves External/Internal pool types for both Standard and CoinJoin, and the FFIAddressPool wrapper is constructed once at the match end.
Test fixtures updated to dual-pool CoinJoin
key-wallet/src/tests/account_tests.rs, key-wallet/src/transaction_checking/transaction_router/tests/conversions.rs, key-wallet/src/transaction_checking/transaction_router/tests/routing.rs, key-wallet/src/transaction_checking/wallet_checker.rs, key-wallet-ffi/src/utxo_tests.rs
All test fixtures and destructuring patterns updated to construct ManagedAccountType::CoinJoin with external_addresses and internal_addresses, and to derive addresses from external_addresses.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • dashpay/rust-dashcore#804: Introduced configurable CoinJoin gap limits and modified ManagedAccountType::from_account_type CoinJoin derivation, which is the same code path this PR extends with dual-pool support.

Suggested labels

ready-for-review

Suggested reviewers

  • xdustinface

🐇 Two pools now bloom where once was one,
External and internal, derivation done!
CoinJoin hops down both branch 0 and 1,
Addresses sorted beneath a clear sun.
The rabbit rejoices — dual pools have won! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: CoinJoin accounts now track internal address pools.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/coinjoin-external

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
key-wallet/src/managed_account/managed_account_collection.rs (1)

588-613: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consolidate CoinJoin pool construction into one helper.

This branch now duplicates the same dual-pool CoinJoin build logic in ManagedAccountType::from_account_type, and those two constructors have already drifted in adjacent Standard-path error handling. Keeping both copies in sync is going to be fragile. Extract the pool-building logic once and have both call sites delegate to it.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@key-wallet/src/managed_account/managed_account_collection.rs` around lines
588 - 613, The CoinJoin dual-pool construction logic is duplicated and should be
centralized to avoid future drift. Extract the shared external/internal
AddressPool setup into a single helper used by
ManagedAccountType::from_account_type and the other CoinJoin constructor path,
keeping the base_path cloning, ChildNumber indices, AddressPoolType values, gap
limit, network, and key_source wiring in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@key-wallet/src/transaction_checking/account_checker.rs`:
- Around line 559-563: The CoinJoin address matching flow is now classifying
internal addresses, but they are not included in the returned payload. Update
the CoinJoin branch in account_checker.rs, specifically the
ManagedAccountType::CoinJoin handling and the CoreAccountTypeMatch::CoinJoin
builder, so the internal_addresses collection is carried through and merged
alongside the receive/other addresses instead of being dropped. Verify the
returned CoinJoin match data includes external, internal, and any other
classified addresses consistently.

---

Nitpick comments:
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 588-613: The CoinJoin dual-pool construction logic is duplicated
and should be centralized to avoid future drift. Extract the shared
external/internal AddressPool setup into a single helper used by
ManagedAccountType::from_account_type and the other CoinJoin constructor path,
keeping the base_path cloning, ChildNumber indices, AddressPoolType values, gap
limit, network, and key_source wiring in one place.
🪄 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: 219b56cf-621c-400d-82dd-d66c78cf142f

📥 Commits

Reviewing files that changed from the base of the PR and between d9fe0a3 and 2f6e1f7.

📒 Files selected for processing (10)
  • key-wallet-ffi/src/managed_account.rs
  • key-wallet-ffi/src/utxo_tests.rs
  • key-wallet/src/managed_account/managed_account_collection.rs
  • key-wallet/src/managed_account/managed_account_trait.rs
  • key-wallet/src/managed_account/managed_account_type.rs
  • key-wallet/src/tests/account_tests.rs
  • key-wallet/src/transaction_checking/account_checker.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/conversions.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
  • key-wallet/src/transaction_checking/wallet_checker.rs

Comment thread key-wallet/src/transaction_checking/account_checker.rs
@ZocoLini
ZocoLini force-pushed the feat/coinjoin-external branch from 2f6e1f7 to 2d0a191 Compare June 24, 2026 10:28
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 24, 2026
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.56522% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.01%. Comparing base (d9fe0a3) to head (23d0260).

Files with missing lines Patch % Lines
key-wallet-ffi/src/managed_account.rs 30.43% 16 Missing ⚠️
...allet/src/managed_account/managed_account_trait.rs 25.00% 3 Missing ⚠️
.../src/managed_account/managed_account_collection.rs 92.85% 1 Missing ⚠️
...wallet/src/managed_account/managed_account_type.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #821      +/-   ##
==========================================
- Coverage   73.04%   73.01%   -0.03%     
==========================================
  Files         323      323              
  Lines       72046    72064      +18     
==========================================
- Hits        52623    52619       -4     
- Misses      19423    19445      +22     
Flag Coverage Δ
core 76.74% <ø> (ø)
ffi 47.35% <30.43%> (-0.01%) ⬇️
rpc 20.00% <ø> (ø)
spv 90.17% <ø> (-0.14%) ⬇️
wallet 71.85% <89.13%> (+0.02%) ⬆️
Files with missing lines Coverage Δ
...wallet/src/transaction_checking/account_checker.rs 53.98% <100.00%> (-0.05%) ⬇️
...-wallet/src/transaction_checking/wallet_checker.rs 99.22% <100.00%> (+<0.01%) ⬆️
.../src/managed_account/managed_account_collection.rs 58.83% <92.85%> (+0.34%) ⬆️
...wallet/src/managed_account/managed_account_type.rs 41.29% <95.23%> (+1.62%) ⬆️
...allet/src/managed_account/managed_account_trait.rs 36.80% <25.00%> (ø)
key-wallet-ffi/src/managed_account.rs 63.52% <30.43%> (+0.08%) ⬆️

... and 7 files with indirect coverage changes

@xdustinface
xdustinface merged commit 8c7b501 into dev Jun 26, 2026
41 checks passed
@xdustinface
xdustinface deleted the feat/coinjoin-external branch June 26, 2026 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants