feat(wallet): add multi-device passkey signer management flow#192
Merged
KevinMB0220 merged 4 commits intoGalaxy-KJ:mainfrom Mar 28, 2026
Merged
feat(wallet): add multi-device passkey signer management flow#192KevinMB0220 merged 4 commits intoGalaxy-KJ:mainfrom
KevinMB0220 merged 4 commits intoGalaxy-KJ:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughRefactors SmartWalletService signer flows: adds robust Base64 helpers, changes AddSigner API to admin-signer inputs while preserving legacy session-signer flow via a new addSessionSigner, implements WebAuthn-based auth-entry simulation/signing for add/remove operations, and extends RemoveSigner params. Changes
Sequence DiagramssequenceDiagram
participant Caller
participant SmartWalletService
participant Soroban as Soroban Contract
participant WebAuthn
Caller->>SmartWalletService: addSigner(signerCredentialId, signerPublicKey, authCredentialId?)
SmartWalletService->>Soroban: simulate add_signer(credential_id, public_key)
Soroban-->>SmartWalletService: auth-entry (includes hash)
SmartWalletService->>SmartWalletService: derive WebAuthn challenge from hash
SmartWalletService->>WebAuthn: get assertion (provided or navigator.credentials.get(authCredentialId))
WebAuthn-->>SmartWalletService: PublicKeyCredential assertion
SmartWalletService->>SmartWalletService: build AccountSignature::WebAuthn and attach to auth-entry
SmartWalletService->>Soroban: submit signed auth-entry / assemble signed transaction XDR
SmartWalletService-->>Caller: return signed XDR
sequenceDiagram
participant Caller
participant SmartWalletService
participant Soroban as Soroban Contract
participant WebAuthn
Caller->>SmartWalletService: addSessionSigner(sessionPublicKey, ttlSeconds, credentialId)
SmartWalletService->>Soroban: simulate add_session_signer(credential_id, session_public_key, ttl_ledgers)
Soroban-->>SmartWalletService: auth-entry (includes hash)
SmartWalletService->>SmartWalletService: derive WebAuthn challenge
SmartWalletService->>WebAuthn: get assertion (provided or navigator.credentials.get(credentialId))
WebAuthn-->>SmartWalletService: PublicKeyCredential assertion
SmartWalletService->>SmartWalletService: build AccountSignature::WebAuthn and attach
SmartWalletService->>Soroban: submit signed auth-entry / assemble signed XDR
SmartWalletService-->>Caller: return signed XDR
sequenceDiagram
participant Caller
participant SmartWalletService
participant Soroban as Soroban Contract
participant WebAuthn
Caller->>SmartWalletService: removeSigner(signerCredentialId | credentialId | signerPublicKey, authCredentialId?)
SmartWalletService->>Soroban: simulate remove_signer(credential_id_or_key)
Soroban-->>SmartWalletService: auth-entry (includes hash)
SmartWalletService->>SmartWalletService: derive WebAuthn challenge
SmartWalletService->>WebAuthn: get assertion (provided or navigator.credentials.get(authCredentialId ?? credentialId))
WebAuthn-->>SmartWalletService: PublicKeyCredential assertion
SmartWalletService->>SmartWalletService: build AccountSignature::WebAuthn and attach
SmartWalletService->>Soroban: submit signed auth-entry / assemble signed XDR
SmartWalletService-->>Caller: return signed XDR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/core/wallet/src/smart-wallet.service.ts (3)
318-327: Consider extracting duplicated auth-entry hash computation.The auth-entry SHA-256 hash computation is duplicated verbatim in
addSigner,addSessionSigner,removeSigner, andsign. A shared helper would reduce maintenance burden.♻️ Suggested helper extraction
async function computeAuthEntryChallenge( authEntry: xdr.SorobanAuthorizationEntry ): Promise<string> { const authEntryBytes = authEntry.toXDR(); const authEntryArrayBuffer = authEntryBytes.buffer.slice( authEntryBytes.byteOffset, authEntryBytes.byteOffset + authEntryBytes.byteLength ) as ArrayBuffer; const authEntryHash = new Uint8Array( await crypto.subtle.digest("SHA-256", authEntryArrayBuffer) ); return toBase64Url(authEntryHash); }Usage in each method:
- const authEntryBytes = authEntry.toXDR(); - const authEntryArrayBuffer = authEntryBytes.buffer.slice( - authEntryBytes.byteOffset, - authEntryBytes.byteOffset + authEntryBytes.byteLength - ) as ArrayBuffer; - const authEntryHash = new Uint8Array( - await crypto.subtle.digest("SHA-256", authEntryArrayBuffer) - ); - const challenge = toBase64Url(authEntryHash); + const challenge = await computeAuthEntryChallenge(authEntry);Also applies to: 432-440, 527-536
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 318 - 327, Extract the duplicated auth-entry SHA-256 + Base64URL logic into a single async helper (e.g., computeAuthEntryChallenge) and replace the repeated blocks in addSigner, addSessionSigner, removeSigner, and sign with calls to that helper; the helper should accept an xdr.SorobanAuthorizationEntry, call toXDR(), slice the ArrayBuffer from .buffer/.byteOffset/.byteLength, run crypto.subtle.digest("SHA-256") and return toBase64Url of the resulting Uint8Array so all four methods reuse the same implementation.
329-348: Consider extracting duplicated WebAuthn assertion retrieval logic.The WebAuthn
navigator.credentials.get()call with identical options structure is repeated in three methods. A shared helper would consolidate this pattern.♻️ Suggested helper extraction
private async getWebAuthnAssertion( challenge: string, credentialId: string, existingAssertion?: PublicKeyCredential | null ): Promise<PublicKeyCredential> { if (existingAssertion) return existingAssertion; const assertion = (await navigator.credentials.get({ publicKey: { challenge: Buffer.from(base64UrlToUint8Array(challenge)), rpId: this.webAuthnProvider.rpId, // assumes getter is added allowCredentials: [ { type: "public-key" as const, id: Buffer.from(base64UrlToUint8Array(credentialId)), }, ], userVerification: "required", timeout: 60_000, }, })) as PublicKeyCredential | null; if (!assertion) { throw new Error("WebAuthn authentication was cancelled or timed out."); } return assertion; }Also applies to: 442-458, 538-554
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 329 - 348, Extract the duplicated navigator.credentials.get(...) WebAuthn retrieval into a private helper (e.g., getWebAuthnAssertion) that accepts (challenge: string, credentialId: string, existingAssertion?: PublicKeyCredential | null) and returns a Promise<PublicKeyCredential>; inside, if existingAssertion is provided return it, otherwise call navigator.credentials.get with the same publicKey options (challenge converted via base64UrlToUint8Array, rpId from this.webAuthnProvider, allowCredentials with the credentialId buffer, userVerification "required", timeout 60_000), throw the same "WebAuthn authentication was cancelled or timed out." error when null, and replace the duplicated blocks in addSigner and the other two methods (the blocks that reference webAuthnAssertion, challenge, authCredentialId) with calls to this.getWebAuthnAssertion(...).
208-214: Consider making the interface type-safer with a union or required field.Both
signerCredentialIdandsignerPublicKeyare optional, but the implementation requires at least one. TypeScript won't catch callers who provide neither at compile time.♻️ Suggested type-safe alternative
-export interface RemoveSignerParams { - walletAddress: string; - signerCredentialId?: string; - signerPublicKey?: string; - authCredentialId?: string; - webAuthnAssertion?: PublicKeyCredential; -} +interface RemoveSignerParamsBase { + walletAddress: string; + authCredentialId?: string; + webAuthnAssertion?: PublicKeyCredential; +} + +export type RemoveSignerParams = RemoveSignerParamsBase & ( + | { signerCredentialId: string; signerPublicKey?: string } + | { signerCredentialId?: undefined; signerPublicKey: string } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 208 - 214, The RemoveSignerParams interface allows both signerCredentialId and signerPublicKey to be omitted but the implementation requires at least one; change the type to a union so callers must provide either a signerCredentialId or a signerPublicKey (e.g., a union of the common fields with one variant requiring signerCredentialId and the other requiring signerPublicKey, optionally marking the opposite field as never) while keeping walletAddress, authCredentialId and webAuthnAssertion as common properties; update any usages of RemoveSignerParams accordingly so TypeScript enforces the “at least one” requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 248-267: The legacy-path detection (isLegacySessionPath) is too
permissive and can route partial legacy inputs to addSessionSigner with empty
defaults, causing confusing validation errors; change the condition to require
all legacy fields (sessionPublicKey, ttlSeconds, credentialId) be present before
treating it as the legacy session path, and only then call addSessionSigner with
the provided values; otherwise fall through to the non-legacy path (admin
addSigner) so callers that only provided e.g. credentialId aren’t misrouted;
update the logic surrounding isLegacySessionPath and the try/catch that rewrites
addSessionSigner to addSigner accordingly so errors remain accurate.
- Around line 464-472: The code currently falls back to an empty string for
resolvedCredentialId and passes an empty Uint8Array to
buildWebAuthnSignatureScVal, which can produce an invalid contract input; update
smart-wallet.service.ts to validate that a credential ID exists (check
credentialId and assertion.id via the assertionResponse/context used there) and
throw a clear error or reject/return early if none is found before calling
buildWebAuthnSignatureScVal (i.e., replace the resolvedCredentialId = ""
fallback with an explicit failure), so buildWebAuthnSignatureScVal always
receives a valid credential ID (reference resolvedCredentialId,
assertionResponse, and buildWebAuthnSignatureScVal).
- Around line 331-334: SmartWalletService is accessing a private field via
(this.webAuthnProvider as any).rpId in multiple places; fix by exposing rpId
properly and using typed access: either add a public getter rpId() to the
WebAuthNProvider class and return the internal rpId, or change the
SmartWalletService.webAuthnProvider property/constructor param to use the
existing IWebAuthnProvider interface which already exposes rpId; then replace
all casts with direct access this.webAuthnProvider.rpId in the
SmartWalletService methods (the occurrences around the navigator.credentials.get
call and the other three sites where rpId is read). Ensure imports and
constructor types are updated so TypeScript enforces the correct interface/class
type.
- Around line 490-494: The code currently uses a fallback
resolvedSignerCredentialId = signerCredentialId ?? signerPublicKey which
conflates two distinct values; remove that fallback and make RemoveSignerParams
require both signerCredentialId and signerPublicKey; in removeSigner, validate
that walletAddress, signerCredentialId, and signerPublicKey are present (throw
clear errors referencing removeSigner) and pass only signerCredentialId to the
contract call (do not use signerPublicKey in place of the credential ID). Ensure
type definitions for RemoveSignerParams reflect both fields as required so
callers must supply them.
---
Nitpick comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 318-327: Extract the duplicated auth-entry SHA-256 + Base64URL
logic into a single async helper (e.g., computeAuthEntryChallenge) and replace
the repeated blocks in addSigner, addSessionSigner, removeSigner, and sign with
calls to that helper; the helper should accept an xdr.SorobanAuthorizationEntry,
call toXDR(), slice the ArrayBuffer from .buffer/.byteOffset/.byteLength, run
crypto.subtle.digest("SHA-256") and return toBase64Url of the resulting
Uint8Array so all four methods reuse the same implementation.
- Around line 329-348: Extract the duplicated navigator.credentials.get(...)
WebAuthn retrieval into a private helper (e.g., getWebAuthnAssertion) that
accepts (challenge: string, credentialId: string, existingAssertion?:
PublicKeyCredential | null) and returns a Promise<PublicKeyCredential>; inside,
if existingAssertion is provided return it, otherwise call
navigator.credentials.get with the same publicKey options (challenge converted
via base64UrlToUint8Array, rpId from this.webAuthnProvider, allowCredentials
with the credentialId buffer, userVerification "required", timeout 60_000),
throw the same "WebAuthn authentication was cancelled or timed out." error when
null, and replace the duplicated blocks in addSigner and the other two methods
(the blocks that reference webAuthnAssertion, challenge, authCredentialId) with
calls to this.getWebAuthnAssertion(...).
- Around line 208-214: The RemoveSignerParams interface allows both
signerCredentialId and signerPublicKey to be omitted but the implementation
requires at least one; change the type to a union so callers must provide either
a signerCredentialId or a signerPublicKey (e.g., a union of the common fields
with one variant requiring signerCredentialId and the other requiring
signerPublicKey, optionally marking the opposite field as never) while keeping
walletAddress, authCredentialId and webAuthnAssertion as common properties;
update any usages of RemoveSignerParams accordingly so TypeScript enforces the
“at least one” requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87958ec7-05df-4d8c-a09f-a5b9d3d6058b
📒 Files selected for processing (1)
packages/core/wallet/src/smart-wallet.service.ts
Comment on lines
+248
to
+267
| const isLegacySessionPath = | ||
| params.sessionPublicKey !== undefined || | ||
| params.ttlSeconds !== undefined || | ||
| params.credentialId !== undefined; | ||
| if (isLegacySessionPath) { | ||
| try { | ||
| return await this.addSessionSigner({ | ||
| walletAddress: params.walletAddress, | ||
| sessionPublicKey: params.sessionPublicKey ?? "", | ||
| ttlSeconds: params.ttlSeconds ?? 0, | ||
| credentialId: params.credentialId, | ||
| webAuthnAssertion: params.webAuthnAssertion, | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| throw new Error(error.message.replace(/addSessionSigner/g, "addSigner")); | ||
| } | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete legacy field detection may cause confusing errors.
If a caller passes only one or two of the legacy fields (e.g., { walletAddress, credentialId: "xyz" } without sessionPublicKey), the routing logic triggers but then passes "" and 0 as defaults, which immediately fail validation in addSessionSigner. The user sees an error about missing sessionPublicKey even though they may have intended an admin-signer call.
🛡️ Suggested fix: validate all required legacy fields together
const isLegacySessionPath =
params.sessionPublicKey !== undefined ||
params.ttlSeconds !== undefined ||
params.credentialId !== undefined;
if (isLegacySessionPath) {
+ // Ensure all required legacy fields are present when any legacy field is used
+ if (!params.sessionPublicKey || params.ttlSeconds === undefined) {
+ throw new Error(
+ "addSigner: when using legacy session-signer fields, both sessionPublicKey and ttlSeconds are required"
+ );
+ }
try {
return await this.addSessionSigner({
walletAddress: params.walletAddress,
- sessionPublicKey: params.sessionPublicKey ?? "",
- ttlSeconds: params.ttlSeconds ?? 0,
+ sessionPublicKey: params.sessionPublicKey,
+ ttlSeconds: params.ttlSeconds,
credentialId: params.credentialId,
webAuthnAssertion: params.webAuthnAssertion,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isLegacySessionPath = | |
| params.sessionPublicKey !== undefined || | |
| params.ttlSeconds !== undefined || | |
| params.credentialId !== undefined; | |
| if (isLegacySessionPath) { | |
| try { | |
| return await this.addSessionSigner({ | |
| walletAddress: params.walletAddress, | |
| sessionPublicKey: params.sessionPublicKey ?? "", | |
| ttlSeconds: params.ttlSeconds ?? 0, | |
| credentialId: params.credentialId, | |
| webAuthnAssertion: params.webAuthnAssertion, | |
| }); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new Error(error.message.replace(/addSessionSigner/g, "addSigner")); | |
| } | |
| throw error; | |
| } | |
| } | |
| const isLegacySessionPath = | |
| params.sessionPublicKey !== undefined || | |
| params.ttlSeconds !== undefined || | |
| params.credentialId !== undefined; | |
| if (isLegacySessionPath) { | |
| // Ensure all required legacy fields are present when any legacy field is used | |
| if (!params.sessionPublicKey || params.ttlSeconds === undefined) { | |
| throw new Error( | |
| "addSigner: when using legacy session-signer fields, both sessionPublicKey and ttlSeconds are required" | |
| ); | |
| } | |
| try { | |
| return await this.addSessionSigner({ | |
| walletAddress: params.walletAddress, | |
| sessionPublicKey: params.sessionPublicKey, | |
| ttlSeconds: params.ttlSeconds, | |
| credentialId: params.credentialId, | |
| webAuthnAssertion: params.webAuthnAssertion, | |
| }); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new Error(error.message.replace(/addSessionSigner/g, "addSigner")); | |
| } | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 248 - 267, The
legacy-path detection (isLegacySessionPath) is too permissive and can route
partial legacy inputs to addSessionSigner with empty defaults, causing confusing
validation errors; change the condition to require all legacy fields
(sessionPublicKey, ttlSeconds, credentialId) be present before treating it as
the legacy session path, and only then call addSessionSigner with the provided
values; otherwise fall through to the non-legacy path (admin addSigner) so
callers that only provided e.g. credentialId aren’t misrouted; update the logic
surrounding isLegacySessionPath and the try/catch that rewrites addSessionSigner
to addSigner accordingly so errors remain accurate.
Contributor
|
@Jayrodri088 please fix the conflicts |
Jayrodri088
force-pushed
the
feat-multi-device-passkey
branch
from
7a4abd1 to
84c9723
Compare
…skey' of https://github.com/Jayrodri088/Galaxy-DevKit into feat-multi-device-passkey
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/wallet/src/smart-wallet.service.ts (1)
612-625:⚠️ Potential issue | 🟡 MinorEmpty credential ID passed to contract when
webAuthnAssertionis provided withoutcredentialId.When
webAuthnAssertionis provided butcredentialIdis undefined,credentialBytesbecomesBuffer.alloc(0)(line 615), and the contract'sremove_signeris called with an empty credential ID. The contract will reject this withSignerNotFound, but the caller won't understand why.Either extract the credential ID from
webAuthnAssertion.idor requirecredentialIdexplicitly.🛡️ Suggested fix
async removeSigner(params: RemoveSignerParams): Promise<string> { - const { walletAddress, credentialId, webAuthnAssertion } = params; + const { walletAddress, webAuthnAssertion } = params; + const credentialId = params.credentialId ?? (webAuthnAssertion?.id); if (!walletAddress) { throw new Error("removeSigner: walletAddress is required"); } - if (!webAuthnAssertion && !credentialId) { + if (!credentialId) { throw new Error( - "removeSigner: either webAuthnAssertion or credentialId must be provided" + "removeSigner: credentialId is required (can be derived from webAuthnAssertion.id)" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 612 - 625, The code constructs credentialBytes as empty when credentialId is missing, which causes remove_signer to be called with an empty credential and yields a confusing SignerNotFound error; update the logic in smart-wallet.service.ts around the credentialBytes and webAuthnAssertion handling so that if credentialId is falsy but webAuthnAssertion is provided you extract the credential id from webAuthnAssertion.id (decode via base64UrlToUint8Array) and populate credentialBytes accordingly, and if neither is available throw a clear error before building the TransactionBuilder/Contract.call("remove_signer") request; ensure you validate the resulting credentialBytes is non-empty and reference the variables/operations credentialBytes, webAuthnAssertion, Contract.call("remove_signer"), and TransactionBuilder when making these changes.
♻️ Duplicate comments (2)
packages/core/wallet/src/smart-wallet.service.ts (2)
540-576:⚠️ Potential issue | 🟡 MinorEmpty string fallback for credential ID could produce invalid contract input.
If both
credentialIdandwebAuthnAssertion.idare undefined,resolvedCredentialIdbecomes"", and lines 572-574 pass an emptyUint8Array(0)to the signature builder. The contract will reject this, but the error won't clearly indicate the root cause.🛡️ Suggested fix: fail early if credential ID cannot be resolved
if (webAuthnAssertion) { assertionResponse = getAssertionResponse(webAuthnAssertion); - resolvedCredentialId = credentialId ?? webAuthnAssertion.id ?? ""; + resolvedCredentialId = credentialId ?? webAuthnAssertion.id; + if (!resolvedCredentialId) { + throw new Error("addSessionSigner: unable to resolve credential id from input or assertion"); + } } else { // ... existing else branch ... resolvedCredentialId = credentialId!; } const signerSignature = buildWebAuthnSignatureScVal( new Uint8Array(assertionResponse.authenticatorData), new Uint8Array(assertionResponse.clientDataJSON), - resolvedCredentialId - ? base64UrlToUint8Array(resolvedCredentialId) - : new Uint8Array(0), + base64UrlToUint8Array(resolvedCredentialId), convertSignatureDERtoCompact(assertionResponse.signature) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 540 - 576, The code can end up passing an empty credential ID into buildWebAuthnSignatureScVal (resolvedCredentialId = ""), which yields an unclear contract rejection; update the branch where webAuthnAssertion is present to validate and require a credential ID (check credentialId and webAuthnAssertion.id) and throw a clear, descriptive error if neither exists instead of assigning "". Specifically modify the logic around resolvedCredentialId (set in the webAuthnAssertion branch) so it either uses a validated credentialId/webAuthnAssertion.id or throws, and keep using getAssertionResponse, buildWebAuthnSignatureScVal, and convertSignatureDERtoCompact as before; ensure the error message explicitly states that the credential ID is missing.
344-363:⚠️ Potential issue | 🟡 MinorIncomplete legacy field detection may cause confusing errors.
When a caller provides only a subset of legacy fields (e.g.,
{ walletAddress, credentialId: "xyz" }withoutsessionPublicKey),isLegacySessionPathtriggers but routes with default valuessessionPublicKey: ""andttlSeconds: 0. These immediately fail validation inaddSessionSigner, producing an error about missingsessionPublicKeywhen the caller may have intended an admin-signer call.🛡️ Suggested fix: validate all required legacy fields together
const isLegacySessionPath = - params.sessionPublicKey !== undefined || - params.ttlSeconds !== undefined || - params.credentialId !== undefined; + params.sessionPublicKey !== undefined && + params.ttlSeconds !== undefined; if (isLegacySessionPath) { try { return await this.addSessionSigner({ walletAddress: params.walletAddress, - sessionPublicKey: params.sessionPublicKey ?? "", - ttlSeconds: params.ttlSeconds ?? 0, + sessionPublicKey: params.sessionPublicKey, + ttlSeconds: params.ttlSeconds, credentialId: params.credentialId, webAuthnAssertion: params.webAuthnAssertion, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 344 - 363, isLegacySessionPath currently triggers when any legacy field is present, causing code to call addSessionSigner with defaulted values and misleading validation errors; change the detection so it only takes the "legacy session" branch when all required legacy fields are provided (e.g., ensure params.sessionPublicKey !== undefined && params.ttlSeconds !== undefined && params.credentialId !== undefined), so addSessionSigner is only called with real values (leave the try/catch and error-name-replace logic intact).
🧹 Nitpick comments (1)
packages/core/wallet/src/smart-wallet.service.ts (1)
427-440: Type cast bypasses type safety to accessrpId.Line 430 uses
(this.webAuthnProvider as any).rpIdwhile elsewhere (e.g., line 547, 658) the code accessesthis.webAuthnProvider.relyingPartyIddirectly. This inconsistency suggestsrpIdmay be a private property whilerelyingPartyIdis the public accessor.Use the public
relyingPartyIdproperty consistently to avoid breaking if the internal structure changes.♻️ Suggested fix
assertion = (await navigator.credentials.get({ publicKey: { challenge: Buffer.from(base64UrlToUint8Array(challenge)), - rpId: (this.webAuthnProvider as any).rpId, + rpId: this.webAuthnProvider.relyingPartyId, allowCredentials: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 427 - 440, Replace the unsafe cast accessing (this.webAuthnProvider as any).rpId with the public accessor this.webAuthnProvider.relyingPartyId inside the navigator.credentials.get publicKey options; update the rpId field in the PublicKeyCredentialRequestOptions to use relyingPartyId so you consistently use the public API of webAuthnProvider (ensure the value is a string as expected by the PublicKeyCredential options).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 284-291: Remove the unused fields signerCredentialId,
signerPublicKey, and authCredentialId from the RemoveSignerParams interface so
it only contains walletAddress, credentialId, and webAuthnAssertion (the exact
fields destructured/used by removeSigner). Update the interface definition named
RemoveSignerParams in smart-wallet.service.ts to drop those three properties and
ensure callers and the removeSigner method continue to use walletAddress,
credentialId, and webAuthnAssertion; leave AddSignerParams untouched since it is
the correct home for those fields.
---
Outside diff comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 612-625: The code constructs credentialBytes as empty when
credentialId is missing, which causes remove_signer to be called with an empty
credential and yields a confusing SignerNotFound error; update the logic in
smart-wallet.service.ts around the credentialBytes and webAuthnAssertion
handling so that if credentialId is falsy but webAuthnAssertion is provided you
extract the credential id from webAuthnAssertion.id (decode via
base64UrlToUint8Array) and populate credentialBytes accordingly, and if neither
is available throw a clear error before building the
TransactionBuilder/Contract.call("remove_signer") request; ensure you validate
the resulting credentialBytes is non-empty and reference the
variables/operations credentialBytes, webAuthnAssertion,
Contract.call("remove_signer"), and TransactionBuilder when making these
changes.
---
Duplicate comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 540-576: The code can end up passing an empty credential ID into
buildWebAuthnSignatureScVal (resolvedCredentialId = ""), which yields an unclear
contract rejection; update the branch where webAuthnAssertion is present to
validate and require a credential ID (check credentialId and
webAuthnAssertion.id) and throw a clear, descriptive error if neither exists
instead of assigning "". Specifically modify the logic around
resolvedCredentialId (set in the webAuthnAssertion branch) so it either uses a
validated credentialId/webAuthnAssertion.id or throws, and keep using
getAssertionResponse, buildWebAuthnSignatureScVal, and
convertSignatureDERtoCompact as before; ensure the error message explicitly
states that the credential ID is missing.
- Around line 344-363: isLegacySessionPath currently triggers when any legacy
field is present, causing code to call addSessionSigner with defaulted values
and misleading validation errors; change the detection so it only takes the
"legacy session" branch when all required legacy fields are provided (e.g.,
ensure params.sessionPublicKey !== undefined && params.ttlSeconds !== undefined
&& params.credentialId !== undefined), so addSessionSigner is only called with
real values (leave the try/catch and error-name-replace logic intact).
---
Nitpick comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 427-440: Replace the unsafe cast accessing (this.webAuthnProvider
as any).rpId with the public accessor this.webAuthnProvider.relyingPartyId
inside the navigator.credentials.get publicKey options; update the rpId field in
the PublicKeyCredentialRequestOptions to use relyingPartyId so you consistently
use the public API of webAuthnProvider (ensure the value is a string as expected
by the PublicKeyCredential options).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e98ea839-1792-4e4a-8005-23a1dd004b1c
📒 Files selected for processing (1)
packages/core/wallet/src/smart-wallet.service.ts
Comment on lines
284
to
291
| export interface RemoveSignerParams { | ||
| walletAddress: string; | ||
| signerCredentialId?: string; | ||
| signerPublicKey?: string; | ||
| authCredentialId?: string; | ||
| credentialId?: string; | ||
| webAuthnAssertion?: PublicKeyCredential; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of signerCredentialId or authCredentialId with removeSigner
rg -n "removeSigner.*signerCredentialId|removeSigner.*authCredentialId|signerCredentialId.*removeSigner" --type ts -C3Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 49
🏁 Script executed:
cat -n packages/core/wallet/src/smart-wallet.service.ts | sed -n '280,295p'Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 640
🏁 Script executed:
cat -n packages/core/wallet/src/smart-wallet.service.ts | sed -n '590,620p'Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 1375
🏁 Script executed:
rg "removeSigner\(" packages/core/wallet/src/ --type ts -A 5 -B 1Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 1261
🏁 Script executed:
rg "signerCredentialId|authCredentialId" packages/core/wallet/src/ --type ts -B 2 -A 2Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 4076
🏁 Script executed:
cat -n packages/core/wallet/src/smart-wallet.service.ts | sed -n '593,700p'Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 4306
🏁 Script executed:
rg "removeSigner\(" packages/core/wallet/src/ --type ts -C 10Repository: Galaxy-KJ/Galaxy-DevKit
Length of output: 2552
Remove unused fields signerCredentialId, signerPublicKey, and authCredentialId from RemoveSignerParams interface.
The RemoveSignerParams interface (lines 284-291) declares signerCredentialId, signerPublicKey, and authCredentialId, but the removeSigner() method (line 593-694) only destructures and uses walletAddress, credentialId, and webAuthnAssertion. These unused fields create confusion for API callers who might assume they are valid parameters for removal operations.
These fields belong in AddSignerParams (which already uses them), not in RemoveSignerParams.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 284 - 291,
Remove the unused fields signerCredentialId, signerPublicKey, and
authCredentialId from the RemoveSignerParams interface so it only contains
walletAddress, credentialId, and webAuthnAssertion (the exact fields
destructured/used by removeSigner). Update the interface definition named
RemoveSignerParams in smart-wallet.service.ts to drop those three properties and
ensure callers and the removeSigner method continue to use walletAddress,
credentialId, and webAuthnAssertion; leave AddSignerParams untouched since it is
the correct home for those fields.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/wallet/src/smart-wallet.service.ts (1)
338-339:⚠️ Potential issue | 🟡 Minor
addSignerdocblock still describes TTL/session-expiry behavior.At Line 338, the comment about TTL-based auto-expiry belongs to session signer flow, not admin
add_signer. This is now misleading API documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 338 - 339, The docblock for addSigner (the admin add_signer flow) incorrectly mentions TTL-based auto-expiry; update the comment on the addSigner/add_signer method to remove the TTL/session-expiry sentence and replace it with a clear statement that admin-added signers are persistent until explicitly revoked (e.g., via remove_signer) and do not auto-expire, and if there is a separate session signer API mention that session TTL behavior belongs there instead.
♻️ Duplicate comments (3)
packages/core/wallet/src/smart-wallet.service.ts (3)
427-440:⚠️ Potential issue | 🟠 MajorUse
credentialBackend+ typedrelyingPartyIdinstead of direct navigator +as any.This path bypasses the injected backend abstraction and uses
(this.webAuthnProvider as any).rpId(Line 430), which is brittle and inconsistent with the rest of this service.Proposed fix
- if (!assertion) { - assertion = (await navigator.credentials.get({ - publicKey: { - challenge: Buffer.from(base64UrlToUint8Array(challenge)), - rpId: (this.webAuthnProvider as any).rpId, - allowCredentials: [ - { - type: "public-key" as const, - id: Buffer.from(base64UrlToUint8Array(authCredentialId!)), - }, - ], - userVerification: "required", - timeout: 60_000, - }, - })) as PublicKeyCredential | null; - } + if (!assertion) { + assertion = await this.credentialBackend.get({ + publicKey: { + challenge: Buffer.from(base64UrlToUint8Array(challenge)), + rpId: this.webAuthnProvider.relyingPartyId, + allowCredentials: [ + { + type: "public-key" as const, + id: Buffer.from(base64UrlToUint8Array(authCredentialId!)), + }, + ], + userVerification: "required", + timeout: 60_000, + }, + }); + }#!/bin/bash # Verify inconsistent WebAuthn acquisition and unsafe rpId access patterns rg -n "navigator\\.credentials\\.get|\\(this\\.webAuthnProvider as any\\)\\.rpId|credentialBackend\\.get\\(" packages/core/wallet/src/smart-wallet.service.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 427 - 440, Replace the direct navigator.credentials.get(...) call and the unsafe (this.webAuthnProvider as any).rpId access with the injected backend and a typed relying-party id: call this.credentialBackend.get(/* publicKey options */) instead of navigator.credentials.get, pass the same publicKey options (challenge, allowCredentials, userVerification, timeout) but obtain the relying-party id via a typed property on the provider (e.g. (this.webAuthnProvider as WebAuthnProvider).relyingPartyId or this.webAuthnProvider.relyingPartyId) rather than casting to any, and keep the assertion typed as PublicKeyCredential | null; update the function that performs the assertion (the code around the variable assertion in smart-wallet.service.ts) to use credentialBackend.get and remove the as any usage.
542-575:⚠️ Potential issue | 🟠 MajorCredential ID resolution can still produce empty signature
idbytes.Using
credentialId ?? webAuthnAssertion.id ?? ""means an empty-stringcredentialIdwins overwebAuthnAssertion.id, and Lines 572-574 then emitUint8Array(0)for signatureid.Proposed fix
- resolvedCredentialId = credentialId ?? webAuthnAssertion.id ?? ""; + resolvedCredentialId = credentialId || webAuthnAssertion.id; + if (!resolvedCredentialId) { + throw new Error( + "addSessionSigner: unable to resolve credential id from input or assertion" + ); + } @@ - resolvedCredentialId - ? base64UrlToUint8Array(resolvedCredentialId) - : new Uint8Array(0), + base64UrlToUint8Array(resolvedCredentialId),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 542 - 575, The code uses `credentialId ?? webAuthnAssertion.id ?? ""` which treats an empty string as a valid credential id, causing `resolvedCredentialId` to be empty and producing zero-length id bytes; change the resolution to prefer a non-empty `credentialId` (e.g. `credentialId` if it has length, otherwise `webAuthnAssertion.id`, otherwise fallback) so `resolvedCredentialId` is only empty when truly absent, and ensure the call site in buildWebAuthnSignatureScVal (the conversion `base64UrlToUint8Array(resolvedCredentialId)`) is only invoked for a non-empty id (otherwise pass a zero-length Uint8Array) — update the logic around `resolvedCredentialId`, the assignment from `credentialId`/`webAuthnAssertion.id`, and the conditional before calling `base64UrlToUint8Array` so empty strings do not win.
344-363:⚠️ Potential issue | 🟡 MinorLegacy-path detection is still too permissive and can misroute admin calls.
At Line 344, treating
credentialIdalone as a legacy trigger can route non-legacyaddSignercalls intoaddSessionSigner, then fail with unrelated session-field errors.Proposed fix
- const isLegacySessionPath = - params.sessionPublicKey !== undefined || - params.ttlSeconds !== undefined || - params.credentialId !== undefined; + const isLegacySessionPath = + params.sessionPublicKey !== undefined || + params.ttlSeconds !== undefined; if (isLegacySessionPath) { + if (!params.sessionPublicKey || params.ttlSeconds === undefined) { + throw new Error( + "addSigner: when using legacy session-signer fields, both sessionPublicKey and ttlSeconds are required" + ); + } try { return await this.addSessionSigner({ walletAddress: params.walletAddress, - sessionPublicKey: params.sessionPublicKey ?? "", - ttlSeconds: params.ttlSeconds ?? 0, + sessionPublicKey: params.sessionPublicKey, + ttlSeconds: params.ttlSeconds, credentialId: params.credentialId, webAuthnAssertion: params.webAuthnAssertion, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 344 - 363, The legacy-path detector is too permissive: change the isLegacySessionPath logic in smart-wallet.service.ts so it does NOT treat credentialId alone as a legacy trigger — only consider sessionPublicKey, ttlSeconds, or webAuthnAssertion as indicating the legacy session flow; remove credentialId from that boolean check. Locate the isLegacySessionPath variable and the conditional that calls addSessionSigner, update the boolean expression to exclude credentialId (e.g., sessionPublicKey !== undefined || ttlSeconds !== undefined || webAuthnAssertion !== undefined), and keep the existing try/catch that rewrites addSessionSigner -> addSigner in the error message. Ensure addSigner code path still receives credentialId when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 338-339: The docblock for addSigner (the admin add_signer flow)
incorrectly mentions TTL-based auto-expiry; update the comment on the
addSigner/add_signer method to remove the TTL/session-expiry sentence and
replace it with a clear statement that admin-added signers are persistent until
explicitly revoked (e.g., via remove_signer) and do not auto-expire, and if
there is a separate session signer API mention that session TTL behavior belongs
there instead.
---
Duplicate comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 427-440: Replace the direct navigator.credentials.get(...) call
and the unsafe (this.webAuthnProvider as any).rpId access with the injected
backend and a typed relying-party id: call this.credentialBackend.get(/*
publicKey options */) instead of navigator.credentials.get, pass the same
publicKey options (challenge, allowCredentials, userVerification, timeout) but
obtain the relying-party id via a typed property on the provider (e.g.
(this.webAuthnProvider as WebAuthnProvider).relyingPartyId or
this.webAuthnProvider.relyingPartyId) rather than casting to any, and keep the
assertion typed as PublicKeyCredential | null; update the function that performs
the assertion (the code around the variable assertion in
smart-wallet.service.ts) to use credentialBackend.get and remove the as any
usage.
- Around line 542-575: The code uses `credentialId ?? webAuthnAssertion.id ??
""` which treats an empty string as a valid credential id, causing
`resolvedCredentialId` to be empty and producing zero-length id bytes; change
the resolution to prefer a non-empty `credentialId` (e.g. `credentialId` if it
has length, otherwise `webAuthnAssertion.id`, otherwise fallback) so
`resolvedCredentialId` is only empty when truly absent, and ensure the call site
in buildWebAuthnSignatureScVal (the conversion
`base64UrlToUint8Array(resolvedCredentialId)`) is only invoked for a non-empty
id (otherwise pass a zero-length Uint8Array) — update the logic around
`resolvedCredentialId`, the assignment from
`credentialId`/`webAuthnAssertion.id`, and the conditional before calling
`base64UrlToUint8Array` so empty strings do not win.
- Around line 344-363: The legacy-path detector is too permissive: change the
isLegacySessionPath logic in smart-wallet.service.ts so it does NOT treat
credentialId alone as a legacy trigger — only consider sessionPublicKey,
ttlSeconds, or webAuthnAssertion as indicating the legacy session flow; remove
credentialId from that boolean check. Locate the isLegacySessionPath variable
and the conditional that calls addSessionSigner, update the boolean expression
to exclude credentialId (e.g., sessionPublicKey !== undefined || ttlSeconds !==
undefined || webAuthnAssertion !== undefined), and keep the existing try/catch
that rewrites addSessionSigner -> addSigner in the error message. Ensure
addSigner code path still receives credentialId when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30ab2c3d-f6c2-4458-8389-c4646c9ed2f7
📒 Files selected for processing (7)
jest.config.jspackages/core/defi-protocols/tsconfig.tsbuildinfopackages/core/oracles/tsconfig.tsbuildinfopackages/core/stellar-sdk/tsconfig.tsbuildinfopackages/core/wallet/auth/src/hardware/__tests__/LedgerWallet.test.tspackages/core/wallet/auth/src/hardware/types.tspackages/core/wallet/src/smart-wallet.service.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/wallet/auth/src/hardware/types.ts
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
2 participants
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.
Title
Add multi-device passkey signer management to
SmartWalletServiceContext
This PR addresses the smart-wallet multi-device passkey feature by enabling users to register and manage multiple authorized passkey signers on the same wallet account. The contract already supports admin signer management (
add_signer/remove_signer); this PR brings the wallet service in line with that contract capability.Problem
SmartWalletService.addSignerwas previously wired toadd_session_signersemantics (session Ed25519 key + TTL), which did not support adding additional passkey/admin signers for multi-device redundancy. This blocked management UI and API flows intended for passkey signer expansion and lifecycle management.Solution
Refactor signer-management flows in
packages/core/wallet/src/smart-wallet.service.tsto separate admin signer and session signer responsibilities while preserving compatibility:addSignernow supports admin/passkey signer registration via contractadd_signer.addSessionSignerfor session-key registration viaadd_session_signer.removeSignerfor on-chain signer removal viaremove_signer.addSignercalls to route toaddSessionSigner.Detailed Changes
1) Admin signer registration (
addSigner)walletAddresssignerCredentialId(new signer)signerPublicKey(base64 65-byte uncompressed P-256 key)authCredentialIdorwebAuthnAssertionfor existing signer authorizationadd_signer(credential_id, public_key)AccountSignature::WebAuthn2) Session signer registration (
addSessionSigner)add_session_signer(credential_id, session_public_key, ttl_ledgers)3) Signer removal (
removeSigner)remove_signer(credential_id)invocationsignerPublicKeyfield.4) Encoding robustness
base64UrlToUint8Arraynow handles missing padding safely.base64ToUint8Arrayhelper for standard base64 decoding (e.g., passkey public key input).Backward Compatibility
addSignerremain functional:sessionPublicKey,ttlSeconds,credentialId)addSessionSigneraddSignernaming for compatibility with existing tests/callers.Acceptance Criteria Mapping
addSignernow directly registers additional admin passkey signers.addSigner(admin path) andremoveSignerflows in service layer.Testing
Executed:
npm test -- packages/core/wallet/src/tests/smart-wallet.service.test.tsResult:
Risk Assessment
addSignerpayload fields; mitigated by keeping legacy session path supported.Closes: #164
Summary by CodeRabbit
New Features
Improvements
Tests
Chores