fix(swift-sdk): stop applying the account derivation path twice - #4334
fix(swift-sdk): stop applying the account derivation path twice#4334QuantumExplorer wants to merge 1 commit into
Conversation
`account_derive_private_key_as_wif_at` resolves `Account::derivation_path()`
itself — the FULL path from the wallet master, e.g. `m/9'/5'/3'/1'` for
provider voting keys — so it must be handed the master. The wrapper instead
took a `masterPath`, pre-derived it, and passed the result, applying the
account path twice: callers passing the account root (the only callers, and
what the parameter's own doc told them to pass) got keys from
m/9'/5'/3'/1'/9'/5'/3'/1'/index instead of m/9'/5'/3'/1'/index
Nothing failed locally. The key was well-formed and deterministic, so it
round-tripped through WIF parsing and signing without complaint; it simply
was not the account's key at that index. Only a counterparty holding the
real key could tell. A masternode vote signed with it is rejected by
Platform as having no voter identity, because the voter identity is
SHA256(pro_tx_hash ‖ hash160(voting pubkey)) — derived from the signing
key's own hash. The wallet matched the node's registered voting address in
its address pool, then signed with a key from a different branch, so the
identity it asked for had never existed.
The parameter is removed rather than corrected: its name and doc both said
"account root", so the API was wrong by default and every caller took the
trap.
Tests pin account-based derivation against the explicit DIP-3 path for
voting and owner keys, at indexes 0/1/19, plus testnet (the account
resolves its own coin type) and a guard that the doubled path is NOT what
we derive. Verified they fail with the old behaviour restored and pass with
the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Swift SDK now derives account private keys from the wallet master key at the root path. The API no longer accepts ChangesAccount key derivation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 5128989) |
![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 `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swift`:
- Around line 45-47: Update the surrounding master-key derivation flow to use
distinct FFIError instances for each FFI call, including separate cleanup blocks
passed to error_message_free; ensure account_derive_private_key_as_wif_at cannot
reuse the error value or cleanup pointer associated with the other call.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 023b06d4-16c1-4ddc-a642-7e951fe8b7c9
📒 Files selected for processing (2)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/AccountDerivationPathTests.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR is a correct, narrowly-scoped fix: it stops Account.derivePrivateKeyWIF from applying the DIP-3 account derivation path twice by handing the FFI the wallet master ("m") instead of a pre-derived account-root xpriv, matching the documented behavior of account_derive_private_key_as_wif_at. The single caller was updated, no stale call sites remain, and the new AccountDerivationPathTests pin voting/owner paths across mainnet and testnet with an explicit regression guard against the doubled path. No in-scope defects were found in the diff itself. Source: reviewers codex-general (gpt-5.6-sol), codex-ffi-engineer (gpt-5.6-sol), sonnet-general (claude-sonnet-5), sonnet-ffi-engineer (claude-sonnet-5); final verifier sonnet (claude-sonnet-5). Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed)
|
Superseded — deleting the function instead of fixing it. The bug was real (the account derivation path was applied twice, so owner/voting keys came from Rather than keep a fixed-but-unused API, it's being removed: it was the last of the parallel derivation path #4338 exists to consolidate, and leaving it alive keeps the mechanism without the cross-check, watch-only support, or address that Coverage isn't lost: #4338's Rust tests pin the same DIP-3 paths at the layer that derives them, plus the watch-only case these Swift tests never reached. Replacement PR: deletion, cross-referencing #4338 and |
2 participants
The bug
Account.derivePrivateKeyWIFreturned keys from the wrong derivation path.account_derive_private_key_as_wif_at→derive_xpriv_from_master_xprivresolvesAccount::derivation_path()itself — the full path from the wallet master (m/9'/5'/3'/1'for provider voting keys,…/2'for owner) — so it must be handed the master:The Swift wrapper instead took a
masterPath, pre-derived it, and passed the result. The account path was therefore applied twice:Every caller hit it, because the parameter's own doc said to pass the account root (
"m/9'/5'/3'/1'").Why it went unnoticed
Nothing failed locally. The derived key was well-formed and deterministic, so it round-tripped through WIF parsing, address rendering and signing without complaint. It just wasn't the account's key at that index — only a counterparty holding the real key could tell.
That counterparty turned out to be Platform. A masternode vote is signed with the voting key, and the voter identity is
SHA256(pro_tx_hash ‖ hash160(voting pubkey))— derived from the signing key's own hash. The wallet matched a node's registered voting address in its address pool (correct, that pool comes from the account xpub), then signed with a key from a different branch, so it asked Platform for an identity that had never existed. Platform answeredPublic key 0 doesn't exist, and after #4333 made that diagnostic,No voting identity exists on Platform for masternode ….Confirmed arithmetically against a real device failure: the identity in the error matched neither
SHA256(protx ‖ h160(registered address))nor its byte-reversed variant, proving the address the signer used was not the registered one.The fix
Hand the FFI the wallet master and drop the parameter. It is removed rather than corrected because its name and documentation both said "account root" — the API was wrong by default, and the only caller took the trap.
Blast radius
account_derive_private_key_as_wif_at/derivePrivateKeyWIFhas exactly one caller across the SDK and dashwallet-ios: the masternode key deriver. Identity and DPNS transitions sign through the platform-wallet identity signer in Rust and never touch this path, which is why document creates and identity operations were unaffected.Two user-visible consequences, both fixed by this:
Testing
New
AccountDerivationPathTestspins account-based derivation against the explicit DIP-3 path:m/9'/5'/3'/1') and owner (m/9'/5'/3'/2') keys at indexes 0, 1 and 19m/9'/1'/3'/1'), since the account resolves its own networkVerified these fail with the old behaviour restored (4/4 failing) and pass with the fix (4/4), so they genuinely cover the regression rather than merely passing.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests