Skip to content

wallets: fall back to recovery signer when multiple delegated signers exist#1752

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1774880222-fix-multi-signer-fallback-recovery
Open

wallets: fall back to recovery signer when multiple delegated signers exist#1752
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1774880222-fix-multi-signer-fallback-recovery

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

Description

Fixes a bug where wallets with more than one delegated signer become completely unusable for signing operations (approve, send, etc.).

After the wallets-v1 merge (#1721), initDefaultSigner was intentionally leaving #signer undefined when >1 delegated signers existed, expecting the consumer to call wallet.useSigner(). However, client-side consumers like lobster.cash use useWallet()wallet.approve() directly and never call useSigner(). This causes the error:

"No signer is set. This wallet has multiple signers configured. Call wallet.useSigner() to select which signer to use before signing operations."

This surfaces in lobster.cash when a user tries to set up a second agent — the first agent adds a delegated signer, and the second adds another, putting the wallet into the >1 signers state.

The fix: In the >1 signers case, fall back to the recovery signer (same behavior as the 0-signers case). The recovery signer represents the wallet owner and is the safest default. useSigner() still works as an explicit override.

Key things for review

  • Is recovery signer always the correct default for multi-signer wallets? This changes behavior for all SDK consumers, not just lobster.cash. Previously they'd get an error; now they silently get the recovery signer.
  • No new test was added for the >1 signers → recovery fallback path. Existing tests pass but may not cover this specific branch.
  • Could this mask a bug where a consumer should be explicitly choosing a delegated signer but now silently gets recovery instead?

Test plan

  • Existing wallet.test.ts suite passes (50 tests)
  • All @crossmint/wallets-sdk tests pass (258 passed, 68 skipped)
  • pnpm lint passes cleanly
  • Manual trace: confirmed the error path in requireSigner() (line 1102) is only reachable when #signer is null AND >1 initial signers — this fix ensures #signer is set via recovery in that case

Package updates

  • @crossmint/wallets-sdk — patch changeset added via .changeset/fix-multi-signer-recovery-fallback.md

Link to Devin session: https://crossmint.devinenterprise.com/sessions/262eeb98fa3f4f1ebae9ba85728721da
Requested by: @aigustin

…signers

Co-Authored-By: Agus <agustin@paella.dev>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

Original prompt from Agus

SYSTEM:
=== BEGIN THREAD HISTORY (in #lockdown-lobster-cash-development) ===
manu (U094JFP8BTM): When I try to setup a new agent:

ATTACHMENT:"https://crossmint.devinenterprise.com/attachments/ac6874e2-a68b-46e8-8c08-d7139db83119/image.png"

manu (U094JFP8BTM): I've checked and the delegated signer is added correctly. Could it be due to changes in the wallet SDK?

Agus (U078SGS1ZFV): Hmm what call throws this error @manu (U094JFP8BTM) ?

Agus (U078SGS1ZFV): @Devin can you debug why this error would happen?
=== END THREAD HISTORY ===

Thread URL: https://crossmint.slack.com/archives/C0AJ23LAPEW/p1774869840909219?thread_ts=1774869840.909219&amp;cid=C0AJ23LAPEW

The latest message is the one right above that tagged you.

@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 30, 2026

🦋 Changeset detected

Latest commit: c3927e4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@crossmint/wallets-sdk Patch
@crossmint/wallets-quickstart-devkit Patch
expo-demo Patch
@crossmint/client-sdk-react-base Patch
@crossmint/client-sdk-react-native-ui Patch
@crossmint/client-sdk-react-ui Patch
@crossmint/auth-ssr-nextjs-demo Patch
@crossmint/client-sdk-nextjs-starter Patch

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

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 30, 2026

Important Files Changed

Filename Overview
packages/wallets/src/wallets/wallet.ts Adds recovery signer fallback for >1 delegated signers case in initDefaultSigner. Logic is correct, mirrors the 0-signers case, and includes appropriate logging. Minor duplication with the existing 0-signers branch.
.changeset/fix-multi-signer-recovery-fallback.md Patch changeset with clear description of the fix. No issues.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 171-188

Comment:
**Duplicated recovery signer logic**

The 0-signers case (lines 172-174) and the >1-signers case (lines 186-187) now perform identical logic: build the recovery signer config and assemble it. Consider consolidating them to reduce duplication:

```suggestion
            if (this.#initialSigners.length === 1) {
                // Exactly 1 auto-assemblable signer → use it
                const internalConfig = this.buildInternalSignerConfig(this.#initialSigners[0]);
                this.#signer = await this.assembleFullSigner(internalConfig);
            } else {
                // 0 or >1 signers → fall back to recovery signer so the wallet remains usable.
                // Users who need a specific delegated signer can call useSigner() to override.
                if (this.#initialSigners.length > 1) {
                    walletsLogger.info("wallet.initDefaultSigner.multipleSignersFallbackToRecovery", {
                        signerCount: this.#initialSigners.length,
                        recoveryType: this.#recovery.type,
                    });
                }
                const internalConfig = this.buildInternalSignerConfig(this.#recovery);
                this.#signer = await this.assembleFullSigner(internalConfig);
            }
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: fall back to recovery signer when w..." | Re-trigger Greptile

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.

1 participant