Skip to content

accounts: send a phrase-less wallet's regular-account choice to import (#740) - #762

Merged
derek-flossman merged 3 commits into
release/v0.10.8from
fix/740-ledger-only-add-account
Oct 26, 2026
Merged

derek-flossman merged 3 commits into
release/v0.10.8from
fix/740-ledger-only-add-account

Conversation

@derek-flossman

@derek-flossman derek-flossman commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #740
Refs #727

Problem: after onboarding with only a Ledger (or a pasted private key), Add account → Regular account showed the Ledger onboarding error page: "Something went wrong" over the refusal, with a Try again button that starts a Ledger connect. The wallet has no recovery phrase to derive a regular account from, so retrying can never work. It's a dead end.

Fix: the add-account controller recognises the vault's phrase-less refusal and lands on Import an existing account instead, with the existing reviewed sentence as a status note. "Find my other accounts" is left out of that screen, because it needs the same missing phrase. Every other add-account failure still shows the error page.

Unchanged: vault and key handling. The vault still refuses before any write, and no phrase is created behind the user's back (vault-service-ledger.test.ts keeps covering that).

Tests:

  • controller: the refusal renders import-chooser with the note;
  • view: the note is escaped and the rescan tile is dropped; the plain chooser has no note;
  • runtime (DOM): clicking Regular renders the import routes with the reason and no Ledger connect.

Mutation check: each of these fails 1–2 tests:

  • always rendering the error page;
  • keeping the rescan tile;
  • dropping the note in the runtime;
  • breaking the refusal predicate.

bun run check passes. bun run check:coverage passes (functions 95.54%, lines 96.12%). The first run hit the known multisig timeout under machine load; that test passes alone, and the re-run was clean. fast-checks and pr-guards are green on cbf91f66. E2E wasn't run locally because the machine was loaded.

Merge note: #731 and #733 (Ryun1) also touch journeys/ledger/onboard-*. This PR keeps its edits small (about 20 source lines) and appends its tests at the end of each file, so any conflict should be trivial.

Copy for owner review

No new strings. The note reuses the existing reviewed sentence: "This wallet was set up without a recovery phrase. Import an existing account instead."

Decision for owner

A wallet set up from a Ledger still shows the Regular account tile, and you only find out it can't work after tapping it. There are two ways to fix that properly:

  1. Hide or disable the tile up front. The wallet status would need to say whether the wallet has a recovery phrase. That's a small protocol change.
  2. Let a Ledger-only wallet create a new recovery phrase, with a proper backup step. This is a product and security decision.

This PR does neither. It's the safe minimum: no dead end and no new key material.

🤖 Generated with Claude Code

#740)

A wallet set up from a Ledger or a pasted key holds no recovery phrase, so
wallet.addAccount refuses the "Regular account" tile. The refusal rendered the
Ledger onboarding error page: "Something went wrong" over the mapped sentence,
with a "Try again" that starts a Ledger connect. That is a dead end.

The controller now recognises that refusal and lands on the import chooser
instead, with the existing reviewed sentence as a status note. The rescan tile
("Find my other accounts") is left out there, since it needs the same phrase.
Every other add-account failure keeps the error page.

No vault or key handling changes: the vault still refuses before any write
and no phrase is created behind the user's back.

Refs #740
Refs #727

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
UX and error-routing only; vault still refuses derivation before write and no new secrets or key material are introduced.

Overview
Fixes a dead end when Add account → Regular account is chosen on a wallet with no recovery phrase (Ledger-only or pasted key): the flow now lands on Import an existing account with the existing friendly copy as a status note, instead of a Ledger error page whose Try again cannot succeed.

NO_RECOVERY_PHRASE_REFUSAL is introduced in wallet-model and reused by the vault addDerivedAccount refusal, the background missingPhraseRefusal check, and the panel’s isPhraselessAddRefusal matcher so all layers agree on one string (#740).

The Ledger onboard controller catches that refusal after addHd and renders import-chooser with optional phraselessNote; the import chooser shows the note and omits Find my other accounts (rescan needs the missing phrase). Other add-account failures still use the error view.

Reviewed by Cursor Bugbot for commit 271a0ac. Bugbot is set up for automated code reviews on this repo. Configure here.

@derek-flossman

Copy link
Copy Markdown
Collaborator Author

AI-assisted review (Claude, with an independent Codex gpt-6-astra pass), posted from @derek-flossman's account.

Reviewed: the change is UI-only and the tests cover the controller, the view and the runtime.

  • Nit: isPhraselessAddRefusal matches the vault's exact error text, so rewording that message would silently bring back the error page. A typed error code would be sturdier.
  • Open decision: the Regular tile still shows for phrase-less wallets. It could be hidden (needs a has-phrase flag in wallet status), or a phrase-less wallet could be allowed to create a phrase with a backup step.
  • Known leftover: other add-account failures still offer "Try again", which starts a Ledger connect.

@adrienlacombe adrienlacombe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It's a safe, minimal way out of the #740 dead end. What I checked:

  • Target: release/v0.10.8.
  • No key handling changes. Only UI routing changes. The vault still refuses before writing anything, no phrase or key material is created, and nothing new is stored. Hiding "Find my other accounts" is right, since it needs the missing phrase. Keeping Recovery phrase, Private key and Ready Ledger account makes sense for a Ledger-only or pasted-key wallet.
  • The predicate matches what the background actually sends. account-creation-handlers.ts returns "No recovery phrase stored for this wallet" verbatim (NO_RECOVERY_PHRASE). If it ever doesn't match, you fall back to the old error page, so it fails safe.
  • CI on cbf91f6: fast-checks (01:35 UTC run), test-and-publish, pr-guards and Bugbot all pass. The later fast-checks "skipped" is just the edited re-run. The branch is 3 docs-only commits behind the base.
  • Overlap with #731/#733: both touch onboard-session.ts, onboard-types.ts, onboard-views.ts, onboard-runtime.ts and sidepanel-ledger-chooser.test.ts. From the hunks, the edits are close but don't overlap (e.g. your import-chooser line in onboard-types.ts sits a few lines below #733's doc-comment edit). Whichever lands second should rebase and re-run fast-checks, but I don't expect a real conflict.

One small suggestion (inline): the refusal string now exists as three separate literals (vault, background handler, and this predicate). A shared constant or error code would stop a future copy edit from quietly bringing the dead end back. I agree the proper fix is the owner decision you describe, and option 1 (hide the tile when there's no phrase) seems the natural next step.

* from a Ledger or a pasted key and so holds no phrase to derive from.
*/
export const isPhraselessAddRefusal = (raw: string): boolean =>
raw.trim().toLowerCase() === "no recovery phrase stored for this wallet";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the third copy of this literal (the vault's addDerivedAccount and NO_RECOVERY_PHRASE in account-creation-handlers.ts are the others). If one is reworded, this silently goes back to the error page. That's safe, but #740 would come back. Could the background export the constant, or return a reason code, so a test pins the two ends together?

…und and panel

The panel routes a phrase-less wallet's "Regular account" choice to import
only when it recognises this refusal (#740). The text lived as four separate
literals (vault, background handler, the panel's check and its error copy), so
rewording one would quietly bring the dead end back. It is now one constant,
NO_RECOVERY_PHRASE_REFUSAL in @bramble/wallet-model/accounts, following
UNDEPLOYED_BRAAVOS_REFUSAL. A background test feeds the handler's and the
vault's refusal through the panel's check and error copy, so the two ends
can't drift apart (review feedback).

Refs #740
Refs #727

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@derek-flossman

Copy link
Copy Markdown
Collaborator Author

Review follow-up (AI-assisted, by Claude), posted from @derek-flossman's account.

Thanks @adrienlacombe. I addressed the inline note in 271a0ac.

Shared constant. There were four source copies of the refusal, not three: the vault (service-creation.ts), the background handler (NO_RECOVERY_PHRASE, the one the panel actually receives), the panel's isPhraselessAddRefusal, and the existing friendly-error-copy.ts entry. All four now use NO_RECOVERY_PHRASE_REFUSAL from @bramble/wallet-model/accounts, following the UNDEPLOYED_BRAAVOS_REFUSAL pattern. The error-copy key uses .toLowerCase(), like the WRONG_CHAIN_MESSAGE entry.

Pinning test. add-account-no-phrase.test.ts now passes the real handler's refusal and the real vault's refusal through the panel's isPhraselessAddRefusal and friendlyError, and expects the import copy. Mutation check: rewording the handler's refusal fails this test.

Verified on the branch:

The longer-term fix (hide the Regular tile for phrase-less wallets, or offer to create a phrase) stays an owner decision for a follow-up.

@derek-flossman
derek-flossman merged commit 3d4607c into release/v0.10.8 Oct 26, 2026
6 checks passed
@derek-flossman
derek-flossman deleted the fix/740-ledger-only-add-account branch October 26, 2026 18:01
@derek-flossman

Copy link
Copy Markdown
Collaborator Author

Merged by the repo owner via the release-review bypass; no second review. Head 91b47a3 was up to date with release/v0.10.8, with fast-checks and pr-guards green.

derek-flossman added a commit that referenced this pull request Oct 26, 2026
Merged with today's release/v0.10.8, #765 and #762 each added a few lines
to files this branch also grows. That left styles-onboarding.ts at 501
lines and onboard-session.ts at 502, which check:files refuses. This
shortens four comments: two from this branch and the #762 one, with no
code change. The files are now 497 and 499 lines.

Refs #718
Refs #727

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants