test(swift-sdk): pin non-English BIP-39 mnemonic support against the fixed FFI - #4454
test(swift-sdk): pin non-English BIP-39 mnemonic support against the fixed FFI#4454PastaPastaPasta wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 |
|
✅ Final review complete — no blockers (commit 40cce83) · triage: low |
|
Re-planned per review: the Swift-layer fallback here fixes the symptom at the wrong layer of the stack. The root fix now lives in dashpay/rust-dashcore#981 ( Once #981 merges into rust-dashcore
Converting to draft until then. 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The multilingual BIP-39 derivation is correctly gated by full mnemonic validation and follows the required NFKD/PBKDF2-HMAC-SHA512 procedure. However, the wallet fallback changes mnemonic-created wallets into seed-backed wallets, and the new derivation path handles embedded-NUL passphrases differently from the existing FFI path.
Source: reviewer backends: gpt-5.6-sol (general, security-auditor, FFI-engineer); final verifier backend: gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift:85-87: Localized mnemonic initialization creates a seed wallet
The fallback calls `wallet_create_from_seed[_with_options]`, which the pinned key-wallet implementation routes through `Wallet::from_seed` and stores as `WalletType::Seed`. The normal mnemonic constructor routes through `Wallet::from_mnemonic` and stores `WalletType::Mnemonic`. This difference is directly observable through the public `Wallet.hasMnemonic` property: a valid French phrase now constructs successfully but reports `false`, while an English phrase passed to the same initializer reports `true`. The variants also retain different recovery material and have different serialized representations, so this is not the identical wallet claimed by the comment and can cause backup or capability code to misclassify an imported recovery phrase. Keep construction on a language-aware mnemonic constructor path, such as by updating the pinned Rust implementation, rather than replacing mnemonic construction with seed construction.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Mnemonic.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Mnemonic.swift:163-164: Embedded-NUL passphrases diverge between Swift and Rust derivation
The existing FFI path passes the passphrase through `withCString`, and pinned Rust reads it with `CStr::from_ptr`, so an embedded U+0000 terminates the passphrase and discards its suffix. The new non-English fallback converts the entire Swift string to UTF-8 with an explicit length, causing the NUL and suffix to participate in PBKDF2. Consequently, the same passphrase has language-dependent semantics, and the fallback can derive a different seed from the Rust implementation. BIP-39 defines the passphrase as normalized UTF-8 rather than a C string; either reject embedded NUL before selecting either path or change the Rust ABI to accept a pointer and explicit byte length.
|
Coordination update: the underlying bug is now fixed upstream twice over —
Revised plan for this PR: once #4455 lands, rebase this to drop the Swift-side PBKDF2 fallback entirely and keep only 🤖 Posted autonomously by Claude on behalf of pasta. |
…fixed FFI mnemonic_validate tried every supported wordlist, but the key-wallet FFI behind Mnemonic.toSeed and Wallet(mnemonic:) parsed with a hardcoded English wordlist, so any French/Spanish/Japanese/... phrase that passed validation still failed with 'Invalid Mnemonic: mnemonic contains an unknown word (word 0)'. In dashwallet-ios this broke the DashSync upgrade migration (legacy localized wallets silently not imported) and 'Import from Phrase'. The root fix landed upstream in rust-dashcore dashpay#980/dashpay#981 (every FFI parse path auto-detects the wordlist) and reached this repo through the pin bump in dashpay#4455. Add regression tests that exercise the fixed FFI directly: French vectors against an independent PBKDF2 oracle (with and without passphrase), NFC/NFKD input equivalence, wallet construction from the French phrase (same id as the reference seed, hasMnemonic retained, network-scoped ids), agreement with the platform-wallet language-auto-detecting derivation path, English official-vector guards, and invalid-input rejection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cf2ac62 to
e324d6e
Compare
|
Reworked per the plan above, now at e324d6e (rebased onto
Marking ready for review. 🤖 Posted autonomously by Claude on behalf of pasta. |
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied Phase-2 findings against exact head e324d6e. Both identify the same confirmed coverage gap: the French fixtures contain identical NFC bytes, so the tests do not exercise the promised NFC-versus-NFKD equivalence. The PR changes only tests; no blocking issue was confirmed.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
normalbygpt-6-astra(effort low) — The change adds contained but nontrivial regression tests for mnemonic normalization, seed derivation, and wallet identity across FFI paths without modifying production cryptography, wallet behavior, or dependency pins. - Phase 1 reviewers: not run (skipped for throughput: 37 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort high); agentphase2-reviewer
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/NonEnglishMnemonicTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/NonEnglishMnemonicTests.swift:25-26: Make the baseline French fixture genuinely NFKD
The baseline literal contains four precomposed U+00E9 characters, despite its NFKD comment. Applying `precomposedStringWithCanonicalMapping` therefore leaves it unchanged: the baseline and `frenchPhraseNFC` both contain the same 100 UTF-8 bytes, while the NFKD representation contains 104 bytes. The validation, seed derivation, and wallet equivalence tests consequently exercise NFC twice and cannot detect a regression affecting decomposed recovery phrases. This directly misses the PR's stated normalization-equivalence coverage, particularly because the Swift wrappers forward the input bytes to the FFI. Explicitly compatibility-decompose the baseline and retain the existing NFC conversion. Add an assertion comparing the fixtures' UTF-8 arrays for inequality to guard against this recurring; Swift String equality treats canonically equivalent representations as equal.
The baseline French phrase was a bare source literal carrying four precomposed U+00E9, so `precomposedStringWithCanonicalMapping` left it unchanged and both fixtures held the same 100 UTF-8 bytes. The three tests that read as NFKD-vs-NFC coverage — validation, seed derivation and wallet equivalence — therefore exercised NFC twice, and a regression reaching only decomposed phrases would have passed. That is the form legacy DashSync stores hold, which is the migration this PR exists to protect. Compatibility-decompose the baseline explicitly instead of trusting whatever form an editor saved, and add a guard that the two fixtures differ in their encoded bytes. The guard compares UTF-8 arrays because Swift String equality is canonical: `frenchPhrase == frenchPhraseNFC` is true in both the broken and the fixed state, so string equality can never catch the pair collapsing. Behaviour is unchanged. Verified against key-wallet at the rev v4.2-dev pins (93260bf3): both encodings parse as French and derive the same seed, with and without the TREZOR passphrase. Test would have caught this: the byte-distinctness guard is ✖ on the old fixture (100B vs 100B) and ✔ on the new one (104B vs 100B).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR is limited to regression tests for non-English BIP-39 mnemonic handling and does not introduce production-code or FFI changes. The previously reported fixture issue is fixed: the French baseline is explicitly compatibility-decomposed, and the tests verify that its UTF-8 encoding differs from the NFC fixture while remaining canonically equivalent. No remaining in-scope issues were identified.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: ffi-engineer); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff adds a self-contained Swift regression test file only, with no production behavior, dependency, build, or cryptographic/key-handling implementation changes. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— ffi-engineer (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 63% left, 5h 64% left - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort medium); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort medium); agentphase2-reviewer
3 participants
Issue being fixed or feature implemented
Field report from dashwallet-ios testing (2026-08-22): a user's multi-year-old French 12-word recovery phrase could not be imported into the migrated app:
Invalid Mnemonic: mnemonic contains an unknown word (word 0);Root cause:
mnemonic_validate(key-wallet FFI) tried all 10 supported BIP-39 wordlists, but the two functions actually used for key material —mnemonic_to_seedandwallet_create_from_mnemonic— parsed with hardcodedLanguage::English. So any non-English phrase passedMnemonic.validateand then failed inMnemonic.toSeed/Wallet(mnemonic:).What was done?
The root fix landed upstream in rust-dashcore (#980 fixed every English-hardcoded parse path; #981 unified validation and key derivation on one auto-detecting
from_phrase), and reached this repo via the pin bump in #4455 (current pin393b6122contains both). The earlier revision of this PR carried a Swift-layer PBKDF2 fallback; that was the wrong layer and is dropped entirely —Mnemonic.swiftandWallet.swiftare byte-identical tov4.2-dev.What remains is regression coverage that exercises the fixed FFI directly:
SwiftTests/SwiftDashSDKTests/NonEnglishMnemonicTests.swift(11 tests).000102…0f, official French wordlist) — seed with and without passphrase, matching an independent Python oracle (hashlib.pbkdf2_hmacover the NFKD sentence) that reproduces the official BIP-39 English test vectors;hasMnemonicis retained (it is aWalletType::Mnemonicwallet, not a seed-only one), and ids stay network-scoped;platform_wallet_derive_ext_priv_key_from_mnemonic) derives the same BIP-32 master key as the reference seed, pinning that app-side wallet ids match manager-created wallets;How Has This Been Tested?
Locally on macOS against the current pin (
bash build_ios.sh --target mac --profile dev, thenswift test): the 11NonEnglishMnemonicTestspass; full hermetic unit suite 434 tests, 14 skipped, 0 failures, no source warnings. These same tests were red against the pre-#4455 pin with exactly the field-reported error.Breaking Changes
None — tests only.
Checklist:
🤖 Generated with Claude Code