refactor(key-wallet): ManagedAccountType::from_account_type refactor#828
refactor(key-wallet): ManagedAccountType::from_account_type refactor#828ZocoLini wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo new ChangesAddressPool Construction Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/managed_account/managed_account_collection.rs (1)
551-558: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the platform key matches the derivation account type.
This derives the address pool from
account.account_type, but the returnedManagedPlatformAccountis labeled withkey.account/key.key_class. If those diverge, the account watches the wrong DIP-17 path under the wrong identity.Proposed fix
// Use the account's existing public key let key_source = KeySource::Public(account.account_xpub); + let expected_account_type = crate::account::AccountType::PlatformPayment { + account: key.account, + key_class: key.key_class, + }; + if account.account_type != expected_account_type { + return Err(crate::error::Error::InvalidParameter( + "Platform payment account key does not match account_type".into(), + )); + } + // DIP-17 Platform Payment addresses: single pool on the account's path. let addresses = ManagedAccountType::single_pool( - account.account_type, + expected_account_type, AddressPoolType::Absent, DIP17_GAP_LIMIT, account.network,🤖 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 551 - 558, Validate that the platform key identity matches the derivation account type before building the DIP-17 pool. In `ManagedAccountCollection` where `ManagedAccountType::single_pool` is called, make sure the `account.account_type` used for derivation cannot diverge from the `ManagedPlatformAccount` identity implied by `key.account` and `key.key_class`; if they differ, either reject the entry or derive from the key’s account type instead. Update the account/pool construction so the watched path and returned platform account label stay aligned.
🤖 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/managed_account/managed_account_type.rs`:
- Around line 500-502: The fallback to DerivationPath::master() in the managed
account address-pool helpers hides derivation-path failures and can produce
addresses for the wrong account/network. Update the logic around
account_type.derivation_path(network) in managed_account_type so it returns or
propagates the error instead of using unwrap_or_else to default to m. Adjust the
affected helper(s) to return Result and use proper error propagation with the
existing error type pattern so callers can handle derivation-path failures
explicitly.
---
Outside diff comments:
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 551-558: Validate that the platform key identity matches the
derivation account type before building the DIP-17 pool. In
`ManagedAccountCollection` where `ManagedAccountType::single_pool` is called,
make sure the `account.account_type` used for derivation cannot diverge from the
`ManagedPlatformAccount` identity implied by `key.account` and `key.key_class`;
if they differ, either reject the entry or derive from the key’s account type
instead. Update the account/pool construction so the watched path and returned
platform account label stay aligned.
🪄 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: f5e4013e-802d-4662-9ddd-c8647d6c70b3
📒 Files selected for processing (2)
key-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_type.rs
| let path = account_type | ||
| .derivation_path(network) | ||
| .unwrap_or_else(|_| crate::bip32::DerivationPath::master()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently fall back to the master path.
If derivation_path(network) fails, these helpers still generate address pools from m, which can make callers watch or persist addresses for the wrong account/network path. Propagate or map the derivation-path error instead of replacing it with DerivationPath::master(). As per coding guidelines, “Use proper error types (thiserror) and propagate errors appropriately” and “Return Result<T> for all fallible operations and use the ? operator for error propagation in Rust code.”
Proposed fix
- let path = account_type
- .derivation_path(network)
- .unwrap_or_else(|_| crate::bip32::DerivationPath::master());
+ let path = account_type.derivation_path(network)?;
crate::managed_account::address_pool::AddressPool::new(
path, pool_type, gap_limit, network, key_source,
)
@@
- let base_path = account_type
- .derivation_path(network)
- .unwrap_or_else(|_| crate::bip32::DerivationPath::master());
+ let base_path = account_type.derivation_path(network)?;Also applies to: 525-527
🤖 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_type.rs` around lines 500 -
502, The fallback to DerivationPath::master() in the managed account
address-pool helpers hides derivation-path failures and can produce addresses
for the wrong account/network. Update the logic around
account_type.derivation_path(network) in managed_account_type so it returns or
propagates the error instead of using unwrap_or_else to default to m. Adjust the
affected helper(s) to return Result and use proper error propagation with the
existing error type pattern so callers can handle derivation-path failures
explicitly.
Source: Coding guidelines
ZocoLini
force-pushed
the
refactor/account-type
branch
from
c6a27a2 to
7383c08
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #828 +/- ##
==========================================
+ Coverage 73.18% 73.32% +0.13%
==========================================
Files 323 323
Lines 72295 72234 -61
==========================================
+ Hits 52911 52967 +56
+ Misses 19384 19267 -117
|
1 participant
While benchmarking and implementing the CoinJoin internal pool tracking I found this duplicated code, I consolidated the ManagedAccountType centralizing the logic
Summary by CodeRabbit