feat: refactor Solana shadow signer to use P256 instead of Ed25519 external wallet#1568
Conversation
…ternal wallet Co-Authored-By: Guille <[email protected]>
Co-Authored-By: Guille <[email protected]>
Original prompt from Guille |
🤖 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: 4c4a5cc 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private async createWebAuthnSignature(challenge: string): Promise<{ signature: string }> { | ||
| const challengeHex = challenge.replace("0x", ""); | ||
| const challengeBase64 = Buffer.from(challengeHex, "hex").toString("base64"); | ||
| const challengeBase64url = challengeBase64.replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, ""); |
There was a problem hiding this comment.
Browser Buffer runtime break
SolanaP256KeypairSigner calls Buffer.from(...) to build base64/base64url challenges and to hex-decode signatureMessage (solana-p256-keypair.ts:35,58,61-62). In many browser bundlers Buffer is no longer polyfilled by default, so Solana device signing will throw at runtime (ReferenceError: Buffer is not defined). Since this signer is now used for Solana shadow signers (browser/device path), this is a merge blocker unless the package guarantees a Buffer polyfill in all supported browser targets.
Also appears in packages/wallets/src/signers/p256-keypair.ts:38-39,70,74-75.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/signers/solana-p256-keypair.ts
Line: 33:36
Comment:
**Browser `Buffer` runtime break**
`SolanaP256KeypairSigner` calls `Buffer.from(...)` to build base64/base64url challenges and to hex-decode `signatureMessage` (`solana-p256-keypair.ts:35,58,61-62`). In many browser bundlers `Buffer` is no longer polyfilled by default, so Solana device signing will throw at runtime (`ReferenceError: Buffer is not defined`). Since this signer is now used for Solana shadow signers (browser/device path), this is a merge blocker unless the package guarantees a Buffer polyfill in all supported browser targets.
Also appears in `packages/wallets/src/signers/p256-keypair.ts:38-39,70,74-75`.
How can I resolve this? If you propose a fix, please make it concise.| const originBytes = new TextEncoder().encode(this.STUB_ORIGIN); | ||
| const rpIdHash = sha256(toHex(originBytes)); |
There was a problem hiding this comment.
Incorrect rpIdHash input
rpIdHash is computed as sha256(toHex(originBytes)) where originBytes encodes the full origin string "https://crossmint.com" (solana-p256-keypair.ts:45-46). WebAuthn’s rpIdHash is defined as SHA-256 over the RP ID (effective domain), not the full origin including scheme. If the Squads backend expects the domain-only hash (or any other rpId material), signatures produced here will never verify.
This PR needs to ensure the exact rpIdHash input matches what the verifying backend uses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/signers/solana-p256-keypair.ts
Line: 45:46
Comment:
**Incorrect rpIdHash input**
`rpIdHash` is computed as `sha256(toHex(originBytes))` where `originBytes` encodes the full origin string `"https://crossmint.com"` (`solana-p256-keypair.ts:45-46`). WebAuthn’s `rpIdHash` is defined as SHA-256 over the RP ID (effective domain), not the full origin including scheme. If the Squads backend expects the domain-only hash (or any other rpId material), signatures produced here will never verify.
This PR needs to ensure the exact rpIdHash input matches what the verifying backend uses.
How can I resolve this? If you propose a fix, please make it concise.| const rHex = Buffer.from(signatureBytes.slice(0, 32)).toString("hex").padStart(64, "0"); | ||
| const sHex = Buffer.from(signatureBytes.slice(32, 64)).toString("hex").padStart(64, "0"); | ||
|
|
||
| return { signature: rHex + sHex }; |
There was a problem hiding this comment.
Signature hex prefix mismatch
This signer returns { signature: rHex + sHex } without a 0x prefix (solana-p256-keypair.ts:64), while the existing P256KeypairSigner returns "0x" + rHex + sHex (p256-keypair.ts:77). If any downstream code treats p256-keypair signatures as 0x-prefixed hex (common for typed Hex / RPC encoders), Solana signatures will be rejected or mis-parsed. This needs to be consistent (or the type/consumers need to explicitly support both formats).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/signers/solana-p256-keypair.ts
Line: 61:64
Comment:
**Signature hex prefix mismatch**
This signer returns `{ signature: rHex + sHex }` without a `0x` prefix (`solana-p256-keypair.ts:64`), while the existing `P256KeypairSigner` returns `"0x" + rHex + sHex` (`p256-keypair.ts:77`). If any downstream code treats `p256-keypair` signatures as `0x`-prefixed hex (common for typed `Hex` / RPC encoders), Solana signatures will be rejected or mis-parsed. This needs to be consistent (or the type/consumers need to explicitly support both formats).
How can I resolve this? If you propose a fix, please make it concise.…saction approvals Co-Authored-By: Guille <[email protected]>
Additional Comments (2)
In This same issue exists in signature approvals too ( Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 591:596
Comment:
**Wrong approval signer locator**
In `approveTransactionInternal`, each approval is built with `signer: this.signer.locator()` (wallet’s primary signer) rather than the locator of the signer that actually produced the signature from `signedApprovals`. If `options.additionalSigners` is used (or if the pending approval expects a different signer), this will send a mismatched signer locator/signature pair and the backend will reject the approval.
This same issue exists in signature approvals too (`approveSignatureInternal`). Use the locator from the signer that signed (e.g., carry it alongside the signature and set `signer: <that locator>`).
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 531:536
Comment:
**Wrong approval signer locator**
`approveSignatureInternal` maps `signedApprovals` into approvals but hardcodes `signer: this.signer.locator()`. If any pending approval is signed by an `additionalSigner`, the backend will receive the wallet signer locator paired with a signature from a different key and reject it. This needs to use the locator of the signer that produced each `signature`.
How can I resolve this? If you propose a fix, please make it concise. |
…for P256 approval signing Co-Authored-By: Guille <[email protected]>
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 570:579
Comment:
**Approvals signed with wrong locator**
In `pendingApprovals.map(...)`, you correctly pick the matching `signer` for each `pendingApproval`, but when building `approvals` you always set `signer: this.signer.locator()` (the wallet’s primary signer). This will misattribute signatures when `additionalSigners` are used (and will also be wrong for device signers), causing backend verification to fail. The `signer` field should correspond to the signer that produced each `signature` (likely the `signer.locator()` found for that pending approval).
How can I resolve this? If you propose a fix, please make it concise. |
- Add deviceChallenge and deviceRegister methods to API client - Update wallet factory to call device-challenge/register during shadow signer creation for Solana - SDK generates P256 keypair, calls API to get challenge, signs it, submits signature - API handles all Squads passkey communication server-side Co-Authored-By: Guille <[email protected]>
|
Slithering in with some cold-blooded takes, I see 🐍 Re: the three comments on
Re: the "wrong approval signer locator" comments — this is pre-existing behavior in the base branch, not introduced by this PR. The approval locator logic was already using |
| private async registerSolanaDeviceSigner( | ||
| publicKeyBase64: string, | ||
| shadowSignerStorage?: import("../signers/shadow-signer").ShadowSignerStorage | ||
| ): Promise<void> { | ||
| const { signatureMessage } = await this.apiClient.deviceChallenge({ | ||
| p256PublicKey: publicKeyBase64, | ||
| }); | ||
|
|
||
| const storage = shadowSignerStorage ?? getStorage(); | ||
| const signatureMessageBytes = new Uint8Array(Buffer.from(signatureMessage, "hex")); | ||
| const rawSignature = await storage.sign(publicKeyBase64, signatureMessageBytes); |
There was a problem hiding this comment.
Unconditional device registration call
registerSolanaDeviceSigner() calls deviceChallenge/deviceRegister but never checks the API response for an error shape before using signatureMessage (wallet-factory.ts:570-586). Other client calls in this file typically guard with if ("error" in response) and throw; here a backend error would likely surface later as Buffer.from(undefined, ...) or an invalid signature, making failures hard to diagnose and potentially masking real registration failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet-factory.ts
Line: 566:576
Comment:
**Unconditional device registration call**
`registerSolanaDeviceSigner()` calls `deviceChallenge`/`deviceRegister` but never checks the API response for an `error` shape before using `signatureMessage` (`wallet-factory.ts:570-586`). Other client calls in this file typically guard with `if ("error" in response)` and throw; here a backend error would likely surface later as `Buffer.from(undefined, ...)` or an invalid signature, making failures hard to diagnose and potentially masking real registration failures.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Also appears in Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/signers/shadow-signer/utils.ts
Line: 41:46
Comment:
**Browser build breaks (Buffer)**
`generateShadowSigner()` uses `Buffer.from(publicKeyBase64, "base64")` to decode the public key (`utils.ts:44`). This code runs in the browser/device shadow-signer path, and many browser builds don’t provide a `Buffer` global by default, causing a runtime `ReferenceError: Buffer is not defined` when enabling shadow signers.
Also appears in `packages/wallets/src/signers/shadow-signer/shadow-signer-storage-browser.ts:36,55` and in the newly-added `packages/wallets/src/wallets/wallet-factory.ts:575,578-579` and `packages/wallets/src/wallets/wallet.ts:578-579` (same browser surface).
How can I resolve this? If you propose a fix, please make it concise. |
Co-Authored-By: Guille <[email protected]>
Co-Authored-By: Guille <[email protected]>
Co-Authored-By: Guille <[email protected]>
|
Server-side WebAuthn decoration for transaction approvals Changes:
Corresponding API changes in PR #23269 (crossbit-main). Link to Devin run: https://crossmint.devinenterprise.com/sessions/86ac9ccd01214c538be78325a1852625 |
| signer instanceof SolanaP256KeypairSigner | ||
| ) { | ||
| return signer.signRawMessage(pendingApproval.signatureMessage as string); |
There was a problem hiding this comment.
instanceof SolanaP256KeypairSigner check will never match for device signers. Device signers are wrapped in SolanaNonCustodialSigner/SolanaExternalWalletSigner which contain a SolanaShadowSigner, which in turn wraps the SolanaP256KeypairSigner. The outer signer is what appears here, so this condition will never be true and signRawMessage will never be called when needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 582:584
Comment:
`instanceof SolanaP256KeypairSigner` check will never match for device signers. Device signers are wrapped in `SolanaNonCustodialSigner`/`SolanaExternalWalletSigner` which contain a `SolanaShadowSigner`, which in turn wraps the `SolanaP256KeypairSigner`. The outer signer is what appears here, so this condition will never be true and `signRawMessage` will never be called when needed.
How can I resolve this? If you propose a fix, please make it concise.| const { signatureMessage } = await this.apiClient.deviceChallenge({ | ||
| p256PublicKey: publicKeyBase64, | ||
| }); |
There was a problem hiding this comment.
Missing error handling for API responses. Both deviceChallenge and deviceRegister API calls (lines 570, 581) should check for error responses before using the data. Other API calls in this file use if ("error" in response) guards. Without this, backend errors would surface as cryptic Buffer.from(undefined, ...) failures instead of clear registration errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet-factory.ts
Line: 570:572
Comment:
Missing error handling for API responses. Both `deviceChallenge` and `deviceRegister` API calls (lines 570, 581) should check for error responses before using the data. Other API calls in this file use `if ("error" in response)` guards. Without this, backend errors would surface as cryptic `Buffer.from(undefined, ...)` failures instead of clear registration errors.
How can I resolve this? If you propose a fix, please make it concise.| const signatureMessageBytes = new Uint8Array(Buffer.from(signatureMessageHex, "hex")); | ||
| const signatureBytes = await this.onSignTransaction(this.address(), signatureMessageBytes); | ||
| const rHex = Buffer.from(signatureBytes.slice(0, 32)).toString("hex").padStart(64, "0"); | ||
| const sHex = Buffer.from(signatureBytes.slice(32, 64)).toString("hex").padStart(64, "0"); | ||
| return { signature: rHex + sHex }; |
There was a problem hiding this comment.
Duplicate logic with p256-keypair.ts:70-77. The signRawMessage implementation is nearly identical between Solana and EVM P256 signers (only difference: 0x prefix on line 77 in EVM version). Consider extracting shared logic to reduce duplication, as mentioned in the PR description.
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 With AI
This is a comment left during a code review.
Path: packages/wallets/src/signers/solana-p256-keypair.ts
Line: 34:38
Comment:
Duplicate logic with `p256-keypair.ts:70-77`. The `signRawMessage` implementation is nearly identical between Solana and EVM P256 signers (only difference: `0x` prefix on line 77 in EVM version). Consider extracting shared logic to reduce duplication, as mentioned in the PR description.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| private async createWebAuthnSignature(challenge: string): Promise<{ signature: string }> { | ||
| const challengeHex = challenge.replace("0x", ""); | ||
| const challengeBase64 = Buffer.from(challengeHex, "hex").toString("base64"); | ||
| const challengeBase64url = challengeBase64.replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, ""); | ||
|
|
||
| const clientDataJSON = JSON.stringify({ | ||
| type: "webauthn.get", | ||
| challenge: challengeBase64url, | ||
| origin: this.STUB_ORIGIN, | ||
| crossOrigin: false, | ||
| }); | ||
|
|
||
| const originBytes = new TextEncoder().encode(this.STUB_ORIGIN); | ||
| const rpIdHash = sha256(toHex(originBytes)); | ||
|
|
||
| const flags = "05"; | ||
| const signCount = "00000000"; | ||
|
|
||
| const authenticatorData = (rpIdHash + flags + signCount) as `0x${string}`; | ||
|
|
||
| const clientDataJSONBytes = new TextEncoder().encode(clientDataJSON); | ||
| const clientDataHash = sha256(toHex(clientDataJSONBytes)); | ||
|
|
||
| const signatureMessage = (authenticatorData + clientDataHash.slice(2)) as `0x${string}`; | ||
|
|
||
| const signatureMessageBytes = new Uint8Array(Buffer.from(signatureMessage.slice(2), "hex")); | ||
| const signatureBytes = await this.onSignTransaction(this.address(), signatureMessageBytes); | ||
|
|
||
| const rHex = Buffer.from(signatureBytes.slice(0, 32)).toString("hex").padStart(64, "0"); | ||
| const sHex = Buffer.from(signatureBytes.slice(32, 64)).toString("hex").padStart(64, "0"); | ||
|
|
||
| return { signature: rHex + sHex }; | ||
| } |
There was a problem hiding this comment.
Duplicate WebAuthn signature construction with p256-keypair.ts:33-78. Nearly 40 lines duplicated between Solana and EVM versions. Only differences: line 54 uses sha256 vs keccak256 on line 51 in EVM, and line 72 omits 0x prefix vs line 77 in EVM. Extract shared construction logic into a helper function parameterized by hash function and prefix option.
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 With AI
This is a comment left during a code review.
Path: packages/wallets/src/signers/solana-p256-keypair.ts
Line: 41:73
Comment:
Duplicate WebAuthn signature construction with `p256-keypair.ts:33-78`. Nearly 40 lines duplicated between Solana and EVM versions. Only differences: line 54 uses `sha256` vs `keccak256` on line 51 in EVM, and line 72 omits `0x` prefix vs line 77 in EVM. Extract shared construction logic into a helper function parameterized by hash function and prefix option.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Context Used: Rule from 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 With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet-factory.ts
Line: 589:596
Comment:
Use enum constants instead of string literals for chain comparisons. Replace `"solana"`, `"stellar"`, `"evm"` with appropriate enum constants for consistency and type safety.
**Context Used:** Rule from `dashboard` - Use enum constants (e.g., Chain.SOLANA, Chain.BASE) instead of string literals when comparing chain ... ([source](https://app.greptile.com/review/custom-context?memory=0e60096d-0843-4800-801b-f8a78b766fbc))
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
…sting WebAuthn flow Co-Authored-By: Guille <[email protected]>
Co-Authored-By: Guille <[email protected]>
1 participant
Description
Refactors the Solana shadow signer to use P256 (secp256r1) key generation and WebAuthn-style signing instead of the previous Ed25519 external wallet signer approach. This mirrors the pattern already established for EVM shadow signers, enabling Squads smart account support for Solana device signers.
Key changes:
SolanaP256KeypairSigner: Similar to the EVMP256KeypairSignerbut usessha256(notkeccak256) for the rpIdHash to match the Squads backend, and returns signatures without0xprefixSolanaShadowSigner: Now wrapsSolanaP256KeypairSignerinstead ofSolanaExternalWalletSignerImportant things to review
sha256vskeccak256for rpIdHash insolana-p256-keypair.ts— must match the Squads backend (createHash("sha256")in crossbit-main). EVM useskeccak256for ZeroDev; Solana usessha256for Squads.rHex + sHex(no0xprefix) vs EVM's"0x" + rHex + sHex. Verify this matches what the backend'sencodeP256Signatureexpects.p256-keypair.tsandsolana-p256-keypair.ts— may be worth extracting shared logic in a follow-up.SolanaP256KeypairSignersigning logic (existing tests pass, but new WebAuthn construction is untested).Companion backend PR: https://github.com/Paella-Labs/crossbit-main/pull/23269
Test plan
Package updates
@crossmint/wallets-sdk: minor (changeset added)Link to Devin run: https://crossmint.devinenterprise.com/sessions/db8b1ee4e8c242fab13995ee9d7820a6
Requested by: @guilleasz-crossmint