refactor(keys): let deriving be the mnemonic validation, and parse once - #387
Conversation
`load_identity_from_mnemonic` — what the account screen's "import user" button runs — called `validate_mnemonic` and then `derive_master_key`, and both parse the same phrase with `Mnemonic::parse`. The second parse is not a safety net: it is the same check, so the pair only creates a second place to answer "what counts as a valid mnemonic", and a future edit to one of them would leave the two disagreeing. Deriving already rejects a bad word or a bad checksum with the very same `invalid mnemonic: …` error, so the import path keeps exactly the behaviour it had, and `validate_mnemonic` goes with its only caller rather than staying as a public helper nothing calls. The Dart dialog only checks the shape (12 or 24 alphabetic words), so both kinds of bad phrase do reach this code — pinned in `deriving_refuses_a_phrase_that_is_not_a_mnemonic`, because the whole point of removing the explicit check is that the implicit one has to hold. Verified: `cargo test` (366), `cargo clippy`, `cargo check --target wasm32-unknown-unknown`, `flutter analyze`. The bridge surface is unchanged — neither function was on it, and `frb-generate.sh` produces no diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012VCrukiZdZo2HfZczSaQ8U
|
Warning Review limit reachedNext included review available in 53 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: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
1 participant
Follow-up to a review observation on #235: the mnemonic import path parses the same phrase twice.
The duplication
load_identity_from_mnemonic— behind the account screen's import user button — did:Both call
Mnemonic::parseon the same phrase, and both fail with the sameinvalid mnemonic: …. The second parse is not a safety net over the first; it is the first, run again. What it does create is a second place to answer "what counts as a valid mnemonic here", which is the kind of pair that drifts: change one of them and the two disagree, silently, on the one input where being wrong costs the user their account.The change
validate_mnemonicgoes with its only caller. It waspubbut not on the bridge and had no other user anywhere in the repo (checked across every remote branch, where the single hit is this same line). Three lines to bring back the day something needs validation without derivation.derive_master_key's doc now says it validates, so the next person does not add the pair back.Why the test
Removing an explicit check is only safe if the implicit one holds, so
deriving_refuses_a_phrase_that_is_not_a_mnemonicpins it: a phrase of real BIP-39 words with a wrong checksum, a phrase with a word that is not in the list, and an empty list (an nsec-imported identity stores no words). Both kinds do reach Rust — the import dialog only checks the shape, 12 or 24 alphabetic words — so this is the layer that has to say no.Without it, the failure mode is not a crash: a typo'd word whose checksum happens to pass would derive some other valid identity, and the user would be looking at an empty account rather than an error.
Test plan
cargo test— 366 passcargo clippy --all-targets— cleancargo check --target wasm32-unknown-unknownflutter analyze— no new issues./scripts/frb-generate.shproduces no diff (neither function is on the bridge)🤖 Generated with Claude Code
https://claude.ai/code/session_012VCrukiZdZo2HfZczSaQ8U