wallets: fix server signer recovery check in useSigner#1784
wallets: fix server signer recovery check in useSigner#1784devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…ners
The API-sourced recovery config has shape {type: 'server', address: '...'} without
a secret field. Calling deriveServerSignerDetails on it caused TypeError because
stripAndValidateSecret received undefined.
Now checks for address field first (API path) and falls back to secret derivation
(createWallet path).
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 9edf8fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1347-1373
Comment:
**Missing unit test for fixed code path**
The PR description explicitly notes that no test was added for `isRecoverySigner` when `#recovery` is API-sourced (`{ type: "server", address: "..." }` with no `secret`). Without a test, a future refactor could silently regress this exact crash scenario. Consider adding a test that mocks `getWallet` returning a server recovery config and asserts that `useSigner({ type: "server", secret: "..." })` no longer throws.
**Rule Used:** Add unit tests when implementing new validation lo... ([source](https://app.greptile.com/review/custom-context?memory=9dc35ad5-5868-49a3-bbcb-a42edc5ee697))
**Learnt From**
[Paella-Labs/crossbit-main#21014](https://github.com/Paella-Labs/crossbit-main/pull/21014)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1358-1360
Comment:
**Type definition does not reflect API runtime shape**
`ServerSignerConfig` is typed as `{ type: "server"; secret: string }` with no `address` field, but the API-sourced recovery object has `{ type: "server", address: "..." }`. The `"address" in recovery` runtime guard works, but `recovery.address` is only accessible because TypeScript's `in`-narrowing widens the type; the declared type never acknowledges this field. This creates a gap where tools like IDE autocomplete, exhaustiveness checks, or future type-level guards may behave unexpectedly.
Consider introducing a discriminated union such as:
```ts
export type ServerSignerConfig =
| { type: "server"; secret: string } // user-provided (createWallet)
| { type: "server"; address: string }; // API-sourced (getWallet)
```
or adding `address?: string` to the existing type, so the runtime shape is reflected statically.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: use recovery address directly in is..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ("address" in recovery && typeof recovery.address === "string") { | ||
| return inputDerived === recovery.address; | ||
| } |
There was a problem hiding this comment.
ServerSignerConfig is typed as { type: "server"; secret: string } with no address field, but the API-sourced recovery object has { type: "server", address: "..." }. The "address" in recovery runtime guard works, but recovery.address is only accessible because TypeScript's in-narrowing widens the type; the declared type never acknowledges this field. This creates a gap where tools like IDE autocomplete, exhaustiveness checks, or future type-level guards may behave unexpectedly.
Consider introducing a discriminated union such as:
export type ServerSignerConfig =
| { type: "server"; secret: string } // user-provided (createWallet)
| { type: "server"; address: string }; // API-sourced (getWallet)or adding address?: string to the existing type, so the runtime shape is reflected statically.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 1358-1360
Comment:
**Type definition does not reflect API runtime shape**
`ServerSignerConfig` is typed as `{ type: "server"; secret: string }` with no `address` field, but the API-sourced recovery object has `{ type: "server", address: "..." }`. The `"address" in recovery` runtime guard works, but `recovery.address` is only accessible because TypeScript's `in`-narrowing widens the type; the declared type never acknowledges this field. This creates a gap where tools like IDE autocomplete, exhaustiveness checks, or future type-level guards may behave unexpectedly.
Consider introducing a discriminated union such as:
```ts
export type ServerSignerConfig =
| { type: "server"; secret: string } // user-provided (createWallet)
| { type: "server"; address: string }; // API-sourced (getWallet)
```
or adding `address?: string` to the existing type, so the runtime shape is reflected statically.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Agreed that the type definition doesn't reflect the runtime shape. However, updating ServerSignerConfig to a discriminated union would be a broader change that touches many consumers across the codebase (wallet-factory, signer locator utils, etc.) and is better suited as a separate follow-up PR.
The "address" in recovery runtime guard is safe and idiomatic TypeScript narrowing for this bugfix. Keeping the scope minimal here to unblock the customer.
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Description
Fixes a
TypeError: Cannot read properties of undefined (reading 'startsWith')thrown when callingwallet.useSigner({ type: "server", secret: "xmsk1_..." })on a wallet fetched viagetWallet.Root cause:
isRecoverySigner()unconditionally calledderiveServerSignerDetails(recovery, ...)on the API-sourced recovery config, which has shape{type: "server", address: "..."}— nosecretfield. This passedundefinedintostripAndValidateSecret(), which called.startsWith()on it.Fix: The method now checks which fields the recovery config actually has at runtime:
getWallet): recovery hasaddress→ compare directlycreateWallet): recovery hassecret→ derive address then comparefalseNote: the existing code comment on L1345 already described this intent ("so we use the address directly instead of re-deriving it") but the implementation didn't match — this PR fixes that.
ServerSignerConfigtype is{ type: "server"; secret: string }and does not includeaddress. The API recovery config has a different runtime shape. This fix handles it with"address" in recoveryruntime checks rather than updating the type definitions — worth considering whether the types should also be corrected.isRecoverySignerwith API-sourced server recovery config).Test plan
Package updates
@crossmint/wallets-sdk: patch — changeset addedLink to Devin session: https://crossmint.devinenterprise.com/sessions/726ed7808b1f4757a10ca2165051f3dd
Requested by: @xmint-guille