refactor(key-wallet): dedupe core account types via shared trait + composition#728
Open
QuantumExplorer wants to merge 3 commits intoclaude/adoring-booth-151626from
Open
refactor(key-wallet): dedupe core account types via shared trait + composition#728QuantumExplorer wants to merge 3 commits intoclaude/adoring-booth-151626from
QuantumExplorer wants to merge 3 commits intoclaude/adoring-booth-151626from
Conversation
…mposition Eliminates the near-total method duplication between `ManagedCoreFundsAccount` and `ManagedCoreKeysAccount` introduced by the rename split: - Move all 21 shared methods (address-pool / key-derivation / index queries) into `ManagedAccountTrait` as default impls, expressed in terms of 8 primitive accessors. The bodies now live exactly once. - Restructure `ManagedCoreFundsAccount` as composition: it now wraps a `ManagedCoreKeysAccount` (`core` field) and adds the funds-only state (`balance`, `utxos`, `spent_outpoints`). It implements the trait by delegating the 8 primitives to `core`; only funds-specific operations remain as inherent methods (transaction recording, UTXO updates, balance computation, Standard-account receive/change paths). - Privatize all fields on both structs. External access goes through the trait accessors or new inherent getters/setters (`balance()`, `balance_mut()`, `utxos()`, `utxos_mut()`, etc.). Field accesses across `key-wallet`, `key-wallet-ffi`, `key-wallet-manager`, and `dash-spv` test helpers migrated to method calls. - Replace the manual `Deserialize` impl: it now deserializes the inner `core` (which carries `transactions`) and rebuilds `spent_outpoints` from those transaction inputs. Wire format changes (nested `core` object); serialization compatibility was explicitly out of scope. Net diff: -621 lines across 24 files. `managed_core_funds_account.rs` shrinks 1348 -> 677 lines; `managed_core_keys_account.rs` shrinks 666 -> 157. Verified: - cargo check --workspace --all-targets: clean - cargo clippy --workspace --all-targets --all-features: clean - cargo test --workspace --lib: 1802 passed, 0 failed - cargo test -p key-wallet --lib: 461 passed, 0 failed
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…fields Per review feedback: - Rename `ManagedCoreFundsAccount::core` -> `keys`. Method accessors rename `core()` / `core_mut()` -> `keys()` / `keys_mut()`. The serde helper struct field also renames `core` -> `keys` (changes the wire format again, still fine since serialization compat is out of scope). - Make `balance` and `utxos` `pub` again on `ManagedCoreFundsAccount`. Drop the now-redundant `balance()` / `balance_mut()` / `utxos()` / `utxos_mut()` inherent methods. Call sites that had been migrated to method calls revert to direct field access (`account.balance`, `account.utxos`). `spent_outpoints` stays private — it carries an invariant tied to `keys.transactions()`. Verified: cargo check + clippy clean across the workspace; all 1802 lib tests pass; key-wallet 461/461.
- Drop the `[`Self::record_transaction`]` link from the `ManagedCoreFundsAccount` doc comment; `record_transaction` is `pub(crate)` and rustdoc with `-D warnings` rejects intra-doc links from a public item to a private one. - `cargo fmt --all` to fix import ordering (placement of the new `ManagedAccountTrait` use lines) and a long-line wrap in `account_checker.rs`. Tests + clippy still clean locally.
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
Follow-up to #711 that eliminates the near-total method duplication between
ManagedCoreFundsAccountandManagedCoreKeysAccountthat the rename split introduced.Three coordinated changes:
ManagedAccountTraitas default impls, expressed in terms of 8 primitive accessors. The bodies now live exactly once.ManagedCoreFundsAccountis now a thin wrapper:{ core: ManagedCoreKeysAccount, balance, utxos, spent_outpoints }. It implements the trait by delegating the 8 primitives tocore. Only funds-specific operations (transaction recording, UTXO updates, balance computation, Standard-account receive/change paths) remain inherent.balance(),balance_mut(),utxos(),utxos_mut(), etc.). Field accesses migrated to method calls acrosskey-wallet,key-wallet-ffi,key-wallet-manager, anddash-spvtest helpers.The custom
Deserializeimpl is rewritten: it deserializes the innercore(which carriestransactions) and rebuildsspent_outpointsfrom those inputs. Wire format changes (nestedcoreobject); serialization compatibility was explicitly out of scope per the discussion preceding this PR.Numbers
managed_core_funds_account.rs: 1348 → 677 lines (-49%)managed_core_keys_account.rs: 666 → 157 lines (-76%)managed_account_trait.rs: 57 → 597 lines (the relocated shared logic)Test plan
cargo check --workspace --all-targets— cleancargo clippy --workspace --all-targets --all-features— clean (no warnings)cargo test --workspace --lib— 1802 passed, 0 failedcargo test -p key-wallet --lib— 461 passed, 0 faileddash-spv(needsetup-dashd.py; not run locally)Notes
claude/adoring-booth-151626(PR refactor(key-wallet): split ManagedCoreAccount into funds + keys variants #711's branch) so it stacks cleanly on the structural rename.ManagedCoreFundsAccountis intentional —transactionsand account-type fields now sit under acoreobject. Persisted state from prior versions will not deserialize unmigrated; serialization compat was deemed not required.🤖 Generated with Claude Code