Repository navigation
recovery + ledger: list Braavos accounts like Ready's, and find them in first-run Ledger setup (#718) - #733
Conversation
The recovery-phrase "Find your accounts" Braavos group was the only one with no index-zero row: a phrase with no Braavos activity left it empty (#717 added a bare "No accounts with on-chain activity." line), while Bramble and Ready show "Account 0". Discovery now lists index zero at the class the current Braavos app deploys, beside OpenZeppelin and the pinned Argent release; older Braavos classes are still listed only once used. The picker shows it view only, exactly as an unused Ready account: not selectable (selectable() requires deployment or funds), and labelled "No on-chain activity found — Bramble only imports existing Braavos accounts". Without that, an unused current-class row would have read "Funded and ready to deploy". The background does not check selections against discovery, so this lists one more row without widening what a restore accepts. The empty-group line is removed: every group now always has a row. Refs #718
First-run "Set up with a Ledger" only read Bramble's own LedgerW path, so
a Ledger that owns a Ready or Braavos account (both kept at Ready's
argentx path) found neither; that scan existed only in-wallet, under
Import > "Ready Ledger account".
First-run now reads both paths per page (one device round each, in
order: the transport is a serial pipe) and derives Bramble + Argent from
LedgerW and Ready (Argent) + Braavos from argentx. Every candidate keeps
the path it was read at, so a picker spanning both still creates and
signs each account at its own path; the background already accepts
Ready and Braavos Ledger accounts for vault creation and adds. The
in-wallet routes read what they did before.
The grouped picker now matches the recovery-phrase "Find your accounts":
Bramble, Ready and Braavos groups with an "N found" count, rows titled
"Account N" with the same status lines ("Deployed on Starknet", "No
on-chain activity found — Bramble only imports existing Ready accounts",
"Already in this wallet" beside a disabled checkbox), and no balance.
An Argent group holding only Bramble-path accounts keeps "Argent
accounts", since those are not Ready accounts. When both paths are
listed, an unused Argent account at Bramble's path is hidden so it does
not twin every Ready row; one with activity is listed as
"Account N · Bramble path" (LedgerW is also starknet.js's default Ledger
path, so who made it is not known, only where its key lives).
Device enumeration moves to onboard-enumerate.ts (re-exported, importers
unchanged) to keep the controller under the size cap. Test harnesses now
derive per path, as the background does.
Refs #718
PR SummaryMedium Risk Overview First-run Ledger setup reads both The Ledger account picker is aligned with the recovery picker: Bramble / Ready / Braavos groups with “N found” counts, shared status lines (no balance), disabled checkboxes for duplicates and unused Ready/Braavos, and logic to hide idle Argent rows at Bramble’s path when both schemes are listed so they don’t twin Ready rows. Enumeration moves to Tests and shared harnesses are updated/added for discovery listing, dual-path first-run, and picker behavior. Reviewed by Cursor Bugbot for commit 8ab3c8c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
AI-assisted review (Claude, with an independent Codex gpt-6-astra pass), posted from @derek-flossman's account. Findings were checked against the code; treat them as review input, not approval. Medium — unknown funding shown as "no activity" ( Test gap (suggested): an integration test that a Ready/Braavos account picked as the first account during Ledger first-run keeps its derivation path through vault creation and signing after the vault is reopened. Triage note: this PR doesn't change phrase-derived discovery, so it doesn't resolve #743 or #756 (details on those issues). |
adrienlacombe
left a comment
adrienlacombe
left a comment
There was a problem hiding this comment.
Thanks, the picker parity work makes the two scans much easier to understand side by side. A few findings, mostly about base branch and CI and what this does and doesn't cover:
1. Base branch and CI. This targets fix/718-onboarding-polish (stacked on #731), not release/v0.10.8. Because of that, ci.yml (branches: [main, "release/**"]) never ran here: e4d21eb only has Bugbot, test-and-publish and deploy-pages, with no fast-checks or pr-guards. Also, when #731 merges and GitHub retargets this PR, that's an edited event, and fast-checks is skipped on edited. So after the retarget, please push a rebase (or close and reopen) so fast-checks actually runs before merge.
2. This doesn't fix #743. The reporter confirmed it's an older Braavos account from 2023. Bramble's Braavos table (account-families.ts) only has the base class, braavos 1.0.0, and the current and legacy implementations. As far as I know, 2023 Braavos accounts were deployed through Braavos's Cairo 0 proxy (0x03131fa0…f7d1e, which is declared on Mainnet), with a different constructor calldata. This PR only adds an index-zero row at the current 1.0.0 class. That user will now see "Account 0 · No on-chain activity found" under Braavos, which may confuse them more. It's worth saying on #743 that this PR isn't the fix. The fix is either a legacy-proxy scan shape or find-by-address (#398).
3. Device and RPC cost of the first-run change (inline). First-run now reads two schemes per page, so 2× silent device reads. It also derives four candidates per index instead of two (OZ + Argent at LedgerW, Argent + Braavos at argentx), and each one gets its own deployment and balance reads, so roughly 2× RPC per page. That's fine for first run, but please check it on a real device with "Show more" a few pages deep, and on a slow RPC.
What I verified is fine:
- Each candidate keeps its own
eip2645applicationand path, so it's created and signed at the path it was read at. - Addresses all come from device public keys, so there's no mis-attribution risk.
- Only public keys are read and nothing is persisted before the user picks.
- The unused Braavos row is disabled and never auto-selected, as your test shows.
- The background still doesn't validate restore selections against discovery, so the picker's
selectable()remains the only gate for unused Ready and Braavos rows. That isn't new, just worth remembering.
Overlap and ordering: #762 touches the same journeys/ledger/onboard-* files and sidepanel-ledger-chooser.test.ts. #763 touches onboarding-ops.ts right above your isCreatableCandidate hunk, plus the same two recovery test files. From the hunks nothing overlaps. I'd merge in this order:
As you said, part 2 still needs a hardware check with a Ledger that owns a Ready or Braavos account.
| openConnect(LEDGER_EIP2645_APPLICATION); | ||
| // First-run "Set up with a Ledger" creates Bramble accounts and finds | ||
| // existing Ready and Braavos ones, as a recovery-phrase setup does. | ||
| openConnect(LEDGER_EIP2645_APPLICATION, [ |
There was a problem hiding this comment.
Reading both schemes here doubles the silent device reads per page. Each page also now yields four candidates per index (OZ + Argent at LedgerW, Argent + Braavos at argentx), and every one gets its own deployment and balance reads. It might be worth a quick device-and-RPC timing check with a few "Show more" pages before release, especially on a slow RPC endpoint.
When the balance read for a Ledger candidate failed, the background returned `funded: null`. The new status line then read it as unfunded and showed "No on-chain activity found — Bramble only imports existing Ready/Braavos accounts", so a funded account could look empty, as reported in #756. Before this branch the row said "balance unavailable". It now says "Couldn't check this account's activity. Try again." The row stays unpickable: a Ready or Braavos account is offered only on proven activity. A view test covers it, and fails without the fix. Refs #718 Refs #727 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged with today's release/v0.10.8 (#763 added Xverse tests to the same files), recovery-discovery.test.ts reached 502 lines and sidepanel-recovery-scan-gate.test.ts 510, which check:files refuses. No test changes: - The index-zero listing test moves to recovery-discovery-listing.test.ts. - The picker fixtures move to tests/support/recovery-picker-harness.ts, the same file name and export style #755 uses for them. Refs #718 Refs #727 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review follow-up (AI-assisted, by Claude), posted from @derek-flossman's account. Pushed to this branch with Derek's OK. @Ryun1, heads-up: I pushed three commits here.
Run on this branch after the push:
Adrien's other points, verified:
Still needed before merge: a click-through with a Ledger that holds a Ready or Braavos account, including a few "Show more" pages on a slow RPC to check the timing. Optional test gap (from my earlier note): a Ready/Braavos account picked as the first Ledger account should keep its Known gap, not changed here (it's on the base too): a failed deployment read leaves |
…hiding it In a first-run scan of both paths, an Argent account at Bramble's path joins the Ready group only once it shows activity, so an unused one doesn't twin every Ready row. A row that is undeployed but whose balance read failed was hidden the same way, so the user saw fewer accounts and no sign that a check failed. It is now listed with "Couldn't check this account's activity. Try again." A row still being checked stays out until its activity is known. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged by the repo owner via the release-review bypass; no second review. Head 61b0e40 was up to date with release/v0.10.8, with fast-checks and pr-guards green. |
Summary
Two changes to which accounts onboarding finds, both about Braavos (and Ready):
1. Recovery phrase: Braavos Account 0, view only (
03ffe1a7)On "Find your accounts" the Braavos group was the only one with no index-zero row. With no Braavos activity it showed a bare "No accounts with on-chain activity." line (#717), while Bramble and Ready show "Account 0".
BRAAVOS_DEPLOYMENT_CLASS_HASH), beside OpenZeppelin and the pinned Argent release. Older Braavos classes are still listed only once used.selectable()(deployed or funded) already gates unused Ready rows the same way.2. First-run Ledger setup finds Ready and Braavos accounts (
e4d21eb6)"Set up with a Ledger" only read Bramble's own
LedgerWpath, so a Ledger owning a Ready or Braavos account (both live at Ready'sargentxpath) found neither. That scan existed only in-wallet, under Import > "Ready Ledger account".LedgerW, and Ready (Argent) + Braavos fromargentx. Twice the device reads per page.createLedgerVault/addLedgerAccountalready accept Ready and Braavos atargentx(isLedgerApplicationForFamily), and the vault uses the samederiveLedgerAccountfor the first account as for later ones.LedgerWis also starknet.js's default Ledger path, so who made it is not known.onboard-enumerate.ts(re-exported; importers unchanged).Roadmap & release
release/v0.10.8(after onboarding: UX polish for full-tab setup and Ledger (#718) #731)Checks
bun run checkpasses. Every gate passes locally exceptapps/terminal"Tokens screen > defaults to AVNU…", which fails identically on a cleanrelease/v0.10.8locally and passes in CI.bun run test:e2epasses (22/22)Needs a device before merge: part 2 is unit-tested and its picker was checked by rendering the real view code, but not with hardware. Please try first-run setup with a Ledger that owns a Braavos or Ready account.
Dependencies
none
Reminder: review before merging, especially the discovery change in
onboarding-ops.ts.