feat(wallets): quorum-aware recovery identity — useSigner selects the held member (M4-4 part 4/4) - #1996
Conversation
🦋 Changeset detectedLatest commit: 186a28a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if (recovery.locator != null && recovery.locator !== quorumLocator) { | ||
| throw new Error( | ||
| `Quorum locator "${quorumLocator}" does not match this wallet's quorum admin signer ("${recovery.locator}").` | ||
| ); | ||
| } |
There was a problem hiding this comment.
quorumLocator guard silently skipped when quorum has no locator
The validation fires only when recovery.locator != null, so if the API returns a quorum config without a locator field (the field is typed as locator?: string), any caller-supplied quorumLocator passes unchecked. A caller who deliberately passes quorumLocator: "quorum:deadbeef" expecting a safety guard gets no error — the code proceeds as if the locator matched. Since quorumLocator is explicitly described as a disambiguation tool, silently accepting a wrong value undermines its purpose.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1029-1033
Comment:
**`quorumLocator` guard silently skipped when quorum has no locator**
The validation fires only when `recovery.locator != null`, so if the API returns a quorum config without a `locator` field (the field is typed as `locator?: string`), any caller-supplied `quorumLocator` passes unchecked. A caller who deliberately passes `quorumLocator: "quorum:deadbeef"` expecting a safety guard gets no error — the code proceeds as if the locator matched. Since `quorumLocator` is explicitly described as a disambiguation tool, silently accepting a wrong value undermines its purpose.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (matches.length > 1) { | ||
| // Only passkeys can match several members: id-less, name-less configs match permissively. | ||
| throw new Error( | ||
| "Multiple passkey members are in this wallet's quorum admin signer. " + | ||
| 'Specify the credential id or name: wallet.useSigner({ type: "passkey", id: "<credential-id>" })' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Multi-match error message hardcodes "passkey" regardless of type
matches.length > 1 is only expected for passkeys in practice (because { type: "passkey" } without id/name permissively matches all passkey members), but other types could theoretically trigger this too — an { type: "email" } candidate with no email field matches all email members via the candidate.email == null guard in matchesQuorumMember. The current error message and suggestion are misleading for any non-passkey case.
| if (matches.length > 1) { | |
| // Only passkeys can match several members: id-less, name-less configs match permissively. | |
| throw new Error( | |
| "Multiple passkey members are in this wallet's quorum admin signer. " + | |
| 'Specify the credential id or name: wallet.useSigner({ type: "passkey", id: "<credential-id>" })' | |
| ); | |
| } | |
| if (matches.length > 1) { | |
| // Only passkeys can match several members: id-less, name-less configs match permissively. | |
| // Other types could also multi-match if the candidate omits identity fields (e.g. email == null), | |
| // so build the message from the actual type rather than hard-coding "passkey". | |
| const typeHint = | |
| signer.type === "passkey" | |
| ? 'Specify the credential id or name: wallet.useSigner({ type: "passkey", id: "<credential-id>" })' | |
| : `Specify the identifying field for type "${signer.type}" to disambiguate.`; | |
| throw new Error( | |
| `Multiple ${signer.type} members are in this wallet's quorum admin signer. ${typeHint}` | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1004-1010
Comment:
**Multi-match error message hardcodes "passkey" regardless of type**
`matches.length > 1` is only expected for passkeys in practice (because `{ type: "passkey" }` without `id`/`name` permissively matches all passkey members), but other types could theoretically trigger this too — an `{ type: "email" }` candidate with no `email` field matches all email members via the `candidate.email == null` guard in `matchesQuorumMember`. The current error message and suggestion are misleading for any non-passkey case.
```suggestion
if (matches.length > 1) {
// Only passkeys can match several members: id-less, name-less configs match permissively.
// Other types could also multi-match if the candidate omits identity fields (e.g. email == null),
// so build the message from the actual type rather than hard-coding "passkey".
const typeHint =
signer.type === "passkey"
? 'Specify the credential id or name: wallet.useSigner({ type: "passkey", id: "<credential-id>" })'
: `Specify the identifying field for type "${signer.type}" to disambiguate.`;
throw new Error(
`Multiple ${signer.type} members are in this wallet's quorum admin signer. ${typeHint}`
);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI### Issue 1
packages/wallets/src/wallets/wallet.ts:1029-1033
**`quorumLocator` guard silently skipped when quorum has no locator**
The validation fires only when `recovery.locator != null`, so if the API returns a quorum config without a `locator` field (the field is typed as `locator?: string`), any caller-supplied `quorumLocator` passes unchecked. A caller who deliberately passes `quorumLocator: "quorum:deadbeef"` expecting a safety guard gets no error — the code proceeds as if the locator matched. Since `quorumLocator` is explicitly described as a disambiguation tool, silently accepting a wrong value undermines its purpose.
### Issue 2
packages/wallets/src/wallets/wallet.ts:1004-1010
**Multi-match error message hardcodes "passkey" regardless of type**
`matches.length > 1` is only expected for passkeys in practice (because `{ type: "passkey" }` without `id`/`name` permissively matches all passkey members), but other types could theoretically trigger this too — an `{ type: "email" }` candidate with no `email` field matches all email members via the `candidate.email == null` guard in `matchesQuorumMember`. The current error message and suggestion are misleading for any non-passkey case.
```suggestion
if (matches.length > 1) {
// Only passkeys can match several members: id-less, name-less configs match permissively.
// Other types could also multi-match if the candidate omits identity fields (e.g. email == null),
// so build the message from the actual type rather than hard-coding "passkey".
const typeHint =
signer.type === "passkey"
? 'Specify the credential id or name: wallet.useSigner({ type: "passkey", id: "<credential-id>" })'
: `Specify the identifying field for type "${signer.type}" to disambiguate.`;
throw new Error(
`Multiple ${signer.type} members are in this wallet's quorum admin signer. ${typeHint}`
);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "useSigner with quorums" | Re-trigger Greptile |
da22107 to
87e7fca
Compare
|
Reviews (2): Last reviewed commit: "feat(wallets): quorum-aware recovery ide..." | Re-trigger Greptile |
… held member
useSigner now resolves a config against the wallet's quorum members and
assembles the matched member as an admin signer under its API locator, with
an optional { quorumLocator } to force the member interpretation. Device-signer
recovery reuses a member previously selected this session, replacing the last
QuorumSignerNotSupportedError guards with working flows or actionable errors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2cada2f to
b46b40a
Compare
87e7fca to
186a28a
Compare
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
|
Reviews (3): Last reviewed commit: "feat(wallets): quorum-aware recovery ide..." | Re-trigger Greptile |
Linear: WAL-11292 · EDD §6.5 · Part 4/4 of the M4-4 stack — split for reviewability into #1997 (shared member matcher), #1998 (SignerManager/resolver plumbing), #1999 (factory runtime-config preservation), and this PR (the user-facing flows). The stack's union is byte-identical to this PR's original single-commit form.
Summary
Per EDD §6.5, the caller never selects "the quorum" — it selects, via
useSigner, the member it holds. This PR makes that real on top of the stack's plumbing: the remainingQuorumSignerNotSupportedErrorguards are replaced with working flows or actionable errors. Launch scope is 1-of-n, but nothing here counts thresholds — the same code path serves m-of-n when the API gate lifts.useSigner(signer, options?)matches the config against the quorum's members (via the refactor(wallets): extract shared quorum member matching (M4-4 part 1/4) #1997 matcher) and assembles the member as an admin signer from the API-merged config (adoptQuorumMemberConfig, feat(wallets): quorum member plumbing in SignerManager and server resolver (M4-4 part 2/4) #1998), so the adapter's locator equals the API member locator the approval loop matches on. Precedence: member-first for email/phone/external-wallet, registration-first for passkey/server. New optionaloptions.quorumLocator(exportedUseSignerOptions) forces the member interpretation for a key that is both a member and a delegated signer.resolveServerSignerroutes quorum matching through the resolver so the correct (primary or legacy) derivation is cached, then strips the member's secret.#resolveResumeRecoveryConfigreuses a member the caller already selected viauseSignerthis session (adoptedAssemblableQuorumMember, feat(wallets): quorum member plumbing in SignerManager and server resolver (M4-4 part 2/4) #1998); never silently picks one — otherwise an actionable error listing the member locators.Review guide
src/wallets/wallet.ts—useSigner/resolveNonDeviceSigner: the merged-config idea and the precedence rules;resolveForcedQuorumMemberfor thequorumLocatorpath.src/wallets/services/device-recovery-service.ts—#resolveResumeRecoveryConfig(adopted-member-or-error).src/wallets/wallet.quorum-recovery.test.ts— the flipped "quorum member selection" suite is the behavioral spec; theuseSigner → approveend-to-end test proves the chain joins up.Compatibility
Old-vs-new dist
.d.tssurface diff: one added export (UseSignerOptions), one signature change (useSignergains an optional trailing param), nothing removed.QuorumSignerNotSupportedErrorstays exported (M4-6 may reuse it) but is no longer thrown. All new branches are gated onrecovery.type === "quorum"— single-admin flows are byte-for-byte unchanged (125 pre-existingwallet.test.tstests untouched). Changeset (minor) covers the whole stack and rides here.Testing
packages/walletsunit suite: 735 passed (29 files) at the top of the stack.wallet.quorum-recovery.test.ts(21 tests incl. e2e approve under member locator,quorumLocatorcases, multi-passkey disambiguation, server member secret-strip),device-recovery-service.test.ts(resume with/without adopted member).tsc --noEmitclean; biome clean; dist rebuilt andreact-basetypechecks against it.🤖 Generated with Claude Code