feat: Add Farcaster miniapp support for identity verification flow #10 #14
Conversation
|
You've used up your 5 PR reviews for this month under the Korbit Starter Plan. You'll get 5 more reviews on August 19th, 2025 or you can upgrade to Pro for unlimited PR reviews and enhanced features in your Korbit Console. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the repeated list of externalized Node.js modules into a shared build config or utility to avoid duplication across your Vite and tsup configurations.
- Cache or hoist the dynamic import of '@farcaster/miniapp-sdk' so you’re not re-importing it multiple times during fallback detection and navigation.
- Refactor the universal link generation and Farcaster navigation logic into a single shared helper or custom hook to eliminate duplication across IdentitySDKs and the demo app.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repeated list of externalized Node.js modules into a shared build config or utility to avoid duplication across your Vite and tsup configurations.
- Cache or hoist the dynamic import of '@farcaster/miniapp-sdk' so you’re not re-importing it multiple times during fallback detection and navigation.
- Refactor the universal link generation and Farcaster navigation logic into a single shared helper or custom hook to eliminate duplication across IdentitySDKs and the demo app.
## Individual Comments
### Comment 1
<location> `packages/citizen-sdk/src/utils/auth.ts:94` </location>
<code_context>
+ * @param url - The current URL or callback URL to parse
+ * @returns Object containing verification status and any additional parameters
+ */
+export function handleVerificationResponse(url?: string): {
+ isVerified: boolean;
+ params: URLSearchParams;
</code_context>
<issue_to_address>
Parsing the URL without error handling may throw if the input is malformed.
If parsing fails due to an invalid URL, the function will throw. Please add a try/catch block and return a default value when parsing fails.
</issue_to_address>
### Comment 2
<location> `packages/citizen-sdk/src/utils/auth.ts:7` </location>
<code_context>
+/**
+ * Detects if the SDK is running inside a Farcaster miniapp using the official SDK
+ */
+export async function isInFarcasterMiniApp(timeoutMs: number = 100): Promise<boolean> {
+ if (typeof window === "undefined") return false;
+
</code_context>
<issue_to_address>
Consider refactoring repeated dynamic imports and fallback logic into helper functions to simplify and flatten the code structure.
Here’s one way to collapse all of the repeated dynamic‐imports, pull the fallback logic out into a small helper, and flatten the nesting in isInFarcasterMiniApp / openUrlInFarcaster:
```ts
// new helper to lazy-load & cache the SDK
let _cachedSdk: typeof import('@farcaster/miniapp-sdk').sdk | null = null;
async function loadFarcasterSdk() {
if (!_cachedSdk) {
const { sdk } = await import('@farcaster/miniapp-sdk');
_cachedSdk = sdk;
}
return _cachedSdk;
}
// new helper for the fallback context check
async function fallbackDetect() {
try {
const sdk = await loadFarcasterSdk();
const ctx = await sdk.context;
return !!(ctx.location && ctx.location.type != null);
} catch {
return false;
}
}
export async function isInFarcasterMiniApp(timeoutMs = 100): Promise<boolean> {
if (typeof window === 'undefined') return false;
try {
const sdk = await loadFarcasterSdk();
return await sdk.isInMiniApp();
} catch {
console.warn('SDK failed, trying context fallback…');
return fallbackDetect();
}
}
export async function openUrlInFarcaster(
url: string,
fallbackToNewTab = true
): Promise<void> {
if (typeof window === 'undefined') {
throw new Error('Browser only');
}
if (await isInFarcasterMiniApp()) {
try {
const sdk = await loadFarcasterSdk();
await sdk.actions.ready();
await sdk.actions.openUrl(url);
return;
} catch {
console.warn('SDK.openUrl failed, falling back…');
}
}
fallbackToNewTab ? window.open(url, '_blank') : (window.location.href = url);
}
```
Benefits:
• All `import('@farcaster/miniapp-sdk')` calls are in one place and cached
• Fallback context logic lives in its own small function
• Try/catch blocks are only one level deep in each API function
• Behaviour is unchanged.
</issue_to_address>
### Comment 3
<location> `apps/demo-identity-app/vite.config.mts:37` </location>
<code_context>
},
+ build: {
+ rollupOptions: {
+ external: [
+ // External Node.js modules that should not be bundled for browser
+ "crypto",
</code_context>
<issue_to_address>
Consider generating the list of Node.js built-ins and their globals dynamically using module.builtinModules instead of maintaining a manual array.
You can eliminate the manual list entirely by pulling Node’s built-ins from `module.builtinModules` (and generate the `node:`-prefixed variants on the fly). For example:
```js
// vite.config.js
import { builtinModules } from 'module'
const builtins = [
...builtinModules,
...builtinModules.map((m) => `node:${m}`)
]
const globals = builtins.reduce((acc, name) => {
// strip the `node:` prefix for the global key if present
const key = name.startsWith('node:') ? name.slice(5) : name
// map both `foo` and `node:foo` → global `foo`
acc[name] = key
return acc
}, {})
export default defineConfig({
// …other config…
build: {
rollupOptions: {
external: builtins,
output: { globals }
}
}
})
```
This keeps every built-in, automatically picks up new ones, and avoids the long hand-written arrays.
</issue_to_address>
### Comment 4
<location> `packages/ui-components/tsup.config.claim.ts:16` </location>
<code_context>
},
+ build: {
+ rollupOptions: {
+ external: [
+ // External Node.js modules that should not be bundled for browser
+ "crypto",
</code_context>
<issue_to_address>
Consider dynamically listing Node.js built-ins or using a regex to avoid manually maintaining a long list of external dependencies.
You can drop the hundreds-of-lines hand–listing by pulling Node’s built-ins dynamically from the `module` package (and catching both plain and `node:` names). For example:
```ts
// tsup.config.ts
import { defineConfig } from "tsup"
import { builtinModules } from "module"
const nodeBuiltins = [
...builtinModules, // ['fs','path',…]
...builtinModules.map((m) => `node:${m}`), // ['node:fs','node:path',…]
]
export default defineConfig({
entry: ["src/index.ts"],
format: ["esm"],
platform: "browser",
globalName: "ClaimButton",
splitting: false,
sourcemap: false,
clean: true,
dts: false,
minify: true,
target: "ESNext",
outDir: "dist",
external: [
"@goodsdks/citizen-sdk",
"viem",
...nodeBuiltins,
],
noExternal: [
"lit",
"@reown/appkit",
"@reown/appkit-adapter-ethers",
"ethers",
],
})
```
If you’d rather use a regex to catch all `node:` imports and leave plain built-ins implicit, you can shrink it even more:
```ts
export default defineConfig({
// …
external: [
"@goodsdks/citizen-sdk",
"viem",
/^node:/, // all `node:xxx`
// (esbuild will already treat bare built-ins as external in browser builds)
],
// …
})
```
Either approach removes the manual maintenance burden while keeping the same behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
review changes by sorcery-ai implemented |
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
I also think I miss seeing handling of the 'verified' param that you get when returning from the face-verification?
The verified param from face verification is already properly handled in the demo-identity-app (App.tsx lines 59-71) using the |
|
also, I'd work on building a test mini-app on farcaster, and demonstrate how it works |
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
- The code needs to be organized better. Many duplicates, single line functions etc.
- since farcaster always opens a new tab/window it is possibly best to force cbu, ie popup mode, where the tab/window will be closed after FV is done without redirect back.
- for cbu need to update the check is verified, to not only check the url for isVerified param (since there is no redirect with that param in cbu mode) but also to accept as input from developer the wallet address and check on-chain if wallet is now whitelisted
apps/demo-identity-app/package.json
Outdated
| "format": "prettier --write ." | ||
| }, | ||
| "dependencies": { | ||
| "@farcaster/miniapp-sdk": "^0.1.8", |
There was a problem hiding this comment.
this should be devdep/peerdep
| build: { | ||
| rollupOptions: { | ||
| external: ["@goodsdks/citizen-sdk"] | ||
| } | ||
| } |
There was a problem hiding this comment.
doesnt make sense this has to be included
There was a problem hiding this comment.
This external configuration is required to prevent the citizen-sdk (which contains Node.js dependencies) from being bundled in the browser environment. Without this, the build fails with 'Agent is not exported by vite-browser-external' errors. The citizen-sdk needs to remain external for this demo app.
| /** | ||
| * Create a universal link compatible callback URL | ||
| */ | ||
| private createUniversalLinkCallback( |
| url: string, | ||
| fallbackToNewTab: boolean = true | ||
| ): Promise<void> { | ||
| if (typeof window === "undefined") { |
There was a problem hiding this comment.
duplicate code. if in farcaster then it is obvious theres a window this check is redundant
|
@L03TJ3 the non custodial is just a duplicate code requiring now multiple maintenance. it should be integrated into the regular sdk |
… and added farcaster/miniapp-sdk as dependency to citizen-sdk
|
Hi @sirpy @L03TJ3 , I've been able to test it in web/farcaster environments, here's the loom: Demo in Forecaster & Web, also thanks Lewis for other time, and Emiri helped me alot too |
|
Do let me know if there's anything else pending, thanks alot. |
|
Hi @L03TJ3 have you gotten time to review this pr and also probably watch the loom, so I can complete this task. |
|
@vortex-hue Please make sure you went over all the open items and made the relevant changes. @L03TJ3 should review this by end of week |
Hi @sirpy, I went through all open items and also made the changes requested, is there any specific one I missed? and sure, let me try testing it on mobile right away. |
| window.location.href.includes("farcaster") || | ||
| window.location.href.includes("miniapp") || | ||
| // Check if we're in an iframe which is common for miniapps | ||
| window.self !== window.top |
| if (typeof window === "undefined") { | ||
| throw new Error("URL opening is only supported in browser environments."); | ||
| } |
| * @param additionalParams - Additional parameters to include | ||
| * @returns A universal link compatible URL | ||
| */ | ||
| export function createUniversalLinkCallback( |
There was a problem hiding this comment.
I requested this flow so that the verified param can be appened and a app can handle the redirectBack from face-verification.
You think this should be handled different?
| * @param additionalParams - Additional parameters to include | ||
| * @returns A universal link compatible URL | ||
| */ | ||
| export function createUniversalLinkCallback( |
There was a problem hiding this comment.
@vortex-hue pending on @sirpy response, make sure the verified param is handled.
example here: https://github.com/GoodDollar/GoodWeb3-Mono/blob/70557ec8977959199e7a95e4872b6d3bca3e8327/packages/good-design/src/core/buttons/ClaimButton.tsx#L31
|
Deployment failed with the following error: |
… also created packages/build-config/index.ts with dynamic Node.js built-ins generation
|
hi @L03TJ3 , I've reverted them, you can check it now. |
| ) | ||
| const { sdk: identitySDK } = useIdentitySDK("development") | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
- clean this up. if we want to show someone how the sdk could be implemented, this is not easy to follow
- perhaps consider making it part of the
VerifyButton.tsx? - include a README with examples in citizen-sdk for how it works/should be implemented
There was a problem hiding this comment.
Hi Lewis, I don't fully understand this, mind explaining deeper please?
There was a problem hiding this comment.
do you mean I should revert the file to how it was initially? and remove every code I added relating to the forecaster support?
vortex-hue
force-pushed
the
add-farcaster-support-for-the-identity-flow
branch
from
bc861ab to
3bd36b1
Compare
…-for-the-identity-flow
vortex-hue
force-pushed
the
add-farcaster-support-for-the-identity-flow
branch
from
2654e1c to
4060809
Compare
|
|
||
| interface VerifyButtonProps { | ||
| onVerificationSuccess: () => void | ||
| // No props needed - verification success is handled by URL callback detection |
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
the project does not build:
│ src/sdks/viem-claim-sdk.ts:20:2: ERROR: No matching export in "src/constants.ts" for import "contractAddresses"
│ src/sdks/viem-claim-sdk.ts:22:26: ERROR: No matching export in "src/constants.ts" for import "getGasPrice"
│ src/utils/auth.ts:2:50: ERROR: No matching export in "src/constants.ts" for import "contractAddresses"
| import { waitForTransactionReceipt } from "viem/actions" | ||
|
|
||
| import { IdentitySDK } from "./viem-identity-sdk" | ||
| import { createUniversalLinkCallback } from "../utils/auth" |
There was a problem hiding this comment.
This does not seem to be used, why is it imported?
Resolved conflicts in: - packages/citizen-sdk/src/sdks/viem-claim-sdk.ts - packages/citizen-sdk/src/sdks/viem-identity-sdk.ts Merged changes: - Kept Farcaster miniapp support (Universal Links, callback URLs) - Integrated new chain management system with fallbacks - Added RPC fallback mechanism - Refactored faucet trigger utility - Updated to use identitySDK.navigateToFaceVerification() instead of fvRedirect()
After merging upstream/main, the contract address structure changed from contractAddresses to chainConfigs. Updated isAddressWhitelisted function to use the new chainConfigs structure.
…rt-for-the-identity-flow
- Remove excessive console.log statements from auth utilities and SDKs - Simplify error handling by removing redundant try-catch blocks - Remove verbose JSDoc comments that state the obvious - Consolidate duplicate navigation functions - Improve code readability and maintainability - Follow KISS, DRY, and clarity over cleverness principles
- Remove unnecessary console.log/console.error statements - Remove unused onVerificationSuccess prop from VerifyButton - Simplify error handling in demo components - Rename callBackExample to txConfirm for clarity - Follow DRY and KISS principles
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
Summary of the comments:
A proposed solution in a PR to resolve an issue should tackle only what is relevant to your solution and should not introduced extra refactors or changes without an explicit reason
| } catch (error) { | ||
| console.error("Verification failed:", error) | ||
| // Handle error (e.g., show toast) | ||
| } catch { |
There was a problem hiding this comment.
This refactor is both not needed or in any way related to your fixes for farcaster + its not true.
generateFVLink throws an error and is not handled there
| }, | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
If these are just placeholders and not working this should be clarified in a readme and should be configurable options for developers to pass down
(we are not hosting a farcaster app, developers integrating the identitysdk/claim-sdk might)
| } | ||
| } | ||
|
|
||
| export async function handleVerificationResponse( |
There was a problem hiding this comment.
Not being used anywhere, whats the purpose?
if not needed, remove
| } | ||
| } | ||
|
|
||
| export function handleVerificationResponseSync(url?: string): { |
There was a problem hiding this comment.
Not being used anywyere, if not needed, remove
| return universalLink; | ||
| } | ||
|
|
||
| export async function createVerificationCallbackUrl( |
There was a problem hiding this comment.
What is this supposed to be handling?
There was a problem hiding this comment.
It validates the URL and enforces a routing convention (appending /verify if missing) to ensure the callback hits a route handled by the SDK/app
| * @param authPeriod - The authentication period. | ||
| * @returns The identity expiry data. | ||
| */ | ||
| calculateIdentityExpiry( |
There was a problem hiding this comment.
Why got the comments removed?
There was a problem hiding this comment.
was cleaning up the codebase
| ); | ||
| params[popupMode ? "cbu" : "rdu"] = universalLinkCallback; | ||
| } else { | ||
| const callbackUrlWithParams = await createVerificationCallbackUrl(callbackUrl, { |
There was a problem hiding this comment.
What are these params used for, and why hardcode 'verified': true?
| if (!isWhitelisted) { | ||
| await this.fvRedirect() | ||
| // Use IdentitySDK's navigation method to eliminate code duplication | ||
| await this.identitySDK.navigateToFaceVerification( |
There was a problem hiding this comment.
- what duplication is being handled here exactly?
- if you choose to do a refactor, always make sure functionality does not change along the way
You introduce: hardcoded chain-id, different behavior of reloading page etc.
Its not necessarily a bad idea to move handling of face-verification to the identity-sdk.
but it should always be trippled checked against exisiting flows.
how would this affect dapps that already have this flow?
There was a problem hiding this comment.
- chainid should not be hardcoded to celo
- Flow should stay the same as it was, changes needed for your flow should only affect your intended flow
And include a demo that this change works as expected, as I am not sure this is the way to handle pop-up window (fallbackToNewTab? should it not just close the pop-up window?)
| return new IdentitySDK({ account, ...props }) | ||
| } | ||
|
|
||
| /** |
| console.error("submitAndWait Error:", error) | ||
| throw new Error(`Failed to submit transaction: ${error.message}`) | ||
| } | ||
| return waitForTransactionReceipt(this.publicClient, { hash }) |
There was a problem hiding this comment.
You are removing a valid catch error flow that could be used to give feedback to a user
- Restore original fvRedirect behavior in ClaimSDK (window.location.href) - Use fvDefaultChain instead of hardcoded chainId - Restore error handling in submitAndWait - Restore removed JSDoc comments - Remove unused handleVerificationResponse functions - Fix VerifyButton error handling - Add FarcasterAppConfigs documentation to README - Add JSDoc to createVerificationCallbackUrl
…direct, restore error handling)
3 participants
Description
This PR implements comprehensive Farcaster miniapp support for the identity verification flow, enabling seamless face verification within Farcaster miniapps with proper navigation and universal link handling. The implementation includes automatic miniapp detection, smart navigation, and robust response handling while fixing critical build system issues that were preventing successful compilation.
The changes address the need for better Farcaster miniapp integration by implementing the official
@farcaster/miniapp-sdk, adding universal link support for mobile/native compatibility, and creating utilities for handling post-verification flow continuation. Additionally, this PR resolves significant build configuration issues including Node.js dependency conflicts in browser builds and security vulnerabilities related to environment variable exposure.Dependencies added:
@farcaster/miniapp-sdk(peer dependency)About #10
Demo Video: Loom GoodSDK Forecaster Support
I would like to know how I can demo on a mobile please, but I was able to test the demo on a browser.
Checklist:
Key Implementation Details:
Farcaster Integration:
isInFarcasterMiniApp()- Async detection using official SDKnavigateToFaceVerification()- Smart navigation with automatic environment detectionopenUrlInFarcaster()- Official SDK integration withopenUrlmethodhandleVerificationResponse()- Generic utility for post-verification flowcreateUniversalLinkCallback()- Universal link support for mobile/native appsBuild System Fixes:
crypto,http,https,stream,zlib, etc.) for browser compatibilityprocess.envexposure with specific variablesSensitive Files Modified:
These are some of the files I modified after my implementation due to build error I was getting, because it was building for browser, meanwhile some Node.js functions are also present, I'd appreciate if there's another way I could resolve the error without modifying these files to avoid a merge conflict.
packages/citizen-sdk/src/index.ts,packages/citizen-sdk/src/utils/auth.tspackages/ui-components/tsup.config.claim.ts,packages/ui-components/package.jsonapps/demo-identity-app/vite.config.mts,apps/demo-webcomponents/vite.config.mtsapps/demo-identity-app/src/components/VerifyButton.tsxapps/demo-identity-app/src/globals.d.ts