Refactor web3 auth#108
Conversation
- Add loginWithEVMWallet to auth-helpers for mock wallet flow - First link-email test uses wallet login (fixtures-wallet-mock) - Second test keeps magic link for 'already linked' when user has email - Add link-email.spec.ts to auth project, remove from testIgnore - Update e2e docs
|
Deployment failed with the following error: Learn More: https://vercel.com/gaboesquivel?upgradeToPro=build-rate-limit |
WalkthroughThe PR consolidates Web3 authentication verification into a centralized helper, refactors token/session issuance to a reusable module, updates frontend UI to use config-driven wallet rendering, extracts shared signing utilities, and activates previously skipped E2E wallet tests while introducing link-email verification tests. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant Fastify as Fastify Backend
participant DB as Database
participant Crypto as Crypto Provider
Client->>Fastify: POST /verify (chain, message, signature, domain)
Fastify->>Fastify: parseMessage(message)
alt Parse Failed
Fastify-->>Client: 400 INVALID_MESSAGE
end
Fastify->>Fastify: validateAddress(address)
alt Address Invalid
Fastify-->>Client: 400 INVALID_ADDRESS
end
Fastify->>DB: Delete nonce (chain, address, nonce)
alt Nonce Not Found
Fastify-->>Client: 401 INVALID_NONCE
end
Fastify->>Fastify: Check nonce expiry
alt Nonce Expired
Fastify-->>Client: 401 EXPIRED_NONCE
end
Fastify->>Crypto: verifySignature(message, signature, address)
alt Signature Invalid
Fastify-->>Client: 401 INVALID_SIGNATURE
end
Fastify->>DB: Lookup wallet by (chain, address)
Fastify->>DB: Lookup user by wallet
alt No User or Wallet
Fastify->>DB: Create user (transaction)
Fastify->>DB: Create wallet link
else User exists, no wallet
Fastify->>DB: Link wallet to user
else User and wallet exist
Fastify->>DB: Update wallet lastUsedAt
end
Fastify->>Fastify: Create session (UUID)
Fastify->>Fastify: Sign access & refresh tokens
Fastify->>DB: Store session
Fastify-->>Client: 200 { accessToken, refreshToken }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/fastify/src/plugins/error-handler.ts (1)
72-75:⚠️ Potential issue | 🟠 MajorReplace
request.routerPathwithrequest.routeOptions?.url— removed in Fastify v5
request.routerPathwas fully removed in Fastify v5 and the guard'routerPath' in requestalways evaluates tofalsein Fastify 5.7.4. This causes the code to always fall through to the raw URL, exposing path parameters in Sentry.Security impact: Web3 auth routes like
/auth/web3/eip155/nonce?address=0xABC123...leak wallet addresses into theroutetag (line 107) and rawurlfields sent to Sentry, violating the project's sensitive data logging guidelines.Observability impact: Every unique wallet address produces a distinct Sentry fingerprint instead of grouping under the parameterized template (e.g.,
/auth/web3/eip155/:address).Fix
Replace lines 72–75 with:
const routePath: string = request.routeOptions?.url ?? request.url.split('?')[0] ?? '/'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/plugins/error-handler.ts` around lines 72 - 75, The code uses request.routerPath (guard `'routerPath' in request`) which is removed in Fastify v5 causing routePath to always use the raw URL; update the routePath assignment to use request.routeOptions?.url first and fall back to the URL split and '/' so path params are preserved in Sentry. Locate the routePath variable assignment and replace the routerPath check with using request.routeOptions?.url as the primary source, then fallback to request.url.split('?')[0] ?? '/' so routes like /auth/web3/eip155/:address are captured instead of leaking raw parameter values.packages/react/src/hooks/use-verify-web3-auth.ts (1)
33-43:⚠️ Potential issue | 🟠 MajorExtract
.datafrom the SDK response or specifyresponseStyle: 'data'to fix the type mismatch.The openapi-fetch client returns responses with a
'fields'style by default (not'data'style), which wraps the actual data:{ data: { token, refreshToken }, request, response }. The castas unknown as Web3Eip155VerifyResponse | Web3SolanaVerifyResponseexpects a bare{ token, refreshToken }shape, so accessingresult.tokenon line 50 will beundefinedat runtime.Fix by either:
- Extracting
.data:const result = verifyResult.data- Passing
responseStyle: 'data'in both verify calls to unwrap automatically🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-verify-web3-auth.ts` around lines 33 - 43, The SDK verify call returns a wrapper with .data, so change the handling of verifyResult in use-verify-web3-auth: after calling client.auth.web3.eip155.verify(...) or client.auth.web3.solana.verify(...), extract the payload with verifyResult.data (or alternatively pass responseStyle: 'data' into both verify calls) and assign that to result so the subsequent code reads token/refreshToken from the unwrapped Web3Eip155VerifyResponse | Web3SolanaVerifyResponse object; update references to verifyResult -> verifyResult.data (or add responseStyle: 'data' to both client.auth.web3.eip155.verify and client.auth.web3.solana.verify calls).
🧹 Nitpick comments (16)
apps/fastify/src/plugins/error-handler.ts (1)
45-67:redactBodyonly performs a shallow redact — nested sensitive fields are not scrubbed.The spread
{ ...(body as Record<string, unknown>) }is a single level deep. Any nested object containingpassword,token,accessToken, etc. (e.g.{ user: { password: "…" } }) bypasses redaction entirely before being sent to Sentry. While current web3 auth payloads are flat, the handler is generic and will process all routes including future ones.Additionally, if
bodyis an array,typeof [] === 'object'passes the guard and{ ...arrayBody }produces{ '0': …, '1': … }— structurally incorrect in Sentry context.♻️ Suggested recursive redaction
function redactBody(body: unknown): unknown { if (!body || typeof body !== 'object') return body + if (Array.isArray(body)) return body.map(redactBody) + const redacted = { ...(body as Record<string, unknown>) } const sensitiveFields = [ 'password', 'token', 'secret', 'apiKey', 'accessToken', 'refreshToken', 'idToken', 'authorization', ] for (const field of sensitiveFields) { if (field in redacted) { redacted[field] = '[REDACTED]' + } else if (redacted[field] && typeof redacted[field] === 'object') { + redacted[field] = redactBody(redacted[field]) } } + // Recursively redact all nested objects + for (const key of Object.keys(redacted)) { + if (!sensitiveFields.includes(key) && redacted[key] && typeof redacted[key] === 'object') { + redacted[key] = redactBody(redacted[key]) + } + } return redacted }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/plugins/error-handler.ts` around lines 45 - 67, The redactBody function performs only a shallow copy and fails to redact nested sensitive fields and mishandles arrays; update redactBody to recursively traverse objects and arrays, preserving the original structure (arrays remain arrays, objects remain objects), and replace values for any key matching entries in the sensitiveFields list (e.g., 'password','token','accessToken', etc.) with '[REDACTED]'; ensure you don't mutate the input by creating new objects/arrays during recursion, handle non-plain values by returning them unchanged, and reference the existing redactBody and sensitiveFields identifiers when implementing the recursive sanitizer.apps/next/e2e/fixtures.ts (1)
14-34: Naming inconsistency:authenticatedStorageStatestill usesrunWithwhileauthenticatedPagenow usesuse.Both parameters name the same Playwright fixture callback. The PR updated only
authenticatedPage;authenticatedStorageState(line 17, line 25) was left behind. Consider renaming for consistency:♻️ Proposed fix
authenticatedStorageState: [ async ( { browser }: { browser: import('@playwright/test').Browser }, - runWith: (path: string) => Promise<void>, + use: (path: string) => Promise<void>, ) => { ... - await runWith(authFile) + await use(authFile) }, { scope: 'worker' }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/e2e/fixtures.ts` around lines 14 - 34, The fixture authenticatedStorageState is inconsistent with authenticatedPage: it uses the old Playwright fixture callback name runWith instead of use; change the parameter name in the async function signature from runWith to use and replace the call runWith(authFile) with use(authFile) so both fixtures use the same Playwright callback identifier (referencing authenticatedStorageState, authenticatedPage, runWith, use).apps/next/lib/auth-client.ts (1)
24-31: Optional: replace the IIFE with a flat try/catch for clarity.The immediately-invoked function expression is correct but adds indirection. A plain block achieves the same result and is easier to follow:
♻️ Proposed simplification
- const errorDetail = - (() => { - try { - return (JSON.parse(bodyText) as { message?: string })?.message ?? bodyText - } catch { - return bodyText - } - })() || `HTTP ${response.status}` + let errorDetail: string + try { + errorDetail = (JSON.parse(bodyText) as { message?: string })?.message ?? bodyText + } catch { + errorDetail = bodyText + } + errorDetail ||= `HTTP ${response.status}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/auth-client.ts` around lines 24 - 31, The IIFE used to compute errorDetail is unnecessary indirection; replace it with a simple try/catch block that assigns errorDetail from parsing bodyText (falling back to bodyText) and then default to `HTTP ${response.status}` if falsy. Locate the errorDetail declaration in auth-client.ts where bodyText and response are available, remove the immediately-invoked function, and implement a flat try { parsed = JSON.parse(bodyText); errorDetail = parsed?.message ?? bodyText } catch { errorDetail = bodyText } followed by the final fallback to `HTTP ${response.status}`.apps/next/hooks/use-web3-nonce.ts (1)
30-30:as unknown asdouble cast removes all type safety on the API response — prefer a typed extraction.The double cast means TypeScript can no longer catch a mismatch between the actual API response type from
@repo/coreandWeb3NonceResponse. If the generated client type already exposesnoncedirectly, the cast isn't needed. If it doesn't, an explicit destructuring is safer:♻️ Proposed fix
- return res as unknown as Web3NonceResponse + const { nonce } = res as { nonce: string } + return { nonce }If the generated client already types the return as
{ nonce: string }after the API contract change, dropping the cast entirely is the cleanest option. Otherwise, at minimum use a singleas { nonce: string }rather than going throughunknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/hooks/use-web3-nonce.ts` at line 30, Replace the unsafe double-cast on the API response in the useWeb3Nonce hook: locate the return statement that does "return res as unknown as Web3NonceResponse" and instead return a properly typed value — either drop the cast if the generated client already returns { nonce: string } or perform a single, explicit cast/structured extraction (e.g., destructure { nonce } from res and return as Web3NonceResponse or cast res as { nonce: string }) so TypeScript can validate the API shape; update references to the res variable and the Web3NonceResponse type in the useWeb3Nonce function accordingly.apps/next/lib/web3-sign-utils.ts (2)
9-12:isWalletRejectionmisses plain-object wallet errors.Some wallet adapters throw
{ code: 4001, message: 'User rejected the request.' }without extendingError. The current fallbackString(error)produces"[object Object]", which never matches any pattern. Extracting.messagefrom object-shaped errors would broaden detection.♻️ Proposed fix
export function isWalletRejection(error: unknown): boolean { - const msg = error instanceof Error ? error.message : String(error) + const msg = + error instanceof Error + ? error.message + : typeof error === 'object' && error !== null && 'message' in error + ? String((error as { message: unknown }).message) + : String(error) return REJECTION_PATTERNS.some(p => p.test(msg)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/web3-sign-utils.ts` around lines 9 - 12, The isWalletRejection function currently converts non-Error values with String(error), which yields "[object Object]" for plain objects and misses wallet adapters that throw { code, message }. Update isWalletRejection to detect object-shaped errors: if error is an object (and not null) and has a string "message" property, use that message; otherwise fall back to error instanceof Error ? error.message : String(error). Keep references to isWalletRejection and REJECTION_PATTERNS when making the change.
1-7:/user denied/iand/user rejected/iare redundant — subsumed by/denied/iand/rejected/i.
Array.somewith the broader patterns already matches before reaching the specific ones. No behavioral change; purely a cleanup.♻️ Proposed simplification
export const REJECTION_PATTERNS = [ /denied/i, /rejected/i, /cancel/i, - /user denied/i, - /user rejected/i, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/web3-sign-utils.ts` around lines 1 - 7, REJECTION_PATTERNS contains redundant entries: remove the more specific /user denied/i and /user rejected/i since /denied/i and /rejected/i already match them; update the exported constant REJECTION_PATTERNS in web3-sign-utils.ts to only include the broader patterns (/denied/i, /rejected/i, /cancel/i) so Array.some checks are not redundant.apps/fastify/src/lib/session.ts (3)
29-45: Session insert and token signing are not transactional — orphaned session risk on JWT error.If
db.insertsucceeds and thenfastify.jwt.signthrows (e.g., misconfigured secret), a session row is persisted without any tokens ever being issued to the client. Becausefastify.jwt.signis synchronous in@fastify/jwtv10 the risk is low, but wrapping both operations (or at minimum the insert) in a try/catch that deletes the session on failure would make the function atomic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/session.ts` around lines 29 - 45, The session row insert (db.insert(sessions).values(...)) and JWT creation (fastify.jwt.sign with payloads from createAccessTokenPayload/createRefreshTokenPayload) must be made atomic: either wrap the token signing and insert in a DB transaction or catch synchronous/signing errors and delete the newly created session (by sessionId) before rethrowing; specifically, perform token signing first or start a transaction around the insert + token creation, or if you keep the insert-first flow, surround fastify.jwt.sign calls in try/catch and call db.delete(sessions).where({ id: sessionId }) on error to avoid orphaned sessions, then rethrow the error.
29-35: Consider capturingipAddress/userAgentfor session security auditing.The
sessionsschema hasipAddressanduserAgentcolumns, but they are alwaysnullfor sessions created via this helper. Passing the Fastifyrequestobject (or its derived fields) as optional parameters would enable anomaly detection and session invalidation by device/IP without schema changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/session.ts` around lines 29 - 35, Update the session creation helper to accept optional request-derived fields and persist them: add optional parameters (e.g., ipAddress and userAgent or the Fastify request) to the function that calls db.insert(sessions).values, then include ipAddress and userAgent in the values payload alongside id (sessionId), userId, token (refreshJtiHash), expiresAt (sessionExpiresAt) and the existing wallet fields (wallet.chain, wallet.address) so the sessions.ipAddress and sessions.userAgent columns are populated for auditing and device/IP-based invalidation.
22-22:wallet.chainshould useWeb3Chain(orstringliteral union) instead ofstring.The callers pass
Web3Chainvalues; accepting barestringwidens the contract unnecessarily and loses exhaustiveness checking if more chains are added.♻️ Proposed fix
+import type { Web3Chain } from '../wallet/types.js' // adjust path as needed export async function createSessionAndIssueTokens({ fastify, db, userId, wallet, }: { fastify: FastifyInstance db: Awaited<ReturnType<typeof getDb>> userId: string - wallet?: { chain: string; address: string } + wallet?: { chain: Web3Chain; address: string } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/session.ts` at line 22, Change the wallet.chain property type from plain string to the Web3Chain union (or a string-literal union) to preserve exhaustiveness and match callers; update the wallet type declaration (the interface/type that currently contains "wallet?: { chain: string; address: string }") to use "chain: Web3Chain" and import Web3Chain from its module (or define the literal union in the same file), then run typecheck to fix any downstream call sites or imports that expect the more specific type.apps/next/components/login/wallet-options-view.tsx (1)
18-47:WalletOptionsViewduplicates theWalletSignInButtonslogic — consider composing instead.The
adaptersmap and entireWALLET_ROW_CONFIG.map(...)block (hooks, chain-specific wiring) are identical to whatWalletSignInButtonsrenders.WalletOptionsViewonly adds a back button and heading, so it can simply wrap<WalletSignInButtons />rather than re-implementing the same logic.♻️ Proposed refactor
- const evmAdapter = useWallet('eip155') - const solanaAdapter = useWallet('solana') - const chainId = useChainId() - const { connect, connectors } = useConnect() - const { setVisible: setSolanaModalVisible } = useWalletModal() - const injected = connectors.find(c => c.type === 'injected' || c.id?.includes('injected')) - const adapters = { eip155: evmAdapter, solana: solanaAdapter } + // All wallet logic lives in WalletSignInButtons return ( <div className="flex flex-col gap-4"> ... <div className="flex flex-col gap-2"> - {WALLET_ROW_CONFIG.map(({ chain, label, connectLabel }) => ( - <WalletSignInRow ... /> - ))} + <WalletSignInButtons /> </div> </div> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/components/login/wallet-options-view.tsx` around lines 18 - 47, The WalletOptionsView duplicates the adapters map and the WALLET_ROW_CONFIG.map(...) logic that WalletSignInButtons already implements; instead remove that duplicated block and render WalletSignInButtons beneath the back button and heading. Replace the entire WALLET_ROW_CONFIG.map(...) section with a single <WalletSignInButtons /> call, passing through any required props used by the original logic (e.g., chainId, injected, connect, setSolanaModalVisible) so WalletSignInButtons can perform the same chain-specific wiring; keep the existing Button (onBack) and <h2> intact. Ensure you delete the local adapters declaration and any now-unused imports/props.apps/next/lib/web3-sign-flow.ts (1)
5-15:Web3SignParamscarries bothchainId: numberandnetwork: string, but each is only used for one chain branch.For
eip155,networkis unused; for Solana,chainId(the number) is unused. This makes the type harder to read and allows callers to accidentally omit the relevant field without a compile-time error. A discriminated union would make the intent explicit and eliminate dead parameters per branch.♻️ Proposed refactor (optional)
-export type Web3SignParams = { - chain: Web3Chain - address: string - signMessage: (message: string | Uint8Array) => Promise<{ signature: string }> - nonce: string - domain: string - statement: string - uri: string - chainId: number - network: string -} +type CommonSignParams = { + address: string + signMessage: (message: string | Uint8Array) => Promise<{ signature: string }> + nonce: string + domain: string + statement: string + uri: string +} + +export type Web3SignParams = + | (CommonSignParams & { chain: 'eip155'; chainId: number }) + | (CommonSignParams & { chain: Exclude<Web3Chain, 'eip155'>; network: string })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/web3-sign-flow.ts` around lines 5 - 15, Web3SignParams currently contains both chainId and network even though chainId is only used for the eip155 branch and network only for Solana; change Web3SignParams into a discriminated union keyed on the existing chain field (e.g., Web3SignParams = { chain: 'eip155'; chainId: number; address: string; signMessage: ...; nonce: string; domain: string; statement: string; uri: string; network?: never } | { chain: 'solana'; network: string; address: string; signMessage: ...; nonce: string; domain: string; statement: string; uri: string; chainId?: never }) so callers must supply the correct field per chain and update call sites and any type annotations that reference Web3SignParams, and ensure code paths that handle eip155 vs solana use the narrowed type.apps/next/e2e/wallet-metamask-auth.spec.ts (1)
16-18: Prefer a condition-based wait overwaitForTimeout.
page.waitForTimeout(3000)is a hardcoded sleep that makes tests slower and potentially flaky. Since line 19 already waits forsignInBtnto be visible, consider removing the sleep and relying on that assertion's 20s timeout, or use a more targeted condition like waiting for the wallet address to appear in the DOM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/e2e/wallet-metamask-auth.spec.ts` around lines 16 - 18, Replace the hard sleep used for "Wagmi + EIP-6963 mock" (page.waitForTimeout(3000)) with a condition-based wait: remove the waitForTimeout call and instead wait for the actual UI condition used later (the signInBtn visibility or the wallet address DOM node) to appear using its selector/handle (e.g., the signInBtn locator or the wallet address element) with an explicit timeout (eg. 20s); update the test around where signInBtn is awaited so the test proceeds only after that visibility or the wallet-address selector is present, eliminating the fixed 3s delay and preventing flakiness.apps/next/lib/auth-error-messages.ts (1)
1-15: Inconsistent key casing in error code map.Some keys use
UPPER_SNAKE_CASE(e.g.,INVALID_TOKEN) while others uselower_snake_case(e.g.,missing_params,oauth_failed). This mix can lead to lookup misses if a new code is added with the wrong convention. Consider normalizing—either standardize all keys to one convention or normalize the lookup key ingetAuthErrorMessage(e.g.,errorCode.toUpperCase()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/auth-error-messages.ts` around lines 1 - 15, AUTH_ERROR_MESSAGES contains mixed key casing (e.g., INVALID_TOKEN vs missing_params) which can cause lookup misses; normalize by choosing one convention and updating keys in AUTH_ERROR_MESSAGES to match it (recommended: UPPER_SNAKE_CASE) or change the lookup in getAuthErrorMessage to normalize incoming codes (e.g., call errorCode.toUpperCase() or a consistent transformer before accessing AUTH_ERROR_MESSAGES) so keys always match; update any callers if needed to use the same normalization and ensure keys like missing_params, oauth_failed, and unexpected_error are converted to the chosen format.apps/fastify/src/routes/auth/web3/eip155/verify.ts (1)
49-62:SiweMessageis constructed twice from the same raw string.
parseMessage(line 51) andverifySignature(line 59) each instantiatenew SiweMessage(msg). This is a consequence of theverifyWeb3Authabstraction only passing the raw message string toverifySignature. Not a bug, but a minor inefficiency you could revisit if the abstraction evolves (e.g., allowingparseMessageto return an opaque parsed object passed through toverifySignature).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/web3/eip155/verify.ts` around lines 49 - 62, parseMessage and verifySignature both construct a new SiweMessage from the same raw string causing duplicate parsing; update the flow so the parsed SiweMessage is created once and reused: modify parseMessage (in this file) to return the parsed SiweMessage (or an object containing both the raw msg and the parsed SiweMessage) instead of only primitive fields, and update verifySignature to accept and use that parsed SiweMessage instance rather than calling new SiweMessage(msg) again; if necessary, adjust the verifyWeb3Auth abstraction to allow parseMessage to return an opaque parsed object that verifySignature can accept (refer to parseMessage, verifySignature, SiweMessage, and verifyWeb3Auth to locate the code).apps/fastify/src/lib/auth-web3.ts (2)
87-105: Unnecessary re-query after user creation —userIdis already known.After the transaction on lines 89-102, you already hold the
userId(line 88). The SELECT on line 103 is a redundant DB round-trip since the only field used downstream isuser.id(line 115), which equalsuserId. If the transaction failed, it would have thrown before reaching line 103.♻️ Proposed simplification
if (!user) { const userId = randomUUID() await db.transaction(async tx => { await tx.insert(users).values({ id: userId, email: null, emailVerified: false, }) await tx.insert(walletIdentities).values({ id: randomUUID(), userId, chain, address: validatedAddress, walletProvider: null, }) }) - const [created] = await db.select().from(users).where(eq(users.id, userId)) - if (!created) throw new Error('Failed to create user') - user = created + user = { id: userId } as typeof users.$inferSelect } else if (wallet) {Alternatively, since
user.idis the only field consumed downstream, you could restructure to trackuserIddirectly and avoid theuservariable altogether:+ let resolvedUserId: string + if (!user) { - const userId = randomUUID() + resolvedUserId = randomUUID() await db.transaction(async tx => { await tx.insert(users).values({ - id: userId, + id: resolvedUserId, ... }) ... }) - const [created] = await db.select().from(users).where(eq(users.id, userId)) - if (!created) throw new Error('Failed to create user') - user = created } else if (wallet) { + resolvedUserId = user.id ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/auth-web3.ts` around lines 87 - 105, The block in auth-web3.ts performs a redundant SELECT after creating a user; after the transaction you already have userId (randomUUID()) so remove the db.select() re-query and downstream references to user.id should use userId (or set user = { id: userId } if a user object is required). Update the code paths that consume `user` (e.g., where `user.id` is used later in this function) to use the known `userId` or the minimal user object instead of re-fetching from the database.
106-111:else if (wallet)is always true here — simplify toelse.At line 106,
useris truthy (we passed theif (!user)check). Becauseuseris only ever assigned inside theif (wallet)block (line 84),walletis guaranteed truthy wheneveruseris defined. The conditional reads as though there's an unhandled third case (user found, wallet not found), which is unreachable. Using a plainelseclarifies the intent.♻️ Proposed fix
- } else if (wallet) { + } else { await db .update(walletIdentities) .set({ lastUsedAt: new Date() }) .where(eq(walletIdentities.id, wallet.id)) }If you switch to plain
else, you may need a non-null assertion or guard onwalletfor TypeScript's narrowing. A brief comment explaining the invariant would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/auth-web3.ts` around lines 106 - 111, The conditional "else if (wallet)" in the auth flow is redundant because when you reach that branch "user" is truthy and therefore "wallet" must be defined; replace the "else if (wallet)" with a plain "else" in the block that updates walletIdentities via db.update(...). Ensure TypeScript compiles by either using a non-null assertion on "wallet" (wallet!.id) or add a short runtime guard/assert before calling db.update, and include a one-line comment noting the invariant that wallet is present when user is truthy to aid future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/next/e2e/auth-helpers.ts`:
- Around line 106-110: Replace the hard-coded waits around the EVM wallet
connect flow: drop the initial await page.waitForTimeout(2000) and instead
immediately check/connect using connectBtn (or call connectBtn.waitFor({ state:
'visible', timeout: ... }) if you need an explicit wait), and remove the
redundant await page.waitForTimeout(3000) after connectBtn.click() because the
subsequent signInBtn.waitFor already waits for the post-connect state; update
the logic around connectBtn and signInBtn so you rely on visibility/waitFor
rather than fixed timeouts.
In `@apps/next/e2e/fixtures-solana-mock.ts`:
- Around line 63-66: Disable the false-positive react-hooks/rules-of-hooks for
Playwright fixture files by adding an ESLint override scoped to apps/next/e2e/**
that turns off react-hooks/rules-of-hooks (preferred), or as a quick per-file
workaround add an inline eslint-disable comment at the top of this file; target
the fixture function containing page: async ({ page }, use) => { await
installSolanaMock(page); await use(page) } so the rule no longer flags the
Playwright `use` callback.
In `@apps/next/e2e/fixtures-wallet-mock.ts`:
- Around line 12-20: This file triggers a false positive from
react-hooks/rules-of-hooks in the page fixture using installMockWallet; either
add the same ESLint override used for apps/next/e2e/** to your ESLint config so
files under that glob ignore react-hooks/rules-of-hooks, or add a per-file
suppression at the top of fixtures-wallet-mock.ts to disable
react-hooks/rules-of-hooks for this file; target the page fixture and
installMockWallet call when applying the change.
In `@apps/next/e2e/fixtures.ts`:
- Around line 29-34: The react-hooks/rules-of-hooks linter false positive is
triggered inside the authenticatedPage fixture arrow function; apply the same
fix used in fixtures-solana-mock.ts by adding the identical ESLint directive
(e.g., the react-hooks/rules-of-hooks disable comment) immediately above the
authenticatedPage declaration to suppress the rule for this fixture, so linting
ignores the false positive while leaving the implementation of authenticatedPage
and its use of browser.newContext/newPage unchanged.
---
Outside diff comments:
In `@apps/fastify/src/plugins/error-handler.ts`:
- Around line 72-75: The code uses request.routerPath (guard `'routerPath' in
request`) which is removed in Fastify v5 causing routePath to always use the raw
URL; update the routePath assignment to use request.routeOptions?.url first and
fall back to the URL split and '/' so path params are preserved in Sentry.
Locate the routePath variable assignment and replace the routerPath check with
using request.routeOptions?.url as the primary source, then fallback to
request.url.split('?')[0] ?? '/' so routes like /auth/web3/eip155/:address are
captured instead of leaking raw parameter values.
In `@packages/react/src/hooks/use-verify-web3-auth.ts`:
- Around line 33-43: The SDK verify call returns a wrapper with .data, so change
the handling of verifyResult in use-verify-web3-auth: after calling
client.auth.web3.eip155.verify(...) or client.auth.web3.solana.verify(...),
extract the payload with verifyResult.data (or alternatively pass responseStyle:
'data' into both verify calls) and assign that to result so the subsequent code
reads token/refreshToken from the unwrapped Web3Eip155VerifyResponse |
Web3SolanaVerifyResponse object; update references to verifyResult ->
verifyResult.data (or add responseStyle: 'data' to both
client.auth.web3.eip155.verify and client.auth.web3.solana.verify calls).
---
Nitpick comments:
In `@apps/fastify/src/lib/auth-web3.ts`:
- Around line 87-105: The block in auth-web3.ts performs a redundant SELECT
after creating a user; after the transaction you already have userId
(randomUUID()) so remove the db.select() re-query and downstream references to
user.id should use userId (or set user = { id: userId } if a user object is
required). Update the code paths that consume `user` (e.g., where `user.id` is
used later in this function) to use the known `userId` or the minimal user
object instead of re-fetching from the database.
- Around line 106-111: The conditional "else if (wallet)" in the auth flow is
redundant because when you reach that branch "user" is truthy and therefore
"wallet" must be defined; replace the "else if (wallet)" with a plain "else" in
the block that updates walletIdentities via db.update(...). Ensure TypeScript
compiles by either using a non-null assertion on "wallet" (wallet!.id) or add a
short runtime guard/assert before calling db.update, and include a one-line
comment noting the invariant that wallet is present when user is truthy to aid
future readers.
In `@apps/fastify/src/lib/session.ts`:
- Around line 29-45: The session row insert (db.insert(sessions).values(...))
and JWT creation (fastify.jwt.sign with payloads from
createAccessTokenPayload/createRefreshTokenPayload) must be made atomic: either
wrap the token signing and insert in a DB transaction or catch
synchronous/signing errors and delete the newly created session (by sessionId)
before rethrowing; specifically, perform token signing first or start a
transaction around the insert + token creation, or if you keep the insert-first
flow, surround fastify.jwt.sign calls in try/catch and call
db.delete(sessions).where({ id: sessionId }) on error to avoid orphaned
sessions, then rethrow the error.
- Around line 29-35: Update the session creation helper to accept optional
request-derived fields and persist them: add optional parameters (e.g.,
ipAddress and userAgent or the Fastify request) to the function that calls
db.insert(sessions).values, then include ipAddress and userAgent in the values
payload alongside id (sessionId), userId, token (refreshJtiHash), expiresAt
(sessionExpiresAt) and the existing wallet fields (wallet.chain, wallet.address)
so the sessions.ipAddress and sessions.userAgent columns are populated for
auditing and device/IP-based invalidation.
- Line 22: Change the wallet.chain property type from plain string to the
Web3Chain union (or a string-literal union) to preserve exhaustiveness and match
callers; update the wallet type declaration (the interface/type that currently
contains "wallet?: { chain: string; address: string }") to use "chain:
Web3Chain" and import Web3Chain from its module (or define the literal union in
the same file), then run typecheck to fix any downstream call sites or imports
that expect the more specific type.
In `@apps/fastify/src/plugins/error-handler.ts`:
- Around line 45-67: The redactBody function performs only a shallow copy and
fails to redact nested sensitive fields and mishandles arrays; update redactBody
to recursively traverse objects and arrays, preserving the original structure
(arrays remain arrays, objects remain objects), and replace values for any key
matching entries in the sensitiveFields list (e.g.,
'password','token','accessToken', etc.) with '[REDACTED]'; ensure you don't
mutate the input by creating new objects/arrays during recursion, handle
non-plain values by returning them unchanged, and reference the existing
redactBody and sensitiveFields identifiers when implementing the recursive
sanitizer.
In `@apps/fastify/src/routes/auth/web3/eip155/verify.ts`:
- Around line 49-62: parseMessage and verifySignature both construct a new
SiweMessage from the same raw string causing duplicate parsing; update the flow
so the parsed SiweMessage is created once and reused: modify parseMessage (in
this file) to return the parsed SiweMessage (or an object containing both the
raw msg and the parsed SiweMessage) instead of only primitive fields, and update
verifySignature to accept and use that parsed SiweMessage instance rather than
calling new SiweMessage(msg) again; if necessary, adjust the verifyWeb3Auth
abstraction to allow parseMessage to return an opaque parsed object that
verifySignature can accept (refer to parseMessage, verifySignature, SiweMessage,
and verifyWeb3Auth to locate the code).
In `@apps/next/components/login/wallet-options-view.tsx`:
- Around line 18-47: The WalletOptionsView duplicates the adapters map and the
WALLET_ROW_CONFIG.map(...) logic that WalletSignInButtons already implements;
instead remove that duplicated block and render WalletSignInButtons beneath the
back button and heading. Replace the entire WALLET_ROW_CONFIG.map(...) section
with a single <WalletSignInButtons /> call, passing through any required props
used by the original logic (e.g., chainId, injected, connect,
setSolanaModalVisible) so WalletSignInButtons can perform the same
chain-specific wiring; keep the existing Button (onBack) and <h2> intact. Ensure
you delete the local adapters declaration and any now-unused imports/props.
In `@apps/next/e2e/fixtures.ts`:
- Around line 14-34: The fixture authenticatedStorageState is inconsistent with
authenticatedPage: it uses the old Playwright fixture callback name runWith
instead of use; change the parameter name in the async function signature from
runWith to use and replace the call runWith(authFile) with use(authFile) so both
fixtures use the same Playwright callback identifier (referencing
authenticatedStorageState, authenticatedPage, runWith, use).
In `@apps/next/e2e/wallet-metamask-auth.spec.ts`:
- Around line 16-18: Replace the hard sleep used for "Wagmi + EIP-6963 mock"
(page.waitForTimeout(3000)) with a condition-based wait: remove the
waitForTimeout call and instead wait for the actual UI condition used later (the
signInBtn visibility or the wallet address DOM node) to appear using its
selector/handle (e.g., the signInBtn locator or the wallet address element) with
an explicit timeout (eg. 20s); update the test around where signInBtn is awaited
so the test proceeds only after that visibility or the wallet-address selector
is present, eliminating the fixed 3s delay and preventing flakiness.
In `@apps/next/hooks/use-web3-nonce.ts`:
- Line 30: Replace the unsafe double-cast on the API response in the
useWeb3Nonce hook: locate the return statement that does "return res as unknown
as Web3NonceResponse" and instead return a properly typed value — either drop
the cast if the generated client already returns { nonce: string } or perform a
single, explicit cast/structured extraction (e.g., destructure { nonce } from
res and return as Web3NonceResponse or cast res as { nonce: string }) so
TypeScript can validate the API shape; update references to the res variable and
the Web3NonceResponse type in the useWeb3Nonce function accordingly.
In `@apps/next/lib/auth-client.ts`:
- Around line 24-31: The IIFE used to compute errorDetail is unnecessary
indirection; replace it with a simple try/catch block that assigns errorDetail
from parsing bodyText (falling back to bodyText) and then default to `HTTP
${response.status}` if falsy. Locate the errorDetail declaration in
auth-client.ts where bodyText and response are available, remove the
immediately-invoked function, and implement a flat try { parsed =
JSON.parse(bodyText); errorDetail = parsed?.message ?? bodyText } catch {
errorDetail = bodyText } followed by the final fallback to `HTTP
${response.status}`.
In `@apps/next/lib/auth-error-messages.ts`:
- Around line 1-15: AUTH_ERROR_MESSAGES contains mixed key casing (e.g.,
INVALID_TOKEN vs missing_params) which can cause lookup misses; normalize by
choosing one convention and updating keys in AUTH_ERROR_MESSAGES to match it
(recommended: UPPER_SNAKE_CASE) or change the lookup in getAuthErrorMessage to
normalize incoming codes (e.g., call errorCode.toUpperCase() or a consistent
transformer before accessing AUTH_ERROR_MESSAGES) so keys always match; update
any callers if needed to use the same normalization and ensure keys like
missing_params, oauth_failed, and unexpected_error are converted to the chosen
format.
In `@apps/next/lib/web3-sign-flow.ts`:
- Around line 5-15: Web3SignParams currently contains both chainId and network
even though chainId is only used for the eip155 branch and network only for
Solana; change Web3SignParams into a discriminated union keyed on the existing
chain field (e.g., Web3SignParams = { chain: 'eip155'; chainId: number; address:
string; signMessage: ...; nonce: string; domain: string; statement: string; uri:
string; network?: never } | { chain: 'solana'; network: string; address: string;
signMessage: ...; nonce: string; domain: string; statement: string; uri: string;
chainId?: never }) so callers must supply the correct field per chain and update
call sites and any type annotations that reference Web3SignParams, and ensure
code paths that handle eip155 vs solana use the narrowed type.
In `@apps/next/lib/web3-sign-utils.ts`:
- Around line 9-12: The isWalletRejection function currently converts non-Error
values with String(error), which yields "[object Object]" for plain objects and
misses wallet adapters that throw { code, message }. Update isWalletRejection to
detect object-shaped errors: if error is an object (and not null) and has a
string "message" property, use that message; otherwise fall back to error
instanceof Error ? error.message : String(error). Keep references to
isWalletRejection and REJECTION_PATTERNS when making the change.
- Around line 1-7: REJECTION_PATTERNS contains redundant entries: remove the
more specific /user denied/i and /user rejected/i since /denied/i and
/rejected/i already match them; update the exported constant REJECTION_PATTERNS
in web3-sign-utils.ts to only include the broader patterns (/denied/i,
/rejected/i, /cancel/i) so Array.some checks are not redundant.
| await page.waitForTimeout(2000) | ||
| if (await connectBtn.isVisible().catch(() => false)) { | ||
| await connectBtn.click() | ||
| await page.waitForTimeout(3000) | ||
| } |
There was a problem hiding this comment.
Hard-coded waitForTimeout calls make the EVM wallet login test slow and brittle.
- Line 106: The 2-second wait before checking
connectBtn.isVisible()is a fixed delay. It should be replaced with a condition-based wait. - Line 109: The 3-second wait after clicking
connectBtnshould instead wait forsignInBtnto appear (which already happens on line 111 — so line 109 is simply redundant).
♻️ Proposed fix
- await page.waitForTimeout(2000)
if (await connectBtn.isVisible().catch(() => false)) {
await connectBtn.click()
- await page.waitForTimeout(3000)
}
await signInBtn.waitFor({ state: 'visible', timeout: 20_000 })The connectBtn.isVisible() check can proceed immediately after the heading is confirmed visible (line 102-103 already does a waitFor). And signInBtn.waitFor on line 111 already handles waiting for the post-connect state, making the 3-second delay fully redundant.
📝 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.
| await page.waitForTimeout(2000) | |
| if (await connectBtn.isVisible().catch(() => false)) { | |
| await connectBtn.click() | |
| await page.waitForTimeout(3000) | |
| } | |
| if (await connectBtn.isVisible().catch(() => false)) { | |
| await connectBtn.click() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/next/e2e/auth-helpers.ts` around lines 106 - 110, Replace the hard-coded
waits around the EVM wallet connect flow: drop the initial await
page.waitForTimeout(2000) and instead immediately check/connect using connectBtn
(or call connectBtn.waitFor({ state: 'visible', timeout: ... }) if you need an
explicit wait), and remove the redundant await page.waitForTimeout(3000) after
connectBtn.click() because the subsequent signInBtn.waitFor already waits for
the post-connect state; update the logic around connectBtn and signInBtn so you
rely on visibility/waitFor rather than fixed timeouts.
| page: async ({ page }, use) => { | ||
| await installSolanaMock(page) | ||
| await runWith(page) | ||
| await use(page) | ||
| }, |
There was a problem hiding this comment.
ESLint false positive: react-hooks/rules-of-hooks fires on Playwright's use callback — add suppress or fix at config level.
This is a known false positive: eslint-plugin-react-hooks treats the Playwright fixture use parameter exactly like React.use, triggering the "React Hook 'use' is called in function 'page'" error. The coding guidelines require ESLint to pass before merge.
Two options:
- Inline suppress (quick fix per file):
🔧 Inline eslint-disable
page: async ({ page }, use) => {
await installSolanaMock(page)
+ // eslint-disable-next-line react-hooks/rules-of-hooks
await use(page)
},- ESLint config override (preferred — fixes all three affected fixture files at once): add a
react-hooks/rules-of-hooks: offoverride scoped toapps/next/e2e/**in the project's ESLint config, since these files contain no React hooks.
As per coding guidelines: "Code must pass both Biome (formatting/style) and ESLint (correctness) before merge."
📝 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.
| page: async ({ page }, use) => { | |
| await installSolanaMock(page) | |
| await runWith(page) | |
| await use(page) | |
| }, | |
| page: async ({ page }, use) => { | |
| await installSolanaMock(page) | |
| // eslint-disable-next-line react-hooks/rules-of-hooks | |
| await use(page) | |
| }, |
🧰 Tools
🪛 GitHub Check: Lint Code
[warning] 65-65:
React Hook "use" is called in function "page" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/next/e2e/fixtures-solana-mock.ts` around lines 63 - 66, Disable the
false-positive react-hooks/rules-of-hooks for Playwright fixture files by adding
an ESLint override scoped to apps/next/e2e/** that turns off
react-hooks/rules-of-hooks (preferred), or as a quick per-file workaround add an
inline eslint-disable comment at the top of this file; target the fixture
function containing page: async ({ page }, use) => { await
installSolanaMock(page); await use(page) } so the rule no longer flags the
Playwright `use` callback.
| page: async ({ page }, use) => { | ||
| await installMockWallet({ | ||
| page, | ||
| account: privateKeyToAccount(TEST_PRIVATE_KEY as `0x${string}`), | ||
| defaultChain: mainnet, | ||
| transports: { [mainnet.id]: http() }, | ||
| }) | ||
| await runWith(page) | ||
| await use(page) | ||
| }, |
There was a problem hiding this comment.
Same react-hooks/rules-of-hooks false positive as fixtures-solana-mock.ts — suppression needed here too.
The recommended fix is the ESLint config override scoped to apps/next/e2e/** (noted in fixtures-solana-mock.ts). If opting for per-file suppression instead:
🔧 Inline eslint-disable
page: async ({ page }, use) => {
await installMockWallet({ ... })
+ // eslint-disable-next-line react-hooks/rules-of-hooks
await use(page)
},🧰 Tools
🪛 GitHub Check: Lint Code
[warning] 19-19:
React Hook "use" is called in function "page" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/next/e2e/fixtures-wallet-mock.ts` around lines 12 - 20, This file
triggers a false positive from react-hooks/rules-of-hooks in the page fixture
using installMockWallet; either add the same ESLint override used for
apps/next/e2e/** to your ESLint config so files under that glob ignore
react-hooks/rules-of-hooks, or add a per-file suppression at the top of
fixtures-wallet-mock.ts to disable react-hooks/rules-of-hooks for this file;
target the page fixture and installMockWallet call when applying the change.
| authenticatedPage: async ({ authenticatedStorageState, browser }, use) => { | ||
| const context = await browser.newContext({ baseURL, storageState: authenticatedStorageState }) | ||
| const page = await context.newPage() | ||
| await runWith(page) | ||
| await use(page) | ||
| await context.close() | ||
| }, |
There was a problem hiding this comment.
Same react-hooks/rules-of-hooks false positive (line 32) — see fixtures-solana-mock.ts for the recommended config fix.
🧰 Tools
🪛 GitHub Check: Lint Code
[warning] 32-32:
React Hook "use" is called in function "authenticatedPage" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/next/e2e/fixtures.ts` around lines 29 - 34, The
react-hooks/rules-of-hooks linter false positive is triggered inside the
authenticatedPage fixture arrow function; apply the same fix used in
fixtures-solana-mock.ts by adding the identical ESLint directive (e.g., the
react-hooks/rules-of-hooks disable comment) immediately above the
authenticatedPage declaration to suppress the rule for this fixture, so linting
ignores the false positive while leaving the implementation of authenticatedPage
and its use of browser.newContext/newPage unchanged.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Documentation