fix: review follow-ups for #51–#62 — one rule per place, and the spec matches the code - #64
Merged
Merged
Conversation
Found re-validating the spec review of #52-#54. Spec 2026-09-14 section 4 promises "never a silent mainnet query" off mainnet, and the CHANGELOG entry for it says the same. Both were true only of a fresh database. Migration 009 originally seeded `explorer_api_url` with the mainnet explorer. #54 edited that already-shipped migration to seed an empty string instead, so the runtime resolves the explorer per the profile's network. A migration runs once: every installation that had already applied 009 kept the mainnet URL in `settings`, and `explorer_client_from_settings` prefers an explicit setting over the network default. So an upgraded wallet on testnet or regtest went on reading mainnet and getting confident answers about a chain it was not on — the exact cross-network read G2 exists to close. Two layers, because they fail differently. Migration 032 deletes the setting when it still holds the value 009 seeded, and leaves any other URL alone: a private explorer is the user's own and the wallet cannot tell which chain it serves. The factory then refuses the known mainnet explorer off mainnet, which covers a database that reaches the resolver before a newer build's migrations have run. Four tests pinned the migration count and had to move to 32; the comment block in `migration_tests` names the new one like its neighbours. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2572 passed), npm run lint:format.
CODING_STANDARDS says `Option` means "unknown", not "false", and that
callers act only on a definite answer. Three sync steps broke that rule
the same way: they resolved the profile's network with
get_wallet_profile(..).ok().flatten().and_then(..).unwrap_or_default()
and `Network` derives `Default = Main`. So a profile row that fails to
load resolved to mainnet and the step carried on — picking the mainnet
explorer for a wallet that may be on neither. That is the silent
cross-network read the G2 gate exists to close, and the CHANGELOG entry
for it claims cannot happen.
The trigger is a DB read failure or a profile that disappears mid-sync,
not an everyday path, so this is the rule being applied rather than a
bug anyone has hit. It is still the wrong answer to give.
The three copies of the chain are now one helper on
`commands/active_profile.rs` — the module the standard names as the home
for this, and already the place that turns a profile into a `Network`.
It returns `Option`: missing profile, DB error and an unparseable string
are one answer, because every caller refuses rather than guesses. The
existing `active_profile_network_from_conn` keeps defaulting to mainnet
for callers that only need a label.
The repair and discover steps now report no explorer through the path
they already had, and their message says both reasons it can fire rather
than only the configured-URL one. The SPV step returns early and says
which profile it could not read.
Gates: cargo fmt --check, cargo clippy --all-targets -D warnings,
cargo test (2575 passed).
`find_name_action_context` gathers the evidence that decides whether Reveal, Redeem and the ownership actions are offered, and five of its lookups degraded a failure into an empty answer. It returns `Result<_, AppError>`, so every one of them could have said so. The worst is the name hash. `hash_name` fails only on a name that is not valid, and the fallback substituted `[0u8; 32]` — a hash that matches no coin. Both coin queries below it are keyed by that hash, so any name that reached the fallback reported no reveal coins and no owner coin, which the capability model reads as "nothing to redeem" on a wallet that may have lockups sitting in losing reveals. `list_bid_commitments` failing the same way withdraws Reveal and Redeem from a wallet that has bid. CODING_STANDARDS: a DB failure in a user-triggered command is returned, not swallowed into a default. The bid-coin search was a `find_map` swallowing each commitment's error in turn; it is now a loop that stops at the first coin and returns the first error. This surfaced a test fixture that the zero hash had been hiding: `batch_capabilities_respects_wallet_profile_isolation` seeded "nameforA" and "nameforB", and Handshake names are lowercase — `verify_name` refuses an uppercase letter, so those rows were never hashable. Renamed to valid names; the assertions are unchanged. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2576 passed).
Follows the previous commit through the same function. Five more lookups in `find_name_action_context` turned a DB failure into an answer: - the pending-OPEN coin and draft checks read a failure as `false`, so a broadcast OPEN waiting for a block stopped suppressing the button that would send a second one; - `pending_broadcast_actions_for_name` read a failure as "nothing in flight", which is what every "waiting for a block" label is derived from; - the reveal draft's status read a failure as "no draft", which is the branch that falls back to chain facts; - `get_tracked_name_state` and `estimate_persisted_height` read a failure as "unknown", and unknown deliberately widens the transfer-lockup gate: the gate offers FINALIZE when it cannot see the chain, so a swallowed error offered an action consensus would refuse. `estimate_persisted_height` was already propagated with `?` further down this same file, so the two call sites disagreed about whether its failure mattered. Hashing the name moved out of the `.ok()` chain it was hiding in and is now the same `?` the previous commit introduced, which also flattens the nested closure the fallback needed. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2576 passed).
…tep 2)
The spec called Step 2 done — "route every site that reads node config
from global settings through the resolver; realign now runs only on
fallback resolution" — and several sites still read global.
`start_hsd` was the plainest: it spawned hsd with the api-key from global
settings while every read and broadcast used the profile's, so a profile
with its own key started a node it could not then talk to. It also ran
`realign_loopback_rpc_url` unconditionally, which is what ADR-001's
"Interaction with N11" exists to forbid: an override is an explicit
choice, and rewriting it is the silent abandonment ADR-002 refuses. Both
now come from `effective_node_config_for_profile`. `EffectiveNodeConfig`
was written for exactly this and had no reader outside its own module.
`from_override` meant "differs from global", not "the profile chose it",
so pinning a profile to the value global happens to hold read as no
override at all — and global can change under the user afterwards. It now
means what its name says. Realign needs a narrower question than that
one, since it rewrites the URL and nothing else, so `url_from_override`
answers it: overriding only `chain_source` no longer protects a stale
global loopback port from being fixed. Keys outside the node-config
tuple no longer count as a node override either.
Six paths fell back to the global node when a profile's own config would
not resolve, which ADR-001 names as the thing not to do: "a per-profile
override that fails is not a fallback — it is a user-facing error". The
status probe and the per-profile tip gate now report "cannot tell", and
`stop_hsd`, the paid-swap check and the action history return the error.
Fee estimation falls through to the built-in rate, because an estimate
from another chain's node is worse than no estimate.
That rewrite surfaced how those branches actually worked:
`get_active_profile_id` returns `Ok("")` when there is no active profile
and never `Err`, so the "no profile" case had been arriving through
`for_profile("")` failing and being swallowed. It is now an explicit arm,
and the `Err` arm means what it says — a DB failure.
The background sync gated "is the node authoritative?" on the global
node while every step it governs built a per-profile client, so the
answer described a node nobody was going to talk to. It now asks the
same per-profile gate the daemons use, which #55 added and left unused
here.
`node_config` ran raw SQL; both statements moved to `db::queries`, and
the `get_profile_settings` test moved with the helper.
Docs brought in line with the code: the spec carries a per-step status
table (Steps 1-2 of 9 done, and nothing writes `profile_settings` outside
tests yet, so the override is not reachable by a user), the specs README
lists it, and its dead prototype pointer is gone. CONTEXT.md described
per-slot read/write overrides and slot-independent resolution as current
behaviour; there is no slot column and the resolver returns one tuple, so
that is now marked planned.
Gates: cargo fmt --check, cargo clippy --all-targets -D warnings,
cargo test (2581 passed), npx vitest run (890 passed),
npm run lint:format.
Five fields of `NameActionCapabilities` were declared optional on the TS side with the reason written into the doc comment: "Optional so existing fixtures stay valid". The Rust struct always sends them — `auction_bidding_blocks` and `auction_reveal_blocks` as `Option<i64>`, `pending_broadcast_action` as `Option<String>`, and `stranded_bid_count` and `stranded_lockup_doos` as plain `i64` that cannot be absent at all. CODING_STANDARDS asks for the opposite trade: "Adding a field means: Rust struct + TS interface + every test fixture that builds that object", and fixtures are to be complete. Optional fields buy a quiet fixture at the cost of every consumer having to handle an `undefined` the backend never sends — and of the next field being added the same way. #61's own review fixed this for `nameIsRegistered` and `transferPending` and left these. Two fixtures were genuinely incomplete and now say what the backend says. Also in the bridge: the browser-QA mock's `get_settings` hand-listed twelve of the twenty-three settings, so `allow_remote_broadcast` among others read as `undefined` in browser QA and nowhere else, and `check_node_connection` had no handler at all — "Test connection" fell through to the unknown-command warning and returned null, which the caller reads as a failure it cannot explain. The mock now spreads the typed `DEFAULT_SETTINGS`, so a new setting cannot be forgotten there, and answers the probe. That import had to move: the settings store imports `invoke`, `invoke` imports the mock, and a mock reaching back into the store closed a cycle that left `DEFAULT_SETTINGS` undefined at module-init time. The defaults now live in a leaf module both sides import; the store re-exports them so existing importers are untouched. Two smaller ones from the same review: the sidebar's About link is an icon whose `title` was also its accessible name, and the tooltip conversion removed it without an `aria-label`; and `Tooltip`'s prop doc promised that empty content renders the children "bare, with no wrapper", while the code always renders the wrapper and explains three lines below why it must. Gates: npx tsc -b, npx vite build, npx vitest run (890 passed), npm run lint:format, lint:secure-imports, lint:native-title. Note: `activity-view`, `send-flow` and `wallet-view` each failed once in a full-suite run and passed alone and on re-run. Pre-existing flakes, not from this change; worth their own fix.
… code Three test-hygiene rules from CODING_STANDARDS, each broken in a way the file argued for in a comment. `cookie_vault`'s own tests serialized on a module-local mutex, while every test in `namebase_cmd_tests` serializes on the `serial_test` key `cookie_vault` — and both mutate the same process-global `TEST_DEK` and test-backend slots through the same setters. Two different locks around one piece of state exclude nothing. The comment explaining the choice said `#[serial]` "isn't a dep here"; `serial_test` is a dev-dependency, and its Cargo.toml entry describes this exact use. Both guards are gone and the six tests that touch those slots carry the shared key, so the two files now exclude each other. `node_status_data_dir_defaults_to_home_dot_hsd_when_prefix_unset` reads `$HOME` without `#[serial(hsd_home)]`, while the writer and every other reader carry it. The standard names this as the failure that made a test fail about one run in twenty, and #61 fixed it everywhere except this file. `#[cfg_attr(coverage_nightly, coverage(off))]` sat on six pure functions, which the standard forbids outright — "Never on a pure function — write a test instead". Three were already under test and the attribute was simply hiding that: `daemon_bin_name`, `provision_addresses` (a `_from_conn` shape) and `fetch_wallet_coins_and_txs_with_client`, whose own doc says "Testable against a mock without an AppState" three lines above the attribute saying it is not. `fetch_all` is the same `&dyn NodeRpc` seam and is driven through the poll path. The remaining two had no tests and now do: `output_names_from_pairs`, which pairs each Ledger output index with its covenant name, and `status_word_message`, which turns an APDU status word into what the user reads when a device refuses. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2586 passed, run twice for the serialization change).
`BidGate` decided whether the advanced auction section should draw the Bid and Lockup inputs, so that no phase where bidding is impossible could invite one. Its own doc says the predicate is "shared by BOTH call sites (the guided BIDDING panel and the advanced section)"; both are gone. `GuidedAction` renders `BidForm` directly, and #61's `resolveSections` answers the question a different way — a section that cannot apply is not rendered at all, so there is nothing left to gate. The component had no non-test caller and two test files. Its dedicated unit test goes with it: the behaviour it guarded is still pinned by `name-actions-bid-gate.test.tsx`, which drives the modal, and that is where a regression would now appear. Two assertions in that file looked for the absence of `bid-gate-placeholder` — a testid only `BidGate` ever rendered, so they could no longer fail; they are gone and the describe block says what the file actually holds. `build_batch_renew_draft_inner` and `build_batch_transfer_draft_inner` each took a `names` slice they did not read, with `let _ = names; // kept for API parity` to prove it. There is no parity to keep: `per_name` carries the names, and both functions build `batch_names` from it. The parameter and its five now-orphaned test bindings are gone. `derive_auction_task_state` carried `let _ = has_bid_commitment;` inside the BIDDING arm, which reads as though the parameter were unused. It is used four lines below, in the arm right after. Left alone deliberately: `DataTable`'s `onRowClick` and the `fromInteractiveChild` guard behind it still have no screen passing them, but the spec records that as an accepted gap, and reversing a decision the team wrote down is not a cleanup. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2586 passed), npx tsc -b, npx vitest run (884 passed), npm run lint:format.
A pass over every place the review found a comment arguing for behaviour the code no longer has. Two module headers had a coverage note pasted into the middle of a sentence, so `history.rs` read "...so spend attribution" and then, twelve lines later, "needs no extra roundtrips", and `secure_prompt.rs` cut item 4 of its lifecycle list in half. Both notes moved below the prose they interrupted. The quoted percentages went with them: they were stale the first time anyone touched either file, so what is written down now is the shape of the gap. `chain_synced` had lost its own documentation — two constants were inserted between the doc block and the function, so the 23 lines explaining the sync rule described a float. `pending_broadcast_actions_for_name` had two contradicting doc blocks stacked on it, one saying "the most recent wins" and the next saying to look at all of them; they belong to two different functions, and the singular one had no doc at all. Claims that were simply untrue: `namebase.rs` said the keyring-unavailable branch could not be produced deterministically, next to a test helper that produces it; `read.rs` linked a function deleted some commits ago; `chain_scan.rs` called the reveal-to-bid pairing "a best-effort heuristic; each bidder has one BID per name", directly above the exact-outpoint match that replaced it and the comment explaining why; `names.rs` kept an old paragraph calling the expiry constant "the single source of truth" above a new one saying live code reads something else; `OwnershipActions.tsx` described the "Buy with payment" button withdrawn in #62; and `read.rs` pointed at another file for a rule, which the standard names as a sign the rule should be shared instead. One of them was hiding dead code. `GuidedAction`'s BIDDING case returned a wait-for-reveal panel when the task state was `waitingForBidding`, with copy stating the one-bid-per-wallet rule. The BIDDING arm of `derive_auction_task_state` has returned `ReadyToBid` unconditionally since multi-bid, so that branch could not run against a real node. Its test built the impossible pair by hand; it now pins what actually happens — the bid form stays offered to a wallet that has already bid. The flag `alreadyBidWaiting` survives as `biddingNotOpenYet`, which is what `waitingForBidding` means now that only OPENING and a pending OPEN produce it. `Settings.tsx` carried a second implementation of the sync rule with a comment asking whoever changed one to remember the other. `node_status` already computes the shared verdict to pick its read source; it now reports it, the TS mirror carries it, and the screen reads it. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2586 passed), npx tsc -b, npx vitest run (884 passed), npm run lint:format, lint:native-title.
UI text, doc comments and `docs/*.md` may only claim what the code enforces. Six places claimed more. Paid name swaps were withdrawn in #62 and the spec explains at length why the shape could not work — but the manual still walked the user through both flows step by step, the README still listed the feature and promised "neither party can renege", the owned-name action table still offered "Buy with payment", and the QA checklist still asked a tester to find the buttons. All four now say the feature is gone, why, and what survives: an offer recorded before the withdrawal can still be claimed. The manual's batch-operations section described hsd's `createbatch` RPC and automatic chunking, followed by a note saying the description was inaccurate and would be rewritten later. Neither exists. The section now states the behaviour once: one transaction, one txid, capped at 100 names, no chunking. `NODE_SETUP.md` told SPV users to open a "Node mode" dropdown that #52 replaced with the Chain source selector, and said the network comparison only starts applying in Settings because onboarding has no wallet yet — which #54 changed, and onboarding now compares against the network picked on the previous step. The CHANGELOG entry for that carried the same stale clause. It also described per-slot read and write overrides and slot-independent resolution as how the wallet stores node config. There is no slot column and resolution returns one tuple; that is step 8 of the per-profile spec, now labelled as planned. And it documented no rule for when a node counts as synced, although that one rule decides reads, writes, the status panel and "Test connection". It does now, including both fallbacks and why a first-contact probe assumes the opposite of a configured node. Two spec pointers had gone stale: N14 named `NameInfoModal`, folded into `NameActionsModal` in #57, and R7 described the probe reading the network from the DB, which since #54 is the fallback for when the caller supplies none. The QA checklist named the deleted modal too. Gates: npm run lint:format.
`docs/specs/README.md` says a spec is the source of truth a reviewer checks the code against, and that when behaviour changes the spec changes in the same PR. Two gaps against that. #62 changed the multiple-bids feature in six places and touched only the new paid-swaps spec. Those six are now requirements, each naming the code that enforces it and the test that pins it: - R11b grew from two capabilities to five. Renew was ungated although hsd's RENEW handler ends a transfer exactly as UPDATE does, and Transfer was offered on a name already being transferred; Revoke stays offered because consensus allows it and destroying the name is what the button says. - R11c, the FINALIZE lockup gate, including why it prefers the live tip and why an unknown height leaves the action offered. - R11d, the `batch-` prefix strip, without which a batched transfer read as no transfer at all. - R13 gained the Owned Names State column, the fourth surface reading `pendingBroadcastAction` and the last one that did not. - R13b, TRANSFER as a task rather than a phase hsd never reports, and where it ranks. - R13c, REDEEM classified as spendable, which is what made reclaimed lockups visible in the balance again. - R13d, the confirm dialog summing every non-change output, which replaces a Known gap the same commit had closed and left standing. Batch name operations had no spec at all: six commands, a frontend selection flow, a confirmation modal, a user-manual section and a QA checklist, shipped in #53 across every layer the README says requires one. `docs/specs/2026-09-15-batch-name-operations.md` records what is enforced (one transaction and one txid, the 100-name cap and why that number, abort-before-write on a bad name, atomic reservation, a batch of one taking the same path) and what is not (no chunking, no per-name result, no cross-covenant batching). Every pointer in it resolves. Also in the registry: the new spec is listed, and three rows named the feature branch they shipped on rather than saying whether the behaviour is in the product. Gates: npm run lint:format. No code changed.
Layering first. CODING_STANDARDS names `commands::read` a legacy shared layer and says not to add to it; #53 and #55 added three node-readiness helpers, and `chain_scan`, `sync`, `names`, `deadlines` and the watched-name daemon all reach into it for them. They now live in `commands/node_readiness.rs` — the "small dedicated helper module under commands/" the standard points at, of which `active_profile.rs` is its own example. `estimate_persisted_height` moved with them: it answers what to believe about the chain height when no node does, which is the same question with the node taken away, and it was the only other reason `names.rs` imported the legacy layer. Nothing about the functions changed. What remains of `commands::read` in sibling commands is `resolve_profile` and two pre-existing imports. Then three things written down more than once. `node_config::DEFAULT_NODE_RPC_URL` spelled out mainnet's loopback port beside `Network::default_rpc_url`, which builds the same string from the per-network port table. It now calls it, and its doc says why mainnet is the only defensible default in a resolver that deliberately does not consult the profile's network. "Allow sending via remote node" was hand-rolled in both Onboarding and Settings, down to a shared `data-testid` that would match twice if a page rendered both. The two differed only in text size and whether they showed the explanatory line, so those are props on one `AllowRemoteBroadcastToggle`. This is a safety opt-in whose real enforcement is in `broadcast_tx_draft`; the least the two screens can do is ask the same question. The "no explorer available" message existed in four wordings across the read and sync paths, including two I introduced earlier on this branch. It is one constant now, next to the factory that decides there is no explorer, covering both reasons it refuses. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2586 passed), npx tsc -b, npx vite build, npx vitest run (884 passed), npm run lint:format, lint:secure-imports, lint:native-title.
…imes CODING_STANDARDS asks tests to "build complete fixtures (every field of the type, no partials)". Three files built `NameActionCapabilities` some other way: one cast with `as unknown as`, which switches the type check off entirely, and two returned untyped literals from a mocked `invoke`, which TypeScript never saw at all. They share one builder now, in `src/test/fixtures/`, alongside the settings fixture that already works this way. It is the complete-object builder `nameSections.test.ts` had written locally — the one file doing this right — so that one now imports what it used to own. Switching the type back on found two fixtures describing a backend that does not exist. `name-actions-bid-gate` sent `taskState: "none"`, which is not a member of the type. And both it and `auction-ux` paired a BIDDING phase with a task state the BIDDING arm stopped producing when multiple bids became allowed; they now carry what the backend derives, and `auction-ux` asserts the same things about it. Three notification opt-ins — updates, deadlines, watchlist — were written out in full in `Settings.tsx`, identical but for a setting key, a label, two testids and one sentence. Three places to fix a permission bug, and three chances for the notices to drift. One `NotificationToggle` owns the checkbox, the permission request that must originate from the click, and the three notices that follow from the answer; each section passes its own sentence and whatever extra fields it has. `Settings.tsx` loses 125 lines. Two duplications that should stay duplicated, now checked rather than asked about: - `src/lib/utils.ts::defaultNodeRpcUrl` re-spells hsd's per-network RPC ports for a placeholder in Settings, under a comment asking whoever edits `Network::default_rpc_url` to remember it. A round-trip for grey hint text would cost more than the bug, so the copy stays — but a test in `contract_shape_tests`, the module that exists for exactly this, reads the TypeScript and fails if the tables disagree. - Migration 030 backfills using per-network offsets spelled out as 2197, 469 and 21, derived from `NameParams`. Deriving them live would be wrong: a migration transforms rows written under the rules of their own time, and a later consensus change must not silently rewrite history differently. A test asserts the arithmetic still matches, so editing `NameParams` tells you a migration records the old values instead of leaving you to find out. It also pins the assumption the migration's `CASE` rests on — that the schema cannot store a simnet profile. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2588 passed), npx tsc -b, npx vite build, npx vitest run (884 passed), npm run lint:format, lint:secure-imports, lint:native-title.
The rule that owning a name is not yet the right to act on it — `owner_covenant_type >= COV_REGISTER` — was written out four times: three in the capability gates and once in `sign_name_message`, the last under a comment saying "same rule as the ownership capabilities", which is a claim rather than a fact. It is now one predicate in `noncustodial`, where the covenant constants live, with the consensus reasoning in one doc and a test over every covenant either side of the line. Four spellings of a consensus fact is how the gates and the signer came to disagree about "owned" in the first place. The same for which auction a bid commitment belongs to: `names.rs` had it as a closure, `read.rs` as an inline match, and the permissive treatment of two different unknowns had to be reasoned out twice. It is a method on `BidCommitmentRow` now — the row answering a question about itself — where the reason only a recorded mismatch excludes is written once. `u32le_hex` and `doos_from_hns` stay in `chain_scan`: they have one consumer, so the sharing rule does not reach them. They had no tests, including the over-four-bytes branch that keeps a name hash from being read as a height. They do now. Two more places where duplication is the right answer and the plea for vigilance was not. The auctions table seeds the name modal's capability cache and spelled the query key out again, asking for the two to be kept identical; the query now exports the key and the table calls it. And `ActivityView`'s list of value-re-homing actions is deliberately a subset of the backend's labels — that part is the screen's decision — but the spellings are the backend's, so a Rust test reads the list and fails if any entry is not a label `classify_tx` emits. `ActionHint` stays a one-line wrapper around `Tooltip`. What it adds is the name: `reason` says a capability refused, where `content` would say only that some text exists. Its doc claimed `Tooltip` "renders nothing extra" for an empty hint, which is the same inaccuracy corrected in `Tooltip` itself earlier on this branch. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2594 passed), npx tsc -b, npx vite build, npx vitest run (884 passed), npm run lint:format, lint:secure-imports, lint:native-title.
`start_hsd` refuses rather than defaulting when it cannot tell which network a profile is on — its own comment says spawning is "the one place where guessing mainnet is an action with consequences: it starts a full mainnet chain sync into a data dir the user prepared for another network". Three lines below that, it called `resolve_data_dir`, which reads the network again through the reader that degrades to mainnet, and that is the directory the chain gets written into. It now derives the path from the network it already refused to guess. `resync_hsd_chain` had the same shape and more at stake. It stops the node, moves `blocks`, `chain` and `tree` into a timestamped backup and starts a fresh sync — and it resolved the data dir and the network as two separate defaulting reads, so they could disagree, with the backup aimed at whichever answered first. With no active profile both said mainnet, and the command would back up and re-sync over a mainnet directory for a wallet that is not on mainnet. It resolves the network once now and refuses when there is none, like its sibling. `resolve_data_dir` keeps defaulting, which is right for the status panel that reports a path rather than acting on one. Its doc now says so, and says which callers must not use it. The existing resync test's comment claimed there was no active profile and that mainnet was therefore assumed; the harness has seeded one since `start_hsd` grew its guard. Corrected, and the new refusal has its own test. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2595 passed), npm run lint:format.
`derive_auction_task_state` decides which task a name is asking the user
to do — reveal, register, redeem, renew, finalize — and took thirteen
positional parameters to do it. Six were `bool`. Swapping two adjacent
ones compiled silently and changed the answer, and there was nothing at a
call site to say which was which; three test helpers had resorted to
trailing `// has_bid_commitment` comments to keep their place in the
list.
Nine of those facts already travel together in `NameActionContext`, which
the production caller was unpacking field by field only to pass them
straight through. The function takes the context now and reads them by
name. A tenth, whether a transfer is in flight, was
`ctx.transfer_has_items.unwrap_or(false)` at the call site and is now
resolved inside, where the reasoning about why it cannot come from the
phase already lives.
Five parameters remain and only one is a bare `bool`, so there is no
longer a pair to transpose.
`NameActionContext` derives `Default`, which is the no-evidence state the
capability model already falls back to. A test now states the one or two
facts it is about — `NameActionContext { has_pending_open: true,
..Default::default() }` — instead of counting commas to the ninth
argument.
Thirty-seven call sites moved, thirty-three of them tests. The mapping
from position to field name is what the suite verifies: a wrong one shows
up as a failing assertion, not as silence.
The function is `pub(crate)` now. It was `pub` while taking a
`pub(crate)` type, which clippy objects to and which claimed a reach it
never had.
Gates: cargo fmt --check, cargo clippy --all-targets -D warnings,
cargo test (2595 passed), npx tsc -b, npx vitest run (884 passed),
npm run lint:format.
Four sites still resolved the network through the reader that degrades to `Network::Main`, which is the default the enum derives. Three used it to pick an explorer, and that is where it hurts: the explorer factory hands back the mainnet explorer for `Main` and `None` for every other network, so an unknown network did not merely mislabel anything — it turned the "no explorer for this chain, serve nothing" path into a live request to mainnet. A testnet or regtest wallet could be shown a mainnet name's auction phase, its bids and its DNS records. The guard added earlier on this branch does not help here: it refuses the known mainnet URL only when the network is known not to be mainnet. `read_name_info`, `read_name_bids` and `get_resource` now treat an unreadable network as unknown, and every one of them already had a no-explorer path to fall into. Two of those commands are handed a profile id, so they ask that profile's network rather than the active one, which is also the correct answer when the two differ. The fourth site keys the chain-scan cursor and index, both of which are network-scoped by migration 028 — its own comment says "a cursor from another chain says nothing about this one's coverage" while reading it with a resolver that could name another chain. With no known network the coverage reads as zero, so the command falls through to the explorer the way an un-caught-up scanner already does. Separately, `get_write_capability` resolved its comparison network with `.ok().flatten()` and no justifying line. A DB failure there produced the same `None` as "no profile yet", which the chain check treats as nothing to compare, so the wallet reported itself ready to send through a node whose chain had never been checked. It is returned now. The label was the whole exposure: `broadcast_tx_draft` resolves the network from the draft's own profile and propagates the error, so nothing could actually be broadcast cross-chain. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2597 passed), npm run lint:format.
`is_fatal_startup_line` decides whether the log tail shown to a user says their node failed to start or says nothing at all, and it was a closure inside an IO function with no test anywhere. The standard asks for pure logic to be split out so it can be tested; it is a `pub(crate)` function now, and its tests give it real hsd log shapes on both sides. Writing them settled what the trade-off is. The predicate runs only when the node's RPC is already unreachable, so over-reporting relabels "still starting" on a node that is down either way, while under-reporting leaves a genuinely broken node silent. The broad matchers therefore stay broad, including `Cannot `, which would match a benign "Cannot find …" outside a tagged module line — and a test now pins that as a choice rather than an oversight. One matcher goes. `bind` on its own was there for the address-in-use failure, which arrives as `bind EADDRINUSE` and is caught by the `EADDRINUSE` matcher beside it; the bare substring also matched "binding", "rebinding", and any data-dir path with those letters in it, so a node opening `/Volumes/bind-drive/hsd-data` reported itself broken. Also: `set_profile_override` existed five times — four test files and the watched-name daemon's own test module. Four were byte-identical and the fifth used `INSERT OR REPLACE`, which on an existing row rewrites the whole row rather than the value. That divergence is the argument: one copy now lives in `tests::command_helpers`, which the daemon module can reach now that it is `pub(crate)`. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2601 passed).
Three behaviours from `d053c141` were enforced in code and written down nowhere a reviewer checks. The specs README says a spec is the source of truth the code is checked against, and that behaviour changes with the spec in the same PR. The sync rule goes in the 2026-09-11 spec as R6b, beside R6, which already owns the other half of the same question: R6 says what a first-contact probe assumes when a node reports nothing, R6b says how the verdict is reached when it reports something. It records the tip as the ground truth, why `verificationprogress` only corroborates, both fallbacks, and that one function answers for every gate so the label cannot disagree with what reads do. Per-network data dirs go in the 2026-09-14 spec as N16 and N17, next to N15, which already refuses to guess a network when starting a node. N17 is the one worth having written down: `migrate_network_prefix` is the only place the wallet relocates a user's chain files, and it had no record at all. The entry states each of its refusals — never mainnet, never over an existing chain, idempotent. N18 records what the wallet claims when a node fails to start, including that the matching is deliberately broad and why, so the next person to tighten it knows what they are trading. Migration 010's comment named `chain_source` and `allow_remote_broadcast` as reintroduced and omitted `hsd_prefix`, which 011 re-seeds — empty, not with the user's value. A database upgrading across that point loses its configured hsd data directory and falls back to `~/.hsd`, so the chain looks like it vanished. The migration is left alone, for the reason the comment now gives: editing a shipped migration changes history for databases that have not reached it and does nothing for those that have. Gates: npm run lint:format, cargo test (migration suite). No code changed.
Three duplications the review flagged and the second pass confirmed were worth closing. The block that prefers a live tip over the persisted estimate appeared in two branches of the capability evaluator, four lines of code under three lines of identical comment. It is `NameActionContext::with_live_tip` now, on the type it is about, with the reasoning in one place. Copied prose is how a comment comes to describe only one of the branches it sits in. `Vec<(String, [u8; 32], queries::NameCoin, NameState)>` was written out in four signatures and the five-element bid variant in four more. The repo had already decided how to handle this — `PerNameFinalize` exists — and just had not applied it. `PerNameOwner` and `PerNameBid` join it, so all three batch prefetch shapes are named and each says which of two coins and which `[u8; 32]` it carries. The sync guard and the broadcast guard each built their own "node is on network X but this wallet is Y" refusal, so which sentence the user read depended on which fired. One builder takes the only part that legitimately differs: what did not happen, and what was left alone. The third such message stays where it is — it is a `WriteCapability` reason with a Settings navigation hint, a different artefact for a different surface. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2601 passed).
`statusBadge` had an `onchain` arm returning a placeholder under the comment "Handled by the caller", and the caller then re-derived variant, label and hint from `row.confirmed` — three conditionals restating what the badge is for. "Onchain" is not a state a user can act on anyway: a row the node has seen is either in a block or waiting for one. The function takes that fact now and returns the real answer, and the caller destructures what it asked for. The network picker existed twice, in onboarding and in Add wallet, with the same three options. They had already drifted: only the onboarding copy carried a `data-testid`, so only one was reachable from a test, and only it reset the connection probe on change. Which networks exist is a fact about the wallet rather than about either screen, so it is one component; resetting the probe stays with onboarding, where the probe lives, and Add wallet gains the testid it was missing. Gates: npx tsc -b, npx vite build, npx vitest run (884 passed), npm run lint:format, lint:secure-imports, lint:native-title.
Found reviewing my own changes on this branch against the rule they were enforcing. When the per-profile readiness gate replaced the global one earlier here, it kept the shape the old code had: read the profile's network, and pass whatever came back — including `None` on a failure. `None` tells the readiness gate there is nothing to compare. That is the right answer during onboarding, when no profile exists yet, and the wrong one for a profile that has a network and simply could not be read: the node is then declared authoritative without its chain ever being checked, and the steps behind that gate seed this profile's cache from it. The comment three lines above says a node on another chain must never be authoritative for this one, which is exactly what it then allowed. An unreadable profile now makes the node not authoritative, so the sync falls back to the explorer path, which is the conservative one and already exists. Gates: cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test (2601 passed).
…gate Review of #51-#62 on the standards axis. - build_redeem_draft swallowed a get_name_coin failure into "no owner coin", which dropped the filter that keeps the winning REVEAL out of the redeem transaction (bad-redeem-owner would then reject the whole tx). It is a user-triggered command: the failure is returned. - The action-context builder and estimate_persisted_height defaulted an unreadable profile network to mainnet. Both now use one shared active_profile::profile_network_from_conn, which is also what read.rs used to keep privately; on regtest the mainnet guess aged heights by a wall clock the chain does not keep. - node_tip_height_if_synced and the watched-names daemon turned a DB failure into "no network to compare", which let a node on another chain count as ready. Same shape sync.rs was fixed to in 7c66925. - The tests that reach hsd_candidates / pid_file_path (HOME readers) carry the hsd_home serial key the writer already has. - pending_broadcast_action_for_name is the head of the plural form instead of a second copy of the query; the six owner-action reasons share one owner_gate; test comments name functions instead of line numbers. Gates: cargo fmt, cargo clippy --lib --tests -D warnings, cargo test --lib (affected modules).
Spec 2026-09-11 N3 named the code that refuses to sync a mainnet profile from a regtest node but no test; multi-bid R11c said the finalize gate prefers a live tip and pinned only the lockup arithmetic. Both are now pinned: the sync test also asserts the coin route is never asked and no foreign height reaches sync_cursors.
…h signs Review of #51-#62 on the standards and spec axes, frontend. - "Shakeshift indexes mainnet only" was spelled as a network literal in six places across four screens. explorerCoversNetwork in lib/openExternal is now the one home, so the screens cannot drift on it. - The five handleBatch* handlers were one shape with five copies, four of which reported a failure as a raw ${e}. One runBatch, one mapError. - BatchConfirmModal now states the amount the batch moves (every output except change), which spec B9 said it already did. batch-transfer pins it. - NameActionsModal: run and handleRevealConfirm shared the whole build -> track -> exec -> discard path and differed only in what happens after broadcast; that is now a callback. The two hooks for the withdrawn paid-swap commands are gone. Stale header and gate comments fixed. - Types: expectedNetwork is a WalletNetwork, settingKey is a keyof Settings, update_notify_enabled is required (the store always merges its default). - CI runs lint:native-title, which the standards table and R17 said it did. - action-reason-banner and name-details tests drive the real hooks through a per-command invoke mock with complete fixtures (new fixtures/wallet.ts, fixtures/nodeStatus.ts) instead of partial hook mocks cast to any. Gates: tsc -b, vite build, vitest (884), prettier, lint:secure-imports, lint:native-title.
Spec-axis follow-ups from the #51-#62 review. - The node gate moved to commands/node_readiness.rs; the remote-node and network specs, the ADR and their Terms/Pointers still named read.rs. The remote-node spec's status named a branch that no longer exists. - Network spec: N3 names the test that now pins it; the explorer-fallback bullet records the seeded-mainnet-URL refusal and migration 032 shipped in 94b5836; the from_settings count is current; the read-gate bullet says which callers fail closed on an unreadable network and which one degrades, since 7c66925 and this branch changed that. - Multi-bid spec: R6 points at the two reveal builders that stamp the txid (actions.rs never did); R12 names has_pending_open_coin as the local it is; R11c lists the two tests it had left out; R13b states that a pending transfer outranks a redeem and why. Batch spec B9 is now true (amount row), so no change there. - USER_MANUAL: synced follows the chain-tip rule, not a progress percentage; the Chain source row describes sending and says reads are routed by readiness; the SPV section no longer refers to a Node mode dropdown; batch operations list Transfer Selected and the amount line; a new section covers several bids on one name and stranded lockups. README qualifies node-free reads as mainnet-only. QA_WORKFLOWS gains the transfer row. - Three code comments said verificationprogress wins; chain_synced says the tip decides. chain_scan's header named the settings gate it no longer uses. - CHANGELOG: the batch amount line and the redeem/readiness fixes.
github-actions
Bot
added
domain: wallet
github-actions
Bot
added
domain: sync
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-ups from a two-axis review (standards + spec) of PRs #51–#62 — the coverage push, the remote-node/network work, per-profile nodes, multiple bids, the transfer lifecycle and the paid-swap withdrawal. The review checked the current state of every file those PRs touched, not the individual diffs.
Nothing here adds a feature. Each commit either closes a gap between what the code does and what the spec / manual claims, or removes a way for two places to disagree about the same rule.
Correctness
build_redeem_draftswallowed aget_name_coinfailure into "no owner coin", which silently dropped the filter that keeps the winning REVEAL out of the redeem transaction — consensus then rejects the whole tx (bad-redeem-owner). The action-context builder andestimate_persisted_heightdefaulted an unreadable profile network to mainnet; on regtest that aged heights by a wall clock the chain does not keep. All three now use one sharedactive_profile::profile_network_from_conn(previously a private copy inread.rs) and propagate.node_tip_height_if_synced, the watched-name daemon, and the background sync each turned a DB failure intoNone, which the readiness gate reads as "skip the chain check" — so a node on another chain could count as ready. Same shape as the fix insync.rs; now applied everywhere the answer is acted on.Tests
sync_wallet_state_refuses_a_node_on_another_chainpins N3 (previously code-only): the coin route is never asked and no foreign height reachessync_cursors.a_live_tip_replaces_the_persisted_estimatepins the half of R11c the spec named but no test covered.hsd_candidates/pid_file_path(HOME readers) now carries thehsd_homeserial key the writer already had.ActionReasonBannerandNameDetailsdrive the real hooks through a per-commandinvokemock with complete fixtures (fixtures/wallet.ts,fixtures/nodeStatus.ts) instead of partial hook mocks cast toany.lint:native-titleruns in CI. The standards table and R17 said it was a build gate; it was only a script.One home per rule
explorerCoversNetworkinlib/openExternal.tsreplaces sixnetwork === "mainnet"literals across four screens.runBatchreplaces fivehandleBatch*copies (four of which reported errors as a raw${e}).runandhandleRevealConfirmin the name modal shared the whole build → track → exec → discard path; the only difference (close vs. stay open) is now a callback.owner_gateclosure replaces the six copies of the "not owned / not registered" cascade inbuild_name_action_capabilities;pending_broadcast_action_for_nameis the head of the plural query instead of a second copy of it.bool) became a struct; a dozen small helpers that nothing called are gone.Spec and docs
BatchConfirmModalshows the amount the batch moves (every output except change) — the spec said it did.commands/node_readiness.rs(the gate moved there); R6 names the two reveal builders that actually stamp the txid; R12 nameshas_pending_open_coinas the local it is; R13b states that a pending transfer outranks a redeem and why; N3 and R11c name their tests; thefrom_settingscount and the explorer-fallback bullet match the code.verificationprogresswins;chain_syncedsays the tip decides.Deliberately left alone
PrefixMigration::AdoptLegacySubdircarrying its path instead of anexpect: judgement call, would churn 16 tests."1"/"0"vs"true"/"false"settings: two real persisted encodings; unifying needs a migration on both sides.Gates
cargo fmt --check,cargo clippy --all-targets -D warnings,cargo test --lib(2605 passed)tsc -b,vite build,vitest run(884 passed)prettier --check,lint:secure-imports,lint:native-title