refactor(masternode-keys): one provider key resolver, not two - #938
Merged
QuantumExplorer merged 1 commit intoAug 8, 2026
Merged
Conversation
#936 and #937 landed independently and each brought its own memoised wrapper over `ManagedPlatformWallet.providerKeyAtIndex` — byte-identical bodies, one for the ECDSA owner/voting family and one for BLS/Ed25519. Two caches to keep in step and two places to change. Collapse both into a file-private `ProviderKeyResolver`, now the single call site for all four families. `MasternodeProviderKeyDeriver` keeps its distinct job — it carries the address pool the owner/voting rows join against — and delegates derivation. Operator/evonode-operator have no address rows, so their wrapper shell goes away entirely and the model holds a resolver directly; `tenderdashNodeKeyBase64` moves onto the resolver, still guarded on the Ed25519 kind. Also drops three pieces of state left dead by #937, when derivation moved Rust-side: * `wallet` — written in init, never read since. It held a `Wallet` owned by the `WalletManager` that `derivationWallet()` builds per call and that this class never retained, so it was a reference into a graph released at the end of init. Unused, so harmless, but not worth keeping. * `key` — written, never read. * the bound `network` — it selected the coin type back when this class composed the DIP-3 path app-side. Now a boolean test, with a comment for why the check stays. No behaviour change. Same kinds, same `includePrivate: true`, same cache lifetime (one per deriver instance). The host-wallet guard now runs before the `derivationWallet()` guard rather than after, which cannot change the outcome: `derivationWallet()` returns nil unless that same wallet is present, so the guard only fires where the other would have. Verified: clean `dashpay` build, no warnings left in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughDerivationPathKeysModel now uses ProviderKeyResolver for operator, evonode-operator, owner, and voting keys. It removes ProviderKeyDeriver and routes key serialization and lookup operations through the shared resolver. ChangesProvider key resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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 `@DashWallet/Sources/UI/Menu/Tools/Masternode`
Keys/DerivationPathKeys/Models/DerivationPathKeysModel.swift:
- Around line 170-173: Update the .publicKey branch in the derivation-path key
selection logic to obtain the key through ecdsaDeriver, ensuring .owner and
.voting rows do not depend on the nil providerResolver. Leave the
.publicKeyLegacy branch unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbe3b974-ebe6-4071-b471-89347caf2da7
📒 Files selected for processing (1)
DashWallet/Sources/UI/Menu/Tools/Masternode Keys/DerivationPathKeys/Models/DerivationPathKeysModel.swift
Comment on lines
170
to
+173
| case .publicKey: | ||
| value = providerDeriver?.key(at: index)?.publicKeyHex | ||
| value = providerResolver?.key(at: index)?.publicKeyHex | ||
| case .publicKeyLegacy: | ||
| value = providerDeriver?.key(at: index)?.legacyPublicKeyHex | ||
| value = providerResolver?.key(at: index)?.legacyPublicKeyHex |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the ECDSA public-key lookup.
providerResolver is nil for .owner and .voting. Line 171 therefore returns "Not available" for their .publicKey rows. Route this lookup through ecdsaDeriver too.
Proposed fix
case .publicKey:
- value = providerResolver?.key(at: index)?.publicKeyHex
+ value = providerResolver?.key(at: index)?.publicKeyHex
+ ?? ecdsaDeriver?.publicKeyHex(at: index) func privateKeyHex(at index: UInt32) -> String? {
keyResolver.key(at: index)?.privateKeyHex
}
+
+func publicKeyHex(at index: UInt32) -> String? {
+ keyResolver.key(at: index)?.publicKeyHex
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case .publicKey: | |
| value = providerDeriver?.key(at: index)?.publicKeyHex | |
| value = providerResolver?.key(at: index)?.publicKeyHex | |
| case .publicKeyLegacy: | |
| value = providerDeriver?.key(at: index)?.legacyPublicKeyHex | |
| value = providerResolver?.key(at: index)?.legacyPublicKeyHex | |
| case .publicKey: | |
| value = providerResolver?.key(at: index)?.publicKeyHex | |
| ?? ecdsaDeriver?.publicKeyHex(at: index) | |
| case .publicKeyLegacy: | |
| value = providerResolver?.key(at: index)?.legacyPublicKeyHex |
🤖 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 `@DashWallet/Sources/UI/Menu/Tools/Masternode`
Keys/DerivationPathKeys/Models/DerivationPathKeysModel.swift around lines 170 -
173, Update the .publicKey branch in the derivation-path key selection logic to
obtain the key through ecdsaDeriver, ensuring .owner and .voting rows do not
depend on the nil providerResolver. Leave the .publicKeyLegacy branch unchanged.
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.
What
#936 and #937 landed independently and each brought its own memoised wrapper over
ManagedPlatformWallet.providerKeyAtIndex— byte-identical bodies, one for the ECDSA owner/voting family and one for BLS/Ed25519. Two caches to keep in step, two places to change.Both collapse into a file-private
ProviderKeyResolver, now the singleproviderKeyAtIndexcall site for all four families.MasternodeProviderKeyDeriverkeeps its distinct job — it carries the address pool the owner/voting rows join against — and delegates derivation to the resolver. Its name and public surface are unchanged, soWalletKeysOverviewModelandMasternodeVoterRegistryare untouched.tenderdashNodeKeyBase64moves onto the resolver, still guarded onkind == .platformNodeEdDSA.Dead state removed
Three pieces left behind by #937 when derivation moved Rust-side — none introduced here:
wallet— written ininit, never read since. It held aWalletowned by theWalletManagerthatSwiftDashSDKHost.derivationWallet()builds per call, and that this class never retained — so it was a stored reference into a graph released at the end ofinit. Nothing read it, so no live defect, but it shouldn't stay.key— written, never read.network— it selected the coin type back when this class composed the DIP-3 path app-side. Now a boolean test, with a comment for why the check remains. This also clears a pre-existing build warning.Behaviour
Unchanged. Same kinds, same
includePrivate: true, same cache lifetime (one resolver per deriver instance, as before).One ordering change worth naming: the host-wallet guard now runs before the
derivationWallet()guard rather than after. It cannot change the outcome —derivationWallet()itself returnsnilunlessSwiftDashSDKHost.shared.walletis present, so the resolver's guard can only fail where the other would have failed anyway.Test plan
dashpaybuild (ARCHS=arm64), no errors and no warnings remaining in the file🤖 Generated with Claude Code
Summary by CodeRabbit