feat(wallets): widen create-side recovery to accept a quorum object#1989
Open
Conversation
Widen WalletCreateArgs.recovery to also accept a quorum config
({ type: "quorum", threshold?, methods }) whose members exclude api-key
and device signers. createWallet preps each member through the existing
per-type prep (passkey credential creation, server-signer address
derivation) and forwards the quorum on the `recovery` wire property;
a single-method quorum collapses to a plain admin signer and empty
methods are rejected. Existing-wallet validation compares threshold and
member set order-insensitively. Single-signer payloads are unchanged.
WAL-11290
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8154d93 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 |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/client/react-base/src/providers/CrossmintWalletBaseProvider.tsx:255-262
**WebView init skipped for quorum email/phone members**
`initializeWebViewIfNeeded` only checks the top-level `signer.type`. When the recovery is `{ type: "quorum", methods: [{ type: "email", ... }] }`, the outer type is `"quorum"` — neither `"email"` nor `"phone"` — so `initializeWebView` is never called. Any email/phone OTP flow inside a quorum recovery will then try to use the WebView frame before it has been initialized, likely throwing at runtime. The PR description acknowledges this is deferred to a follow-up ticket, but worth tracking explicitly so the ticket includes walking `methods` for quorum inputs.
### Issue 2 of 2
packages/wallets/src/wallets/wallet-factory.ts:498-506
**Passkey fallback returns `true` for any candidate when neither `id` nor `name` is set**
When a passkey member has neither `id` nor `name`, `isMatchingQuorumMember` returns `true` for the first passkey found in `unmatched`. With multiple passkey members all lacking identifiers, the consume-once greedy loop will pair them with existing passkeys in list order rather than by content, so `compareSignerConfigs` may be called on a mis-matched pair. In practice `compareSignerConfigs` is then the sole safety net; the pre-selection step provides no additional guarantee. Consider throwing (or at least logging a warning) when a quorum contains ≥ 2 passkey members without identifiers, similar to how `isMatchingPasskeySigner` already throws for the delegated-signer case.
Reviews (1): Last reviewed commit: "feat(wallets): widen create-side recover..." | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet-factory.ts:252-255
Collapsed single-member quorum loses runtime data for `server`/`external-wallet` members.
`createArgs.recovery.type` is always `"quorum"` here when the user passes a quorum config, so the `"server" || "external-wallet"` guard never fires even after `normalizeRecovery` already collapsed the quorum to its sole member. The wallet instance is therefore built from `apiRecovery` — a server signer without the secret, or an external-wallet signer without the `onSign` callback — causing all subsequent recovery operations to fail with "no secret available" / no signing function.
```suggestion
// Normalize before the type check so that a single-member quorum that was collapsed
// to a server or external-wallet signer still preserves its runtime data (secret / onSign).
const normalizedRecovery =
createArgs.recovery != null ? this.normalizeRecovery(createArgs.recovery) : undefined;
const recovery =
normalizedRecovery?.type === "server" || normalizedRecovery?.type === "external-wallet"
? normalizedRecovery
: apiRecovery;
```
Reviews (2): Last reviewed commit: "type fixes" | Re-trigger Greptile |
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
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
Implements WAL-11290 (M4-2, per EDD §6.2/§6.3 option B). Stacked on the M4-1 OpenAPI regen branch (WAL-11289).
WalletCreateArgs.recoverynow also accepts a quorum config{ type: "quorum", threshold?, methods }; newQuorumMemberConfigForChain(members excludeapi-keyanddevice),QuorumRecoveryConfig,RecoveryConfigForChain, andisQuorumRecoveryguard, all exported.thresholddefaults to 1.methodsentry runs through the existing per-type prep (passkey credential creation, server-signer address derivation) and the quorum is forwarded on theconfig.recoverywire property (member array mapped to the wire namesigners). A 1-elementmethodscollapses to a plain single admin; emptymethodsand invalid thresholds (non-integer,< 1,> methods.length) are rejected before any API call.?? 1on both sides) and the member set order-insensitively with consume-once matching — server members via derived address (primary + legacy), emails Gmail-normalized, passkeys by id then name,compareSignerConfigsas the per-pair field backstop. Quorum-vs-single mismatches in either direction throw.initDefaultSignerskips a quorum recovery — previouslygetSignerDescriptor("quorum")would throw after the wallet was created on-chain. The approval loop is untouched (separate ticket; still throwsQuorumSignerNotSupportedError).initializeWebViewIfNeeded) to keep compilation; runtime quorum handling (WebView init, email population) is audited in its own ticket.Everything is additive: single-signer create payloads are byte-for-byte unchanged (covered by a regression assertion).
Notes for review
type: "quorum"discriminant (the EDD surface is the bare{ threshold, methods }); if preferred, we can split into an external type withouttypeand an internal one with it.config.adminSigneryet, so the validation arm reads it through a runtime-tolerant cast per EDD §5.1.Testing
wallet-factory.test.ts: wire shape (prepared server/passkey members, threshold omission), single-method collapse, empty-methods and threshold rejections, and existing-wallet validation (out-of-order match,undefined≡1threshold, threshold/member/count mismatches, quorum↔single mismatches, Gmail-dot normalization).@crossmint/wallets-sdksuite: 673 passed.tsc --noEmitclean; wallets-sdk, react-base, react-ui, and react-native all build including DTS.🤖 Generated with Claude Code