feat(kyc): extract tiered KYC flow into clean branch#496
Open
onahprosper wants to merge 4 commits into
Open
Conversation
Port the tiered KYC API, UI, and migration changes onto latest main while preserving current main exports and dependency baseline to keep the PR isolated and reviewable. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughAdds KYC tiers, phone OTP (KudiSMS/Twilio), SmileID ID capture, Dojah utility‑bill verification, monthly swap‑limit dry‑run/enforcement, Supabase migrations/storage, server API routes, frontend KYC context and UI modals, helper libs, and env/docs updates. ChangesKYC Tier System & Phone Verification KYC Tier System & Swap Limits
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/SettingsDropdown.tsx (1)
73-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t show a successful copy flow for an empty address.
During wallet hydration,
walletAddresscan beundefinedhere. PassingwalletAddress ?? ""lets the UI copy a blank string and still open the warning modal.Suggested fix
const handleCopyAddress = async () => { - const ok = await copyToClipboard(walletAddress ?? "", "Address"); + if (!walletAddress) return; + const ok = await copyToClipboard(walletAddress, "Address"); if (!ok) return; setIsAddressCopied(true); setTimeout(() => setIsAddressCopied(false), 2000); setIsWarningModalOpen(true); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/SettingsDropdown.tsx` around lines 73 - 79, The copy flow in handleCopyAddress should not proceed when walletAddress is undefined or empty; before calling copyToClipboard (and before setting setIsAddressCopied or setIsWarningModalOpen), validate that walletAddress is a non-empty string and return early if not. Update the handleCopyAddress function to only call copyToClipboard(walletAddress, "Address") when walletAddress is truthy, and only set setIsAddressCopied(true) and setIsWarningModalOpen(true) after a successful copy; avoid passing "" to copyToClipboard and avoid opening the warning modal for an empty address.app/pages/TransactionPreview.tsx (1)
801-812:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the ref for the duplicate-save guard.
Line 801 reads
isSavingTransactionbefore the update from Line 802 is committed, so overlapping callers can still both pass this check and write the same transaction twice. That will duplicate history rows and double-count KYC usage.Suggested fix
}) => { - if (!activeWallet?.address || isSavingTransaction) return; + if (!activeWallet?.address || isSavingTransactionRef.current) return; + isSavingTransactionRef.current = true; setIsSavingTransaction(true); try { const accessToken = await getAccessToken(); @@ } catch (error) { console.error("Error saving transaction:", error); // Don't show error toast as this is a background operation } finally { + isSavingTransactionRef.current = false; setIsSavingTransaction(false); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/pages/TransactionPreview.tsx` around lines 801 - 812, The current duplicate-save race happens because the component checks the state boolean isSavingTransaction before calling setIsSavingTransaction(true), allowing concurrent callers to both proceed; update the guard to use a ref-backed flag (e.g., isSavingTransactionRef) in the TransactionPreview component: read and set isSavingTransactionRef.current synchronously to prevent concurrent entry, set it to true before starting the save flow and clear it in finally, and still keep setIsSavingTransaction(...) to update UI state so existing consumers work unchanged; modify the early-return to check isSavingTransactionRef.current (and activeWallet?.address) and ensure all places that set or read the save-guard use the ref to avoid duplicate saves.
🧹 Nitpick comments (12)
app/lib/countries.ts (1)
21-28: ⚡ Quick winAdd timeout for external API request.
The fetch to
restcountries.comhas no timeout. If the external API is slow or unresponsive, this could hang indefinitely. Other API clients in this PR (e.g.,dojah.ts,phone-verification.ts) correctly implement timeouts.Proposed fix
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + try { const response = await fetch( - 'https://restcountries.com/v3.1/all?fields=name,cca2,idd,flag' + 'https://restcountries.com/v3.1/all?fields=name,cca2,idd,flag', + { signal: controller.signal } ); + clearTimeout(timeoutId); if (!response.ok) { throw new Error('Failed to fetch countries'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/countries.ts` around lines 21 - 28, The fetch call in app/lib/countries.ts (the async fetch('https://restcountries.com/v3.1/all?fields=name,cca2,idd,flag') block) needs a timeout: create an AbortController, pass controller.signal to fetch, set a timer (e.g., setTimeout) that calls controller.abort() after the desired timeout, and clear the timer when fetch resolves or rejects; catch the abort error and rethrow a descriptive timeout Error (e.g., "Fetching countries timed out") so callers of the surrounding function can handle it. Ensure you add the controller and timer cleanup in the same function that performs the fetch.app/api/aggregator.ts (1)
974-1021: 💤 Low valueConsider adding explicit typing for the SmileID payload.
The
payloadparameter is typed asany, which loses type safety. Consider using a more specific type that matches the expected SmileID submission structure.+interface SmileIDPayload { + images: any[]; + partner_params: Record<string, unknown>; + id_info: { + country: string; + id_type: string; + id_number?: string; + }; + email?: string; +} + export const submitSmileIDData = async ( - payload: any, + payload: SmileIDPayload, accessToken: string, ): Promise<SmileIDSubmissionResponse> => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/aggregator.ts` around lines 974 - 1021, The function submitSmileIDData currently types its payload as any; define a concrete interface or type (e.g., SmileIDPayload or SmileIDSubmissionPayload) that models the expected SmileID submission fields and replace the payload parameter's type with that new type; update any related usages (e.g., axios.post generic type or response typings if needed) and ensure SmileIDSubmissionResponse stays aligned with the defined payload/response shape so the compiler enforces correct fields when calling submitSmileIDData.app/pages/TransactionForm.tsx (1)
1166-1167: 💤 Low valueClarify the targetTier logic for KYC modal.
The expression
tier === 2 ? 3 : 2assumes users only upgrade from Tier 2→3 or need Tier 2. Consider adding a comment or using a more explicit function to determine the next tier.- targetTier={tier === 2 ? 3 : 2} + // If already Tier 2, offer Tier 3 upgrade; otherwise start with Tier 2 + targetTier={tier >= 2 ? 3 : 2}Note: Using
>=would be more defensive if a Tier 3 user somehow opens this modal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/pages/TransactionForm.tsx` around lines 1166 - 1167, The ternary targetTier={tier === 2 ? 3 : 2} is brittle and unclear; replace it with a small, explicit helper (e.g., getNextTier(tier)) used where the KYC modal is rendered and add a one-line comment explaining intent; implement getNextTier to defensively return 3 if tier >= 2 (or otherwise 2) so Tier 3 users don’t get a lower/incorrect target, and update the prop to targetTier={getNextTier(tier)} to make the logic self-documenting and robust.app/components/KycModal.tsx (2)
851-889: ⚡ Quick winFile validation logic is duplicated between client and server.
The allowed extensions and max file size are defined both here (lines 851-852) and in the route handler (lines 15-21 of tier3-verify/route.ts). Consider extracting these constants to a shared location to keep them in sync.
// app/lib/kyc-constants.ts export const TIER3_ALLOWED_EXTENSIONS = ["JPG", "JPEG", "PNG", "PDF"]; export const TIER3_MAX_FILE_BYTES = 5 * 1024 * 1024;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/KycModal.tsx` around lines 851 - 889, ALLOWED_TIER3_EXTENSIONS and TIER3_MAX_BYTES are duplicated client/server; extract them into a shared module (e.g., export constants from a new kyc-constants file) and import them into this component and the tier3-verify route so both use the same source of truth; update references in renderTier3Upload (handleTier3FileChange and handleTier3Drop) to use the imported TIER3_ALLOWED_EXTENSIONS / TIER3_MAX_FILE_BYTES names and remove the local definitions.
9-16: 💤 Low valueClarify purpose of Window.tf interface extension.
The
Window.tfinterface withloadandreloadmethods is declared but its purpose isn't clear from the code. If this is for a specific library (TensorFlow.js?), add a comment. If unused, remove it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/KycModal.tsx` around lines 9 - 16, The Window.tf interface extension (Window.tf with load and reload methods) in KycModal.tsx lacks context; either add a one-line comment above the declaration clarifying which library it represents (e.g., TensorFlow.js or another 3rd-party lib) and why load/reload are needed, or remove the entire declare global block if those properties are not used anywhere; reference the Window.tf, load, and reload symbols when making the change so the declaration matches actual usage (or is removed) to avoid unused global typings.app/api/v1/transactions/route.ts (1)
182-199: 💤 Low valueRate fetch error handling could log for observability.
The empty catch block at line 197 silently swallows rate fetch errors. While this is intentional (the RPC handles rate unavailability), consider logging for observability:
} catch { - // Rate unavailable — stored procedure will return rate_unavailable if cNGN is involved + // Rate unavailable — stored procedure will return rate_unavailable if cNGN is involved + console.warn("[transactions] Failed to fetch cNGN rate, proceeding with rate=0"); }This helps with debugging when rate issues occur in production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/transactions/route.ts` around lines 182 - 199, The empty catch swallowing errors around the cNGN rate fetch (the block that sets cngnToUsdRate using aggregatorUrl and AbortSignal.timeout) reduces observability; update the catch to log the error and context (e.g., include aggregatorUrl, that this was the USDC→NGN rate fetch, and the thrown error) using the project's logger (processLogger if available) or console.error so failures are recorded while still allowing the stored procedure to handle rate_unavailable.app/hooks/useSwapButton.ts (1)
134-160: 💤 Low valueButton action routing logic looks correct but parameter naming could be clearer.
The
setIsLimitModalOpenparameter at line 138 is actually used for phone verification gating (line 155), which might be confusing. The TransactionLimitModal appears to handle phone verification flow internally.Consider renaming for clarity if this is intentional, or verify the modal does handle phone verification:
const buttonAction = ( handleSwap: () => void, login: () => void, handleFundWallet: () => void, - setIsLimitModalOpen: () => void, + openVerificationModal: () => void, // Opens limit modal which handles phone verification isPhoneVerified: boolean, setIsKycModalOpen: () => void,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useSwapButton.ts` around lines 134 - 160, The parameter name setIsLimitModalOpen in the buttonAction function is misleading because it's used to gate phone verification when !isPhoneVerified; either rename the parameter to setIsPhoneVerificationModalOpen (and update all callers) or confirm TransactionLimitModal handles phone verification and rename to something like openPhoneVerificationFlow; update the function signature in useSwapButton and any places that pass that callback to match the new name (refer to buttonAction, setIsLimitModalOpen, isPhoneVerified and TransactionLimitModal to locate the related code).app/api/kyc/smile-id/route.ts (2)
245-249: 💤 Low valueUnhandled error from attempt counter reset could mask success.
The
attemptsreset on Line 246-249 doesn't check for errors. While unlikely to fail, if it does, the user received a success response but their attempts weren't reset, which could cause issues on subsequent verifications.Consider logging or handling the error:
Proposed fix
// Reset attempt counter on success - await supabaseAdmin + const { error: resetError } = await supabaseAdmin .from("user_kyc_profiles") .update({ attempts: 0 }) .eq("wallet_address", walletAddress); + + if (resetError) { + console.error("Failed to reset KYC attempts counter:", resetError); + // Don't fail the request - verification was successful + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/kyc/smile-id/route.ts` around lines 245 - 249, The attempts reset using supabaseAdmin.from("user_kyc_profiles").update({ attempts: 0 }).eq("wallet_address", walletAddress) is unhandled and may fail silently; update the code in the route handler to await the Supabase response, check for errors on the returned object (error or status fields), and handle them—at minimum log the failure with context (walletAddress, error) using your logger and, if appropriate, raise or return an internal error so the client is aware; ensure the change references the same supabaseAdmin call and the user_kyc_profiles attempts update.
233-243: 💤 Low valueRedundant check after update — profile existence already verified.
The check at Lines 234-243 for "No KYC profile exists" is defensive but logically unreachable since you already fetched and validated
existingProfileexists at Lines 70-79. If the row disappears mid-request, this catches it, but the error message is misleading (it's not about missing phone verification at this point).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/kyc/smile-id/route.ts` around lines 233 - 243, The defensive check for updatedProfile after you already validated existingProfile is misleading; either remove the redundant block or, if you want to keep the safety check, change the error to reflect an update failure (not missing phone verification). Update the code around updatedProfile so that when updatedProfile is falsy you return a 500 (or 409) with a message like "Failed to update KYC profile; please retry" instead of the phone-verification message, and ensure the check references updatedProfile (and not existingProfile) to make its intent clear..env.example (1)
144-148: 💤 Low valueMinor formatting issues flagged by linter.
Static analysis flagged several formatting concerns:
- Line 144:
SMILE_IDENTITY_BASE_URL=XXXXXX#getfrom docs— value needs quotes or cleaner placeholder- Line 147:
SMILE_ID_CALLBACK_URL=#optional`` — spaces around=- Line 148: Comment after value
These don't affect functionality but cleaning them up improves consistency:
Proposed fix
-SMILE_IDENTITY_BASE_URL=XXXXXX `#get` from docs +# Get base URL from SmileID documentation +SMILE_IDENTITY_BASE_URL=https://api.smileidentity.com SMILE_IDENTITY_API_KEY=your_api_key_here SMILE_IDENTITY_PARTNER_ID=your_partner_id_here -SMILE_ID_CALLBACK_URL= `#optional` callback url -SMILE_IDENTITY_SERVER=0 # 0 for sandbox, 1 for production +# Optional callback URL for async verification results +SMILE_ID_CALLBACK_URL= +# 0 for sandbox, 1 for production +SMILE_IDENTITY_SERVER=0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 144 - 148, Environment variable entries have minor formatting issues: replace the bare placeholder for SMILE_IDENTITY_BASE_URL with a quoted or cleaner placeholder (e.g. "https://example.com"), remove extra spaces around the equals sign for SMILE_ID_CALLBACK_URL (make it SMILE_ID_CALLBACK_URL=), and move or separate inline comments from values (for example put the explanatory text for SMILE_IDENTITY_SERVER on its own comment line above or use a single-space inline comment) so variable assignments are clean and consistent; update SMILE_IDENTITY_BASE_URL, SMILE_ID_CALLBACK_URL, and SMILE_IDENTITY_SERVER accordingly.app/api/phone/verify-otp/route.ts (1)
17-19: ⚡ Quick winPlain SHA-256 for OTP hashing is acceptable but consider timing-safe comparison.
Using SHA-256 to hash the OTP before storage is reasonable for short-lived codes. However, the comparison at Line 206 uses
!==which is not timing-safe. For OTPs with limited attempts, this is low risk, but a timing-safe comparison would be more robust.Proposed fix using timingSafeEqual
+import { createHash, timingSafeEqual } from "crypto"; function hashOTP(otp: string): string { return createHash("sha256").update(otp).digest("hex"); } +function compareOTPHash(submittedOTP: string, storedHash: string): boolean { + const submittedHash = hashOTP(submittedOTP); + try { + return timingSafeEqual(Buffer.from(submittedHash), Buffer.from(storedHash)); + } catch { + return false; + } +}Then at Line 206:
- if (verification.otp_code !== hashOTP(otpCode)) { + if (!compareOTPHash(otpCode, verification.otp_code)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/phone/verify-otp/route.ts` around lines 17 - 19, The current hashOTP(otp: string) returns a hex SHA-256 string but the verify path performs a plain !== comparison; replace that with a timing-safe comparison: when verifying in the OTP verification handler (the code that compares storedHash and candidate hash), convert both hex hashes to Buffer (or derive Buffers directly from createHash), ensure equal length then use crypto.timingSafeEqual to compare; if lengths differ, treat as non-equal (do not throw), and log/return the same failure flow as before. Ensure you reference hashOTP for the hashing step and the verify routine where storedHash is compared to the candidate.supabase/migrations/create_user_kyc_profiles.sql (1)
256-257: ⚡ Quick winExplicitly revoke PUBLIC execute permission on SECURITY DEFINER function.
In PostgreSQL, functions are executable by PUBLIC by default. When you
GRANT EXECUTEto a specific role, PUBLIC permissions remain intact unless explicitly revoked. AddREVOKE EXECUTE ... FROM PUBLICbefore the GRANT to follow security best practices:Proposed addition
-- Only the service_role backend may call this function +REVOKE EXECUTE ON FUNCTION public.insert_swap_transaction_if_within_limit FROM PUBLIC; GRANT EXECUTE ON FUNCTION public.insert_swap_transaction_if_within_limit TO service_role;Note: The same pattern exists in
supabase/migrations/create_transactions_table.sqlfor theset_current_wallet_addressfunction and should also have an explicit REVOKE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/create_user_kyc_profiles.sql` around lines 256 - 257, The SECURITY DEFINER function insert_swap_transaction_if_within_limit currently grants EXECUTE to service_role but leaves PUBLIC execute permission intact; before the GRANT, add an explicit REVOKE EXECUTE ON FUNCTION public.insert_swap_transaction_if_within_limit FROM PUBLIC to remove default public access, then re-apply GRANT EXECUTE TO service_role; apply the same pattern for the set_current_wallet_address function in create_transactions_table.sql (REVOKE FROM PUBLIC then GRANT to the intended role).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/kyc/status/route.ts`:
- Around line 53-54: rawTier can be NaN if kycProfile?.tier is non-numeric;
before clamping in the calculation for tier, explicitly validate rawTier (e.g.,
with Number.isFinite(rawTier) or !Number.isNaN(rawTier)) and fallback to 0 when
invalid, then apply the Math.min/Math.max clamp and cast to 0|1|2|3 so tier
never becomes NaN/null; update the logic around the rawTier and tier variables
to perform that check and fallback.
In `@app/api/kyc/tier3-verify/route.ts`:
- Around line 285-318: The separate reset of attempts (the second
supabaseAdmin.update on "user_kyc_profiles") must be merged into the main update
so both the profile changes and attempts: 0 are applied atomically; modify the
updatePayload used in the first call (the one that updates "user_kyc_profiles"
where .eq("wallet_address", walletAddress)) to include attempts: 0, remove the
subsequent standalone update block that targets attempts, and keep the existing
error/cleanup handling (removing from KYC_BUCKET and returning errors) tied to
the single update call to ensure a single success/failure path for profile
update and attempt reset.
In `@app/api/phone/verify-otp/route.ts`:
- Around line 188-193: The expiry check can throw or behave incorrectly if
verification.expires_at is null/invalid; update the block that checks expiry
(where verification is used in route.ts) to first validate expires_at: if it's
missing or new Date(verification.expires_at) is invalid (isNaN(date.getTime())),
treat the OTP as expired and return the same NextResponse.json({ success: false,
error: "OTP has expired. Please request a new one." }, { status: 400 });
otherwise perform the existing date comparison. This ensures defensive handling
for null/invalid expires_at values.
In `@app/components/PhoneVerificationModal.tsx`:
- Around line 164-170: The OTP fetches use getAccessToken() without verifying
the Privy auth state or the token value; update the PhoneVerificationModal
handlers (e.g., the functions that call getAccessToken() before calling
/api/phone/send-otp and /api/phone/verify-otp) to first check usePrivy()'s ready
and authenticated flags and only call getAccessToken() when authenticated, then
validate that the returned accessToken is a non-empty string before constructing
Authorization: Bearer ${accessToken}; if not authenticated or token is falsy,
short-circuit the handler to show a re-authentication prompt/tooltip or return
an error rather than sending a request with "Bearer null"/"Bearer undefined".
In `@app/components/SettingsDropdown.tsx`:
- Around line 64-71: The logout cleanup uses user?.wallet?.address which can
differ from the resolved active address (walletAddress), so update the logout
call to pass the resolved walletAddress (fallback to user?.wallet?.address if
walletAddress is null/undefined) when invoking clearUserSessionData;
specifically replace the current second argument with walletAddress ??
user?.wallet?.address so clearUserSessionData(user?.id, walletAddress ??
user?.wallet?.address) is used (ensure walletAddress from the existing
resolution logic is in scope where clearUserSessionData is called).
In `@app/components/TransactionLimitModal.tsx`:
- Around line 152-165: The "Learn more." span in TransactionLimitModal is styled
as interactive but has no click or keyboard handlers; change it to an accessible
control or plain text: either replace the span with a semantic interactive
element (e.g., a button or anchor) inside the TransactionLimitModal component
and wire its onClick/onKeyDown to the existing "learn more" handler (or open the
relevant modal/router link), ensure keyboard focusability and ARIA label as
needed, or convert it to non-interactive plain text (remove interactive styling)
so there is no dead affordance.
In `@app/context/KYCContext.tsx`:
- Around line 140-160: The transaction-summary fetch should not reset
monthlySpent to zero on failure or mark the whole KYC data as fresh when only
the KYC-status succeeded; update the fetch handler around
fetch(`/api/kyc/transaction-summary`) and setTransactionSummary so that on
non-ok or catch you do not call setTransactionSummary with the zeroed object and
instead return a failure signal (e.g., false or undefined) so the caller using
Promise.allSettled can detect which request failed; then adjust the combined
Promise.allSettled logic to apply partial results (apply transactionSummary only
when the transaction-summary promise fulfilled) and only mark the aggregated KYC
data as fresh when all required pieces, including the transaction summary,
succeeded.
- Around line 151-155: The fetch handlers (the ones that call
setTransactionSummary and setTier after awaits) can overwrite state for a
different wallet; capture the current walletAddress into a local variable at the
start of each async fetch (e.g., in the functions that call
setTransactionSummary and setTier) and after each await verify it still matches
the latest walletAddress before calling setState; additionally, ensure the
disconnect path clears KYC context by running the existing state-clearing logic
whenever walletAddress becomes falsy (not only when truthy) so previous account
data is removed on disconnect.
In `@app/lib/dojah.ts`:
- Around line 90-95: The current code calls res.json() unguarded so a non-JSON
response will throw before you check res.ok; change the logic to first read the
response safely (e.g., await res.text()), attempt JSON.parse in a try/catch to
populate data (fall back to raw text if parse fails), then perform the res.ok
check and construct the message using parsed data?.message || parsed
data?.entity?.result?.message || rawText || res.statusText; ensure you still
throw a meaningful Error when !res.ok and that variables referenced (res,
res.json(), data, and message) are updated to use the safe parse/fallback
approach.
In `@app/lib/kyc-tier-limits.ts`:
- Around line 12-21: The parseNonNegativeNumber function currently reads
process.env[envKey] dynamically which prevents Next.js from inlining
NEXT_PUBLIC_* values for client code; change parseNonNegativeNumber to accept
the raw string value (e.g., value: string | undefined, fallback: number) and
keep its logic to return fallback for undefined/empty/non-finite/negative
inputs, then update callers such as getKycMonthlyLimitsRecord and any usage in
KYCContext.tsx to pass static references like process.env.NEXT_PUBLIC_<NAME>
(not dynamic indexing) so Next.js can inline the public env values at build
time.
In `@app/lib/smileID.ts`:
- Around line 29-121: The WebApi constructor is being called with serverUrl (a
full URL) but it expects sid_server mode ('0' for sandbox, '1' for production);
in submitSmileIDJob replace the fourth argument in new WebApiClass(...) with a
mode value (e.g. derive const sidServerMode =
process.env.SMILE_IDENTITY_SERVER_MODE ?? '0' or map
process.env.SMILE_IDENTITY_SERVER to '0'/'1' based on environment) and pass
sidServerMode instead of serverUrl; locate this in submitSmileIDJob around the
getSIDWebAPI()/WebApiClass instantiation and update the env var usage
accordingly.
In `@supabase/migrations/create_transactions_table.sql`:
- Around line 36-38: Add a uniqueness guard to prevent duplicate transaction
rows by creating a unique constraint/index on the transactions table for the
combination of order_id and tx_hash; update the migration that currently creates
idx_transactions_wallet_address and idx_transactions_created_at to also create a
UNIQUE INDEX (or ALTER TABLE ADD CONSTRAINT UNIQUE) named something like
idx_transactions_order_txhash_unique on transactions(order_id, tx_hash) so
repeated saves for the same order_id/tx_hash are rejected.
---
Outside diff comments:
In `@app/components/SettingsDropdown.tsx`:
- Around line 73-79: The copy flow in handleCopyAddress should not proceed when
walletAddress is undefined or empty; before calling copyToClipboard (and before
setting setIsAddressCopied or setIsWarningModalOpen), validate that
walletAddress is a non-empty string and return early if not. Update the
handleCopyAddress function to only call copyToClipboard(walletAddress,
"Address") when walletAddress is truthy, and only set setIsAddressCopied(true)
and setIsWarningModalOpen(true) after a successful copy; avoid passing "" to
copyToClipboard and avoid opening the warning modal for an empty address.
In `@app/pages/TransactionPreview.tsx`:
- Around line 801-812: The current duplicate-save race happens because the
component checks the state boolean isSavingTransaction before calling
setIsSavingTransaction(true), allowing concurrent callers to both proceed;
update the guard to use a ref-backed flag (e.g., isSavingTransactionRef) in the
TransactionPreview component: read and set isSavingTransactionRef.current
synchronously to prevent concurrent entry, set it to true before starting the
save flow and clear it in finally, and still keep setIsSavingTransaction(...) to
update UI state so existing consumers work unchanged; modify the early-return to
check isSavingTransactionRef.current (and activeWallet?.address) and ensure all
places that set or read the save-guard use the ref to avoid duplicate saves.
---
Nitpick comments:
In @.env.example:
- Around line 144-148: Environment variable entries have minor formatting
issues: replace the bare placeholder for SMILE_IDENTITY_BASE_URL with a quoted
or cleaner placeholder (e.g. "https://example.com"), remove extra spaces around
the equals sign for SMILE_ID_CALLBACK_URL (make it SMILE_ID_CALLBACK_URL=), and
move or separate inline comments from values (for example put the explanatory
text for SMILE_IDENTITY_SERVER on its own comment line above or use a
single-space inline comment) so variable assignments are clean and consistent;
update SMILE_IDENTITY_BASE_URL, SMILE_ID_CALLBACK_URL, and SMILE_IDENTITY_SERVER
accordingly.
In `@app/api/aggregator.ts`:
- Around line 974-1021: The function submitSmileIDData currently types its
payload as any; define a concrete interface or type (e.g., SmileIDPayload or
SmileIDSubmissionPayload) that models the expected SmileID submission fields and
replace the payload parameter's type with that new type; update any related
usages (e.g., axios.post generic type or response typings if needed) and ensure
SmileIDSubmissionResponse stays aligned with the defined payload/response shape
so the compiler enforces correct fields when calling submitSmileIDData.
In `@app/api/kyc/smile-id/route.ts`:
- Around line 245-249: The attempts reset using
supabaseAdmin.from("user_kyc_profiles").update({ attempts: 0
}).eq("wallet_address", walletAddress) is unhandled and may fail silently;
update the code in the route handler to await the Supabase response, check for
errors on the returned object (error or status fields), and handle them—at
minimum log the failure with context (walletAddress, error) using your logger
and, if appropriate, raise or return an internal error so the client is aware;
ensure the change references the same supabaseAdmin call and the
user_kyc_profiles attempts update.
- Around line 233-243: The defensive check for updatedProfile after you already
validated existingProfile is misleading; either remove the redundant block or,
if you want to keep the safety check, change the error to reflect an update
failure (not missing phone verification). Update the code around updatedProfile
so that when updatedProfile is falsy you return a 500 (or 409) with a message
like "Failed to update KYC profile; please retry" instead of the
phone-verification message, and ensure the check references updatedProfile (and
not existingProfile) to make its intent clear.
In `@app/api/phone/verify-otp/route.ts`:
- Around line 17-19: The current hashOTP(otp: string) returns a hex SHA-256
string but the verify path performs a plain !== comparison; replace that with a
timing-safe comparison: when verifying in the OTP verification handler (the code
that compares storedHash and candidate hash), convert both hex hashes to Buffer
(or derive Buffers directly from createHash), ensure equal length then use
crypto.timingSafeEqual to compare; if lengths differ, treat as non-equal (do not
throw), and log/return the same failure flow as before. Ensure you reference
hashOTP for the hashing step and the verify routine where storedHash is compared
to the candidate.
In `@app/api/v1/transactions/route.ts`:
- Around line 182-199: The empty catch swallowing errors around the cNGN rate
fetch (the block that sets cngnToUsdRate using aggregatorUrl and
AbortSignal.timeout) reduces observability; update the catch to log the error
and context (e.g., include aggregatorUrl, that this was the USDC→NGN rate fetch,
and the thrown error) using the project's logger (processLogger if available) or
console.error so failures are recorded while still allowing the stored procedure
to handle rate_unavailable.
In `@app/components/KycModal.tsx`:
- Around line 851-889: ALLOWED_TIER3_EXTENSIONS and TIER3_MAX_BYTES are
duplicated client/server; extract them into a shared module (e.g., export
constants from a new kyc-constants file) and import them into this component and
the tier3-verify route so both use the same source of truth; update references
in renderTier3Upload (handleTier3FileChange and handleTier3Drop) to use the
imported TIER3_ALLOWED_EXTENSIONS / TIER3_MAX_FILE_BYTES names and remove the
local definitions.
- Around line 9-16: The Window.tf interface extension (Window.tf with load and
reload methods) in KycModal.tsx lacks context; either add a one-line comment
above the declaration clarifying which library it represents (e.g.,
TensorFlow.js or another 3rd-party lib) and why load/reload are needed, or
remove the entire declare global block if those properties are not used
anywhere; reference the Window.tf, load, and reload symbols when making the
change so the declaration matches actual usage (or is removed) to avoid unused
global typings.
In `@app/hooks/useSwapButton.ts`:
- Around line 134-160: The parameter name setIsLimitModalOpen in the
buttonAction function is misleading because it's used to gate phone verification
when !isPhoneVerified; either rename the parameter to
setIsPhoneVerificationModalOpen (and update all callers) or confirm
TransactionLimitModal handles phone verification and rename to something like
openPhoneVerificationFlow; update the function signature in useSwapButton and
any places that pass that callback to match the new name (refer to buttonAction,
setIsLimitModalOpen, isPhoneVerified and TransactionLimitModal to locate the
related code).
In `@app/lib/countries.ts`:
- Around line 21-28: The fetch call in app/lib/countries.ts (the async
fetch('https://restcountries.com/v3.1/all?fields=name,cca2,idd,flag') block)
needs a timeout: create an AbortController, pass controller.signal to fetch, set
a timer (e.g., setTimeout) that calls controller.abort() after the desired
timeout, and clear the timer when fetch resolves or rejects; catch the abort
error and rethrow a descriptive timeout Error (e.g., "Fetching countries timed
out") so callers of the surrounding function can handle it. Ensure you add the
controller and timer cleanup in the same function that performs the fetch.
In `@app/pages/TransactionForm.tsx`:
- Around line 1166-1167: The ternary targetTier={tier === 2 ? 3 : 2} is brittle
and unclear; replace it with a small, explicit helper (e.g., getNextTier(tier))
used where the KYC modal is rendered and add a one-line comment explaining
intent; implement getNextTier to defensively return 3 if tier >= 2 (or otherwise
2) so Tier 3 users don’t get a lower/incorrect target, and update the prop to
targetTier={getNextTier(tier)} to make the logic self-documenting and robust.
In `@supabase/migrations/create_user_kyc_profiles.sql`:
- Around line 256-257: The SECURITY DEFINER function
insert_swap_transaction_if_within_limit currently grants EXECUTE to service_role
but leaves PUBLIC execute permission intact; before the GRANT, add an explicit
REVOKE EXECUTE ON FUNCTION public.insert_swap_transaction_if_within_limit FROM
PUBLIC to remove default public access, then re-apply GRANT EXECUTE TO
service_role; apply the same pattern for the set_current_wallet_address function
in create_transactions_table.sql (REVOKE FROM PUBLIC then GRANT to the intended
role).
🪄 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: b41b4bbb-e52a-4c07-ae6e-89e68166a8aa
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/icons/placeholder.pngis excluded by!**/*.png
📒 Files selected for processing (37)
.env.exampleapp/api/aggregator.tsapp/api/kyc/smile-id/id_types.jsonapp/api/kyc/smile-id/route.tsapp/api/kyc/status/route.tsapp/api/kyc/tier3-verify/route.tsapp/api/kyc/transaction-summary/route.tsapp/api/phone/send-otp/route.tsapp/api/phone/verify-otp/route.tsapp/api/v1/transactions/route.tsapp/components/KycModal.tsxapp/components/PhoneVerificationModal.tsxapp/components/ProfileDrawer.tsxapp/components/SettingsDropdown.tsxapp/components/TransactionLimitModal.tsxapp/components/index.tsapp/components/kyc/DocumentRequirementsModal.tsxapp/components/kyc/index.tsapp/context/KYCContext.tsxapp/context/index.tsapp/hooks/useSwapButton.tsapp/lib/countries.tsapp/lib/dojah.tsapp/lib/kyc-tier-limits.tsapp/lib/phone-verification.tsapp/lib/smileID.tsapp/lib/supabase.tsapp/pages/TransactionForm.tsxapp/pages/TransactionPreview.tsxapp/providers.tsxapp/types.tsapp/types/smart-camera-web.d.tsdocs/environment-variables.mdmiddleware.tspackage.jsonsupabase/migrations/create_transactions_table.sqlsupabase/migrations/create_user_kyc_profiles.sql
- Defer Supabase client creation until first use so next build does not require secrets at bundle time.\n- Restore HomeTransactionFormModeProvider and StarknetProvider around KYC context tree.\n- Add https-proxy-agent for Twilio server external resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/supabase.ts`:
- Line 1: Add a top-level import of "server-only" to this module so it cannot be
accidentally bundled into client code; specifically, insert import "server-only"
as the first statement in the file that exports the Supabase client (the module
that uses SUPABASE_SERVICE_ROLE_KEY and createClient/SupabaseClient) to enforce
compile-time server-only usage.
🪄 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: c201083f-6df2-4e38-86e2-1f5596466cab
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
app/lib/supabase.tsapp/providers.tsxpackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
- Revised .env.example to clarify the use of Supabase secret keys and added comments for better guidance. - Updated jest.setup.js to use the new SUPABASE_SECRET_KEY for testing. - Enhanced KYC API routes to improve error handling and wallet address verification. - Introduced new utility functions for KYC tier management and refined transaction handling logic. - Improved user feedback in KYC and phone verification modals for better clarity and user experience.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.env.example (1)
109-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate
BREVO_LIST_IDentry.Line 110 duplicates line 109. This appears to be a copy-paste error.
BREVO_LIST_ID= -BREVO_LIST_ID=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 109 - 110, Remove the duplicate environment variable entry: keep a single BREVO_LIST_ID declaration in the .env.example file and delete the extra duplicate line so there is only one BREVO_LIST_ID key present (verify surrounding spacing/newline remains consistent).docs/transaction-history-setup.md (1)
52-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation SQL schema differs from actual migration.
The SQL in this guide differs from
supabase/migrations/20260508140001_create_transactions_table.sql:
- Doc shows
order_id TEXT NOT NULLandnetwork TEXT NOT NULL, but migration has them nullable- Doc is missing
CHECKconstraints ontransaction_typeandstatus- Doc is missing the unique index
idx_transactions_order_txhash_unique- Doc is missing
email_sent_at,explorer_linkcolumnsConsider updating this guide to reference the migration file directly, or sync the schema definitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/transaction-history-setup.md` around lines 52 - 97, Update the docs SQL to match the actual migration `20260508140001_create_transactions_table.sql`: make order_id and network nullable (remove NOT NULL), add the CHECK constraints for transaction_type and status (same enum/values used in the migration), add the unique index idx_transactions_order_txhash_unique, and add the missing columns email, email_sent_at, and explorer_link with the same types and defaults as in the migration; alternatively replace the inline SQL with a clear reference/link to the migration file `supabase/migrations/20260508140001_create_transactions_table.sql` so the guide always reflects the canonical schema.
♻️ Duplicate comments (1)
app/context/KYCContext.tsx (1)
254-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale KYC data persists after wallet disconnect.
When
walletAddressbecomesundefined(user disconnects), this effect does nothing because of theif (walletAddress)guard. The previous wallet's tier, phone verification status, and transaction summary remain in context state.🛡️ Proposed fix to clear state on disconnect
useEffect(() => { if (walletAddress) { setTier(0); setIsPhoneVerified(false); setPhoneNumber(null); setTransactionSummary({ dailySpent: 0, monthlySpent: 0, lastTransactionDate: null }); fetchGuardsRef.current = {}; lastFetchTimeRef.current = 0; refreshStatus(true); + } else { + // Clear state on disconnect to prevent leaking previous wallet's KYC data + setTier(0); + setIsPhoneVerified(false); + setPhoneNumber(null); + setTransactionSummary({ dailySpent: 0, monthlySpent: 0, lastTransactionDate: null }); + fetchGuardsRef.current = {}; + lastFetchTimeRef.current = 0; } }, [walletAddress, refreshStatus]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/context/KYCContext.tsx` around lines 254 - 264, The effect watching walletAddress currently only resets KYC state when walletAddress is truthy, leaving stale data when the user disconnects; update the useEffect (the effect that references walletAddress and refreshStatus) to handle both cases: when walletAddress is present keep the existing reset-and-refresh logic (calling setTier(0), setIsPhoneVerified(false), setPhoneNumber(null), setTransactionSummary(...), clearing fetchGuardsRef.current and lastFetchTimeRef.current, then calling refreshStatus(true)), and add an else branch that explicitly clears all KYC-related state (setTier(null or 0 as appropriate), setIsPhoneVerified(false), setPhoneNumber(null), setTransactionSummary to empty/zeroed values, and clear fetchGuardsRef.current and lastFetchTimeRef.current) so state is cleared on disconnect.
🧹 Nitpick comments (4)
.env.example (1)
137-141: 💤 Low valueInline comments may cause issues with some env parsers.
Some
.envparsers don't handle inline comments well (e.g.,SMILE_IDENTITY_SERVER=0 # 0 for sandbox). Consider moving comments to separate lines or removing them from value lines for maximum compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 137 - 141, The inline comments on environment variables (e.g., SMILE_IDENTITY_BASE_URL, SMILE_IDENTITY_API_KEY, SMILE_IDENTITY_PARTNER_ID, SMILE_ID_CALLBACK_URL, SMILE_IDENTITY_SERVER) can break some .env parsers; update the .env.example by removing end-of-line comments from assignment lines and instead place explanatory comments on their own preceding lines (or delete them), ensuring each variable line contains only KEY=VALUE so parsers read values reliably.app/api/v1/transactions/[id]/route.ts (1)
23-38: 💤 Low valueConsider distinguishing database errors from not-found for logging.
The current code treats any
fetchErrorthe same as a missing transaction, which may obscure actual database connectivity issues in logs. For observability, consider logging the actual error type before returning the generic 404.Suggested improvement
if (fetchError || !existingTransaction) { + if (fetchError) { + console.error("Database error fetching transaction:", fetchError); + } trackApiError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/transactions/`[id]/route.ts around lines 23 - 38, Update the fetch block to distinguish a real database error from a not-found result: after the supabaseAdmin.from(...).select(...).eq(...).single() call, if fetchError is truthy call trackApiError(request, `/api/v1/transactions/${id}`, "PUT", fetchError, 500) (or include fetchError message/details) and return a 500 response; only when fetchError is falsy but existingTransaction is null/undefined call trackApiError(..., new Error("Transaction not found or unauthorized"), 404) and return the 404 response. Ensure you reference the existingTransaction and fetchError variables and reuse the same response bodies but change status codes and logged error details accordingly.app/lib/privy.ts (1)
86-104: 💤 Low valueConsider consistent error handling for missing Privy user.
The sibling functions (
getWalletAddressFromPrivyUserId,getSmartWalletAddressFromPrivyUserId) throw explicit errors when the user or linked accounts are missing. This function silently returns an empty array ifuseris null/undefined. If this is intentional (to allow graceful degradation), the docstring should clarify. Otherwise, consider throwing for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/privy.ts` around lines 86 - 104, collectLinkedEvmAddressesForPrivyUserId currently returns an empty array when privy.getUser(userId) is null/undefined, which is inconsistent with getWalletAddressFromPrivyUserId and getSmartWalletAddressFromPrivyUserId that throw on missing user/linked accounts; update collectLinkedEvmAddressesForPrivyUserId to perform the same null checks on user and user.linkedAccounts and throw the same error (or a matching Error message) as the other two functions instead of silently returning [], ensuring you still collect and validate addresses with EVM_ADDRESS_LOWER after confirming user existence via getPrivyClient()/privy.getUser.app/hooks/useSwapButton.ts (1)
137-160: ⚡ Quick winUse one
isUserVerifiedsource of truth.
isEnabledandbuttonTextread the hook prop, butbuttonActionasks the caller to passisUserVerifiedagain. That duplication can desync the rendered CTA from the click behavior and open the wrong modal if the two values ever diverge.Suggested fix
const buttonAction = ( handleSwap: () => void, login: () => void, handleFundWallet: () => void, setIsLimitModalOpen: () => void, isPhoneVerified: boolean, setIsKycModalOpen: () => void, - isUserVerified: boolean, openMigrationModal?: () => void, ) => { if (needsMigration && authenticated && !isInjectedWallet && openMigrationModal) { return openMigrationModal; } if (!authenticated && !isInjectedWallet) { return login; } if (hasInsufficientBalance && !isInjectedWallet && authenticated) { return handleFundWallet; } if (!hasInsufficientBalance && !isUserVerified && (authenticated || isInjectedWallet)) { if (!isPhoneVerified) { return setIsLimitModalOpen; } return setIsKycModalOpen; } return handleSwap; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useSwapButton.ts` around lines 137 - 160, The buttonAction function duplicates the isUserVerified flag by accepting it as a parameter while the hook already has a single source of truth for isUserVerified; remove the isUserVerified parameter from buttonAction's signature and use the hook-scoped isUserVerified value instead, update any call sites that pass isUserVerified to stop passing it, and ensure related conditionals inside buttonAction (the branch that returns setIsKycModalOpen or setIsLimitModalOpen) reference the hook-scoped isUserVerified variable so the rendered CTA and click behavior stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/v1/transactions/route.ts`:
- Around line 211-215: The KYC lookup is using walletAddress (header value) but
the transaction insert uses normalizedBodyWalletAddress, allowing mismatched
checks; update the code so both the KYC query and subsequent limit
check/transaction insert use the same wallet variable—either change the
supabaseAdmin.from("user_kyc_profiles").select(...).eq("wallet_address",
walletAddress) call to use normalizedBodyWalletAddress or ensure later insert
uses walletAddress consistently; ensure references to kycProfile and kycError
remain tied to the chosen wallet variable and adjust any downstream logic (limit
checks and the insert call that currently uses normalizedBodyWalletAddress) to
use the same identifier.
In `@app/hooks/useSwapButton.ts`:
- Around line 141-149: The migration click handler must be required so the
migration gate always intercepts clicks: change the parameter openMigrationModal
from optional to required in the useSwapButton signature (remove the ? so
openMigrationModal: () => void) and in the branch that currently checks
needsMigration && authenticated && !isInjectedWallet remove the runtime optional
check and always return openMigrationModal; update any callers to pass a
migration handler accordingly (refer to symbols: needsMigration, authenticated,
isInjectedWallet, openMigrationModal, and the useSwapButton function).
In `@app/pages/TransactionPreview.tsx`:
- Around line 159-160: The declared isSavingTransactionRef is unused and should
be used as a synchronous guard inside saveTransactionData to prevent
double-invocation race conditions: at the top of saveTransactionData check if
isSavingTransactionRef.current is true and return early, otherwise set
isSavingTransactionRef.current = true, then proceed with the existing logic,
updating the isSavingTransaction state for UI, and in a finally block set both
isSavingTransactionRef.current = false and isSavingTransaction = false (via the
state setter) so the ref enforces immediate mutual exclusion while state keeps
the UI in sync; update any early returns or error paths to ensure the ref is
reset.
In `@supabase/config.toml`:
- Around line 60-66: The supabase config references a missing seed file: in the
[db.seed] block update the configuration so resets don't fail — either create
the referenced "./seed.sql" file in the supabase directory, change sql_paths in
the [db.seed] section to point to an existing seed file/glob, or set enabled =
false to disable seeding; modify the sql_paths or enabled setting accordingly
and ensure the seed file name matches the configured path.
- Around line 154-158: The additional_redirect_urls entry currently uses
"https://127.0.0.1:3000" while site_url is "http://127.0.0.1:3000", causing
exact-match failures; update the additional_redirect_urls array to use the same
protocol as site_url (change "https://127.0.0.1:3000" to
"http://127.0.0.1:3000") so Auth redirect validation matches exactly between
site_url and additional_redirect_urls.
In `@supabase/migrations/20260508140001_create_transactions_table.sql`:
- Line 39: The composite unique index idx_transactions_order_txhash_unique on
table transactions won't prevent duplicates when order_id or tx_hash are NULL;
either make order_id and tx_hash NOT NULL or replace the plain unique index with
one or more partial unique indexes that exclude NULLs (e.g., a UNIQUE index on
(order_id, tx_hash) WHERE tx_hash IS NOT NULL and/or WHERE order_id IS NOT NULL)
so duplicates are constrained for the real values—update the migration to
drop/replace idx_transactions_order_txhash_unique accordingly and ensure the
intended idempotency behavior for transactions is enforced.
---
Outside diff comments:
In @.env.example:
- Around line 109-110: Remove the duplicate environment variable entry: keep a
single BREVO_LIST_ID declaration in the .env.example file and delete the extra
duplicate line so there is only one BREVO_LIST_ID key present (verify
surrounding spacing/newline remains consistent).
In `@docs/transaction-history-setup.md`:
- Around line 52-97: Update the docs SQL to match the actual migration
`20260508140001_create_transactions_table.sql`: make order_id and network
nullable (remove NOT NULL), add the CHECK constraints for transaction_type and
status (same enum/values used in the migration), add the unique index
idx_transactions_order_txhash_unique, and add the missing columns email,
email_sent_at, and explorer_link with the same types and defaults as in the
migration; alternatively replace the inline SQL with a clear reference/link to
the migration file
`supabase/migrations/20260508140001_create_transactions_table.sql` so the guide
always reflects the canonical schema.
---
Duplicate comments:
In `@app/context/KYCContext.tsx`:
- Around line 254-264: The effect watching walletAddress currently only resets
KYC state when walletAddress is truthy, leaving stale data when the user
disconnects; update the useEffect (the effect that references walletAddress and
refreshStatus) to handle both cases: when walletAddress is present keep the
existing reset-and-refresh logic (calling setTier(0), setIsPhoneVerified(false),
setPhoneNumber(null), setTransactionSummary(...), clearing
fetchGuardsRef.current and lastFetchTimeRef.current, then calling
refreshStatus(true)), and add an else branch that explicitly clears all
KYC-related state (setTier(null or 0 as appropriate), setIsPhoneVerified(false),
setPhoneNumber(null), setTransactionSummary to empty/zeroed values, and clear
fetchGuardsRef.current and lastFetchTimeRef.current) so state is cleared on
disconnect.
---
Nitpick comments:
In @.env.example:
- Around line 137-141: The inline comments on environment variables (e.g.,
SMILE_IDENTITY_BASE_URL, SMILE_IDENTITY_API_KEY, SMILE_IDENTITY_PARTNER_ID,
SMILE_ID_CALLBACK_URL, SMILE_IDENTITY_SERVER) can break some .env parsers;
update the .env.example by removing end-of-line comments from assignment lines
and instead place explanatory comments on their own preceding lines (or delete
them), ensuring each variable line contains only KEY=VALUE so parsers read
values reliably.
In `@app/api/v1/transactions/`[id]/route.ts:
- Around line 23-38: Update the fetch block to distinguish a real database error
from a not-found result: after the
supabaseAdmin.from(...).select(...).eq(...).single() call, if fetchError is
truthy call trackApiError(request, `/api/v1/transactions/${id}`, "PUT",
fetchError, 500) (or include fetchError message/details) and return a 500
response; only when fetchError is falsy but existingTransaction is
null/undefined call trackApiError(..., new Error("Transaction not found or
unauthorized"), 404) and return the 404 response. Ensure you reference the
existingTransaction and fetchError variables and reuse the same response bodies
but change status codes and logged error details accordingly.
In `@app/hooks/useSwapButton.ts`:
- Around line 137-160: The buttonAction function duplicates the isUserVerified
flag by accepting it as a parameter while the hook already has a single source
of truth for isUserVerified; remove the isUserVerified parameter from
buttonAction's signature and use the hook-scoped isUserVerified value instead,
update any call sites that pass isUserVerified to stop passing it, and ensure
related conditionals inside buttonAction (the branch that returns
setIsKycModalOpen or setIsLimitModalOpen) reference the hook-scoped
isUserVerified variable so the rendered CTA and click behavior stay in sync.
In `@app/lib/privy.ts`:
- Around line 86-104: collectLinkedEvmAddressesForPrivyUserId currently returns
an empty array when privy.getUser(userId) is null/undefined, which is
inconsistent with getWalletAddressFromPrivyUserId and
getSmartWalletAddressFromPrivyUserId that throw on missing user/linked accounts;
update collectLinkedEvmAddressesForPrivyUserId to perform the same null checks
on user and user.linkedAccounts and throw the same error (or a matching Error
message) as the other two functions instead of silently returning [], ensuring
you still collect and validate addresses with EVM_ADDRESS_LOWER after confirming
user existence via getPrivyClient()/privy.getUser.
🪄 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: 9578c0a9-fc70-450c-81aa-121f998edef4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.env.exampleapp/api/aggregator.tsapp/api/internal/set-wallet-context/route.tsapp/api/kyc/status/route.tsapp/api/kyc/tier3-verify/route.tsapp/api/phone/send-otp/route.tsapp/api/phone/verify-otp/route.tsapp/api/v1/transactions/[id]/route.tsapp/api/v1/transactions/route.tsapp/components/KycModal.tsxapp/components/PhoneVerificationModal.tsxapp/components/SettingsDropdown.tsxapp/components/TransactionLimitModal.tsxapp/context/KYCContext.tsxapp/hooks/useSwapButton.tsapp/lib/dojah.tsapp/lib/kyc-tier-limits.tsapp/lib/phone-verification.tsapp/lib/privy.tsapp/lib/smileID.tsapp/lib/supabase.tsapp/lib/transaction-wallet-auth.tsapp/pages/TransactionForm.tsxapp/pages/TransactionPreview.tsxdocs/environment-variables.mddocs/transaction-history-setup.mdjest.setup.jspackage.jsonpatches/@privy-io__react-auth@3.1.0.patchsupabase/.gitignoresupabase/config.tomlsupabase/migrations/20260508140001_create_transactions_table.sqlsupabase/migrations/20260508140002_create_user_kyc_profiles.sqlsupabase/migrations/20260508140003_create_wallet_migrations.sqlsupabase/migrations/20260508140004_create_saved_recipients.sqlsupabase/migrations/20260508140005_create_refund_accounts.sqlsupabase/migrations/20260508140006_create_blockfest_participants.sqlsupabase/migrations/20260508140007_create_blockfest_cashback_claims.sqlsupabase/migrations/20260508140008_add_user_kyc_profiles_otp_attempts.sql
✅ Files skipped from review due to trivial changes (4)
- jest.setup.js
- supabase/.gitignore
- supabase/migrations/20260508140008_add_user_kyc_profiles_otp_attempts.sql
- app/api/internal/set-wallet-context/route.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- app/lib/dojah.ts
- app/lib/kyc-tier-limits.ts
- app/lib/smileID.ts
- app/lib/phone-verification.ts
- app/components/SettingsDropdown.tsx
- app/api/kyc/status/route.ts
- app/api/kyc/tier3-verify/route.ts
- app/components/PhoneVerificationModal.tsx
- app/pages/TransactionForm.tsx
- app/api/aggregator.ts
- app/components/KycModal.tsx
Comment on lines
+141
to
149
| setIsLimitModalOpen: () => void, | ||
| isPhoneVerified: boolean, | ||
| setIsKycModalOpen: () => void, | ||
| isUserVerified: boolean, | ||
| openMigrationModal?: () => void, | ||
| ) => { | ||
| if (isStarknetOnramp) { | ||
| return () => {}; | ||
| } | ||
| if (needsMigration && authenticated && !isInjectedWallet && openMigrationModal) { | ||
| return openMigrationModal; | ||
| } |
Contributor
There was a problem hiding this comment.
Make the migration handler non-optional.
isEnabled already turns the button on for migration-required users at Lines 72-73, but this branch only intercepts the click when openMigrationModal is passed. If a caller omits that optional arg, the button falls through to the normal login/fund/swap path and can bypass the migration gate.
Suggested fix
const buttonAction = (
handleSwap: () => void,
login: () => void,
handleFundWallet: () => void,
setIsLimitModalOpen: () => void,
isPhoneVerified: boolean,
setIsKycModalOpen: () => void,
isUserVerified: boolean,
- openMigrationModal?: () => void,
+ openMigrationModal: () => void,
) => {
- if (needsMigration && authenticated && !isInjectedWallet && openMigrationModal) {
+ if (needsMigration && authenticated && !isInjectedWallet) {
return openMigrationModal;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/hooks/useSwapButton.ts` around lines 141 - 149, The migration click
handler must be required so the migration gate always intercepts clicks: change
the parameter openMigrationModal from optional to required in the useSwapButton
signature (remove the ? so openMigrationModal: () => void) and in the branch
that currently checks needsMigration && authenticated && !isInjectedWallet
remove the runtime optional check and always return openMigrationModal; update
any callers to pass a migration handler accordingly (refer to symbols:
needsMigration, authenticated, isInjectedWallet, openMigrationModal, and the
useSwapButton function).
Comment on lines
+159
to
+160
| // Ref to prevent duplicate transaction saves | ||
| const isSavingTransactionRef = useRef(false); |
Contributor
There was a problem hiding this comment.
Unused ref for preventing duplicate saves.
isSavingTransactionRef is declared at Line 159-160 but never read or written. The guard at Line 805 only checks the isSavingTransaction state variable, which doesn't prevent race conditions since React state updates are asynchronous. If saveTransactionData is called twice in quick succession, both calls could pass the isSavingTransaction check before either updates the state.
🛡️ Proposed fix to use the ref for synchronous guard
const saveTransactionData = async ({
orderId,
txHash,
providerAccount,
}: {
orderId: string;
txHash?: `0x${string}`;
providerAccount?: V2FiatProviderAccountDTO | null;
}) => {
- if (!activeWallet?.address || isSavingTransaction) return;
+ if (!activeWallet?.address || isSavingTransactionRef.current) return;
+ isSavingTransactionRef.current = true;
setIsSavingTransaction(true);
try {
// ... existing code ...
} catch (error) {
console.error("Error saving transaction:", error);
throw error;
} finally {
+ isSavingTransactionRef.current = false;
setIsSavingTransaction(false);
}
};Also applies to: 805-806
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/pages/TransactionPreview.tsx` around lines 159 - 160, The declared
isSavingTransactionRef is unused and should be used as a synchronous guard
inside saveTransactionData to prevent double-invocation race conditions: at the
top of saveTransactionData check if isSavingTransactionRef.current is true and
return early, otherwise set isSavingTransactionRef.current = true, then proceed
with the existing logic, updating the isSavingTransaction state for UI, and in a
finally block set both isSavingTransactionRef.current = false and
isSavingTransaction = false (via the state setter) so the ref enforces immediate
mutual exclusion while state keeps the UI in sync; update any early returns or
error paths to ensure the ref is reset.
Comment on lines
+60
to
+66
| [db.seed] | ||
| # If enabled, seeds the database after migrations during a db reset. | ||
| enabled = true | ||
| # Specifies an ordered list of seed files to load during db reset. | ||
| # Supports glob patterns relative to supabase directory: "./seeds/*.sql" | ||
| sql_paths = ["./seed.sql"] | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the configured seed path in supabase/config.toml exists.
set -euo pipefail
echo "Configured seed path:"
rg -n '^\s*sql_paths\s*=' supabase/config.toml
echo
echo "Checking seed file presence under supabase/:"
fd -a '^seed\.sql$' supabaseRepository: paycrest/noblocks
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Check for any seed-related files and directories
echo "=== Checking for seed files with various patterns ==="
fd -a 'seed' supabase/ 2>/dev/null || echo "(no matches)"
echo
echo "=== Checking for seeds directory ==="
ls -la supabase/ | grep -i seed || echo "(no seeds directory found)"
echo
echo "=== Checking for any SQL files in supabase ==="
fd -a '\.sql$' supabase/ 2>/dev/null || echo "(no SQL files)"
echo
echo "=== Checking for migrations directory ==="
ls -la supabase/ | head -20Repository: paycrest/noblocks
Length of output: 1259
🏁 Script executed:
cat -n supabase/.gitignoreRepository: paycrest/noblocks
Length of output: 189
Create missing seed.sql file or disable seeding.
Line 65 references ./seed.sql which does not exist in the supabase directory. With seeding enabled, supabase db reset will fail in fresh environments. Either create the seed file, update the path to an existing seed file, or set enabled = false if seeding is not needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/config.toml` around lines 60 - 66, The supabase config references a
missing seed file: in the [db.seed] block update the configuration so resets
don't fail — either create the referenced "./seed.sql" file in the supabase
directory, change sql_paths in the [db.seed] section to point to an existing
seed file/glob, or set enabled = false to disable seeding; modify the sql_paths
or enabled setting accordingly and ensure the seed file name matches the
configured path.
Comment on lines
+154
to
+158
| site_url = "http://127.0.0.1:3000" | ||
| # The public URL that Auth serves on. Defaults to the API external URL with `/auth/v1` appended. | ||
| # external_url = "" | ||
| # A list of *exact* URLs that auth providers are permitted to redirect to post authentication. | ||
| additional_redirect_urls = ["https://127.0.0.1:3000"] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Supabase Auth config.toml, does auth.additional_redirect_urls require exact protocol matching with redirect_to, and is http://127.0.0.1:3000 the correct local value when TLS is disabled?
💡 Result:
- Does auth.additional_redirect_urls require exact protocol matching with redirect_to? Yes. In the self-hosted Supabase Auth config (config.toml), auth.additional_redirect_urls is documented as “a list of exact URLs” that auth providers are permitted to redirect to post authentication [1]. This means protocol must match (http vs https), and typically the full URL must match the allow list entry for it to be accepted. Additionally, Supabase Auth maintainers explicitly noted that “the additional URLs need to be exact matches (including scheme, path)” [2]. Supabase also instructs that the URL in the client’s redirectTo parameter “should match the Redirect URLs list configuration” [3]. 2) Is http://127.0.0.1:3000 the correct local value when TLS is disabled? You should use the non-TLS (http) base URL for your local app that matches what your app actually uses when TLS is disabled. Supabase’s docs give the standard local development redirect URL example as http://localhost:3000 (and use this as the “Site URL” default) [3]. Whether 127.0.0.1 (instead of localhost) is correct depends on what hostname your redirectTo/Site URL in practice uses; Supabase expects exact matching [1][2]. If your app/redirectTo uses 127.0.0.1, then you must allow http://127.0.0.1:3000 (or the specific http://127.0.0.1:3000/... paths) in additional_redirect_urls; if your app uses http://localhost:3000 instead, allow-listing only 127.0.0.1 will not match. So: when TLS is disabled, the protocol portion should be http, but the host (localhost vs 127.0.0.1) must match your redirect_to value exactly, because the allow list uses exact URL matching [1][2].
Citations:
- 1: https://docs-2ej5s7s9m-supabase.vercel.app/docs/guides/self-hosting/auth/config
- 2: Additional redirect URLs not working reliably supabase/auth#123
- 3: https://supabase.com/docs/guides/auth/redirect-urls
Align redirect URL protocol with the configured local site URL.
Line 154 sets site_url = "http://127.0.0.1:3000", but Line 158 allow-lists https://127.0.0.1:3000. Supabase Auth requires exact URL matching for redirect validation, including protocol. This mismatch will cause auth redirects to fail in local development when TLS is disabled.
Suggested fix
- additional_redirect_urls = ["https://127.0.0.1:3000"]
+ additional_redirect_urls = ["http://127.0.0.1:3000"]📝 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
| site_url = "http://127.0.0.1:3000" | |
| # The public URL that Auth serves on. Defaults to the API external URL with `/auth/v1` appended. | |
| # external_url = "" | |
| # A list of *exact* URLs that auth providers are permitted to redirect to post authentication. | |
| additional_redirect_urls = ["https://127.0.0.1:3000"] | |
| site_url = "http://127.0.0.1:3000" | |
| # The public URL that Auth serves on. Defaults to the API external URL with `/auth/v1` appended. | |
| # external_url = "" | |
| # A list of *exact* URLs that auth providers are permitted to redirect to post authentication. | |
| additional_redirect_urls = ["http://127.0.0.1:3000"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/config.toml` around lines 154 - 158, The additional_redirect_urls
entry currently uses "https://127.0.0.1:3000" while site_url is
"http://127.0.0.1:3000", causing exact-match failures; update the
additional_redirect_urls array to use the same protocol as site_url (change
"https://127.0.0.1:3000" to "http://127.0.0.1:3000") so Auth redirect validation
matches exactly between site_url and additional_redirect_urls.
| -- Create indices | ||
| CREATE INDEX idx_transactions_wallet_address ON transactions (wallet_address); | ||
| CREATE INDEX idx_transactions_created_at ON transactions (created_at DESC); | ||
| CREATE UNIQUE INDEX idx_transactions_order_txhash_unique ON transactions (order_id, tx_hash); |
Contributor
There was a problem hiding this comment.
Composite unique index may not prevent all duplicates when columns are NULL.
PostgreSQL allows multiple rows with NULL values in unique indexes. Since both order_id and tx_hash are nullable, rows with (NULL, NULL) or (order_id, NULL) patterns won't be constrained. If idempotency is required, consider partial unique indexes as originally suggested:
Suggested fix
-CREATE UNIQUE INDEX idx_transactions_order_txhash_unique ON transactions (order_id, tx_hash);
+-- Composite index for lookups
+CREATE INDEX idx_transactions_order_txhash ON transactions (order_id, tx_hash);
+-- Partial unique indexes to prevent duplicates when values are present
+CREATE UNIQUE INDEX idx_transactions_order_id_unique
+ ON transactions (order_id)
+ WHERE order_id IS NOT NULL;
+CREATE UNIQUE INDEX idx_transactions_tx_hash_unique
+ ON transactions (tx_hash)
+ WHERE tx_hash IS NOT NULL;📝 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
| CREATE UNIQUE INDEX idx_transactions_order_txhash_unique ON transactions (order_id, tx_hash); | |
| -- Composite index for lookups | |
| CREATE INDEX idx_transactions_order_txhash ON transactions (order_id, tx_hash); | |
| -- Partial unique indexes to prevent duplicates when values are present | |
| CREATE UNIQUE INDEX idx_transactions_order_id_unique | |
| ON transactions (order_id) | |
| WHERE order_id IS NOT NULL; | |
| CREATE UNIQUE INDEX idx_transactions_tx_hash_unique | |
| ON transactions (tx_hash) | |
| WHERE tx_hash IS NOT NULL; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260508140001_create_transactions_table.sql` at line 39,
The composite unique index idx_transactions_order_txhash_unique on table
transactions won't prevent duplicates when order_id or tx_hash are NULL; either
make order_id and tx_hash NOT NULL or replace the plain unique index with one or
more partial unique indexes that exclude NULLs (e.g., a UNIQUE index on
(order_id, tx_hash) WHERE tx_hash IS NOT NULL and/or WHERE order_id IS NOT NULL)
so duplicates are constrained for the real values—update the migration to
drop/replace idx_transactions_order_txhash_unique accordingly and ensure the
intended idempotency behavior for transactions is enforced.
- Added a new endpoint for prechecking swap transactions to verify monthly KYC limits without inserting records. - Introduced utility functions for swap transaction limit checks, improving error handling and user feedback. - Updated KYC-related API routes to enhance wallet address verification and error messaging. - Enhanced the .env.example file with additional configuration options for Dojah utility bill processing. - Improved handling of KYC document uploads and error messages in the KYC verification process. - Updated transaction form to support new swap functionality and ensure accurate limit checks based on user KYC tier.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
app/api/v1/transactions/route.ts (2)
208-243: 💤 Low valueMissing error tracking for
kyc_db_errorandrate_unavailableresponses.These error paths return 503 responses but don't call
trackApiError, unlikekyc_requiredandlimit_exceededwhich do track errors. This creates gaps in error observability.📊 Proposed fix to add error tracking
if (swapResult.kind === "kyc_db_error") { + trackApiError( + request, + "/api/v1/transactions", + "POST", + new Error("KYC DB lookup failed"), + 503, + ); return NextResponse.json( { success: false, error: "Unable to verify transaction limits. Please try again.", }, { status: 503 }, ); } ... if (swapResult.kind === "rate_unavailable") { + trackApiError( + request, + "/api/v1/transactions", + "POST", + new Error("cNGN to USD rate unavailable"), + 503, + ); return NextResponse.json( { success: false, error: "Unable to verify transaction amount. Please try again.", }, { status: 503 }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/transactions/route.ts` around lines 208 - 243, The kyc_db_error and rate_unavailable branches don't call trackApiError, so add calls to trackApiError in those conditionals (where swapResult.kind === "kyc_db_error" and "rate_unavailable") similar to the existing kyc_required/limit_exceeded handling: call trackApiError(request, "/api/v1/transactions", "POST", new Error("<appropriate message>"), 503) before returning the 503 NextResponse.json so the error is tracked with the correct context and status.
272-277: 💤 Low valueRedundant null check after discriminated union exhaustion.
After handling all error
kindvalues and confirmingkind === "success", TypeScript should narrow the type. However, theidfield is typed as optional (id?: string) in thesuccesscase, making this runtime check necessary despite being conceptually redundant for non-dry-run paths.Consider tightening the
SwapLimitCheckResulttype to makeidrequired whendryRun: falsereturns success, but this is a minor type refinement that can be deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/v1/transactions/route.ts` around lines 272 - 277, The runtime null check for rpcDataId after handling all SwapLimitCheckResult kinds is redundant because once you narrow swapResult.kind === "success" the ID should be present; fix by updating the SwapLimitCheckResult success variant returned by insert_swap_transaction_if_within_limit so that success includes a required id (e.g., remove the optional `id?: string` on the success branch and require `id: string` when dryRun is false), update any call sites accordingly, and then remove the extra runtime if-check and throw; as a short-term alternative, if you cannot change the type now, replace the check with a non-null assertion (use swapResult.id!) only in the success branch to signal the invariant..env.example (3)
147-155: 💤 Low valueConsider consistent placeholder formatting.
The Dojah section uses
<DOJAH_APP_ID_PLACEHOLDER>format, which differs from theyour_*_hereformat used elsewhere (e.g., KudiSMS, SmileID). For consistency across the file, consider using the same placeholder style throughout.Also verify whether
KYC_DOCUMENTS_BUCKET=kyc-documentsis a fixed bucket name for all deployments or should be a placeholder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 147 - 155, The placeholders for Dojah are inconsistent with the rest of the file; replace DOJAH_APP_ID=<DOJAH_APP_ID_PLACEHOLDER> and DOJAH_SECRET_KEY=<DOJAH_SECRET_KEY_PLACEHOLDER> with the project-wide placeholder style (e.g., your_dojah_app_id_here and your_dojah_secret_key_here) and confirm DOJAH_BASE_URL stays as the default URL, and change KYC_DOCUMENTS_BUCKET=kyc-documents to a placeholder (e.g., your_kyc_documents_bucket_here) if the bucket name is not guaranteed fixed across deployments so the .env example is consistent and clearly configurable.
20-24: ⚡ Quick winClarify whether these are default values or placeholders.
The comment states "Omitted or empty = defaults below" but the values shown appear to be the actual defaults (0, 100, 15000, 50000). Consider rephrasing to "defaults shown below:" or "default values:" to make it clear these are the fallback values when omitted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 20 - 24, The comment is ambiguous about whether the listed NEXT_PUBLIC_KYC_TIER_* variables are placeholders or actual defaults; update the comment above NEXT_PUBLIC_KYC_TIER_0_MONTHLY, NEXT_PUBLIC_KYC_TIER_1_MONTHLY, NEXT_PUBLIC_KYC_TIER_2_MONTHLY, and NEXT_PUBLIC_KYC_TIER_3_MONTHLY to explicitly state these are the default fallback values (e.g., "Default values shown below:" or "Defaults shown below:") so it is clear that omitting or leaving them empty will use the displayed numeric defaults.
119-124: ⚡ Quick winVerify the KUDISMS_SENDER_ID value.
KUDISMS_SENDER_IDis set toNoblockswhile other KudiSMS variables useyour_*placeholder format. Confirm whether "Noblocks" is the intended sender ID for all deployments or if this should be a placeholder likeyour_sender_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 119 - 124, The KUDISMS_SENDER_ID value in .env.example is hardcoded as "Noblocks" while the other KudiSMS entries use placeholders; update KUDISMS_SENDER_ID to match the placeholder style (e.g., your_sender_id) or confirm and document that "Noblocks" is required for all deployments; modify the KUDISMS_SENDER_ID entry in .env.example and any related README/docs to either use a consistent placeholder (KUDISMS_SENDER_ID=your_sender_id) or add a comment explaining that "Noblocks" is the expected fixed sender ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 137-141: The SmileID env entries have spacing, inline-comment, and
quoting issues; fix the SMILE_IDENTITY_* lines by removing any spaces around the
= (e.g., change SMILE_IDENTITY_CALLBACK_URL= to SMILE_IDENTITY_CALLBACK_URL=),
move all inline comments to their own lines above each variable (e.g., add a
comment line "# get from docs" before SMILE_IDENTITY_BASE_URL), and quote values
that may be ambiguous (wrap XXXXXX, numeric flags like 0/1, and any API keys/IDs
in quotes) so update SMILE_IDENTITY_BASE_URL, SMILE_IDENTITY_API_KEY,
SMILE_IDENTITY_PARTNER_ID, SMILE_IDENTITY_CALLBACK_URL, and
SMILE_IDENTITY_SERVER accordingly.
In `@app/api/aggregator.ts`:
- Around line 1015-1062: The submitSmileIDData function is missing the required
x-wallet-address header when calling /api/kyc/smile-id; update the function
signature (export const submitSmileIDData = async (payload: any, accessToken:
string, walletAddress: string): Promise<SmileIDSubmissionResponse>) or extract
the wallet address from payload (e.g., payload.walletAddress) and include it in
the axios POST headers as 'x-wallet-address': walletAddress; ensure the header
is sent alongside Authorization so the route handler that checks
x-wallet-address will receive it.
In `@app/api/v1/wallets/deprecate/route.ts`:
- Around line 140-146: The update on user_kyc_profiles currently filters only by
wallet_address and then writes a new user_id (using supabaseAdmin
.from("user_kyc_profiles").update(...)), which can reassign KYC across accounts;
narrow the WHERE clause to include the authenticated user by adding a second
equality filter for user_id (using the userId variable) alongside
.eq("wallet_address", oldAddress.toLowerCase()) so the update only affects the
calling user's KYC record and still normalize addresses with toLowerCase().
In
`@supabase/migrations/20260508140011_fix_increment_kyc_attempts_null_attempts.sql`:
- Around line 5-7: Add a schema-level invariant to prevent attempts from
becoming NULL again: after backfilling NULLs in public.user_kyc_profiles (the
existing UPDATE), ALTER the attempts column on table user_kyc_profiles to SET
DEFAULT 0 and then ALTER it to SET NOT NULL. Ensure you reference the column
name attempts and the table user_kyc_profiles so the migration first fixes
existing NULLs (already present) and then applies ALTER TABLE ... ALTER COLUMN
attempts SET DEFAULT 0; followed by ALTER TABLE ... ALTER COLUMN attempts SET
NOT NULL; to enforce the invariant for future writes.
- Around line 19-25: The function increment_kyc_attempts currently returns NULL
for both "profile not found" and "max attempts reached"; update it to
disambiguate these paths by, after the UPDATE INTO v_attempts, if v_attempts IS
NULL then run a SELECT EXISTS(SELECT 1 FROM public.user_kyc_profiles WHERE
wallet_address = p_wallet_address) INTO v_exists (or COUNT(*) INTO v_count) and
if no row exists RETURN a distinct sentinel (e.g., -1) to signal "profile not
found", otherwise RETURN NULL (or another sentinel like -2) to signal "max
attempts reached"; reference p_wallet_address, p_max_attempts and v_attempts in
the logic and ensure the function's callers/docs are updated to expect the new
sentinel.
---
Nitpick comments:
In @.env.example:
- Around line 147-155: The placeholders for Dojah are inconsistent with the rest
of the file; replace DOJAH_APP_ID=<DOJAH_APP_ID_PLACEHOLDER> and
DOJAH_SECRET_KEY=<DOJAH_SECRET_KEY_PLACEHOLDER> with the project-wide
placeholder style (e.g., your_dojah_app_id_here and your_dojah_secret_key_here)
and confirm DOJAH_BASE_URL stays as the default URL, and change
KYC_DOCUMENTS_BUCKET=kyc-documents to a placeholder (e.g.,
your_kyc_documents_bucket_here) if the bucket name is not guaranteed fixed
across deployments so the .env example is consistent and clearly configurable.
- Around line 20-24: The comment is ambiguous about whether the listed
NEXT_PUBLIC_KYC_TIER_* variables are placeholders or actual defaults; update the
comment above NEXT_PUBLIC_KYC_TIER_0_MONTHLY, NEXT_PUBLIC_KYC_TIER_1_MONTHLY,
NEXT_PUBLIC_KYC_TIER_2_MONTHLY, and NEXT_PUBLIC_KYC_TIER_3_MONTHLY to explicitly
state these are the default fallback values (e.g., "Default values shown below:"
or "Defaults shown below:") so it is clear that omitting or leaving them empty
will use the displayed numeric defaults.
- Around line 119-124: The KUDISMS_SENDER_ID value in .env.example is hardcoded
as "Noblocks" while the other KudiSMS entries use placeholders; update
KUDISMS_SENDER_ID to match the placeholder style (e.g., your_sender_id) or
confirm and document that "Noblocks" is required for all deployments; modify the
KUDISMS_SENDER_ID entry in .env.example and any related README/docs to either
use a consistent placeholder (KUDISMS_SENDER_ID=your_sender_id) or add a comment
explaining that "Noblocks" is the expected fixed sender ID.
In `@app/api/v1/transactions/route.ts`:
- Around line 208-243: The kyc_db_error and rate_unavailable branches don't call
trackApiError, so add calls to trackApiError in those conditionals (where
swapResult.kind === "kyc_db_error" and "rate_unavailable") similar to the
existing kyc_required/limit_exceeded handling: call trackApiError(request,
"/api/v1/transactions", "POST", new Error("<appropriate message>"), 503) before
returning the 503 NextResponse.json so the error is tracked with the correct
context and status.
- Around line 272-277: The runtime null check for rpcDataId after handling all
SwapLimitCheckResult kinds is redundant because once you narrow swapResult.kind
=== "success" the ID should be present; fix by updating the SwapLimitCheckResult
success variant returned by insert_swap_transaction_if_within_limit so that
success includes a required id (e.g., remove the optional `id?: string` on the
success branch and require `id: string` when dryRun is false), update any call
sites accordingly, and then remove the extra runtime if-check and throw; as a
short-term alternative, if you cannot change the type now, replace the check
with a non-null assertion (use swapResult.id!) only in the success branch to
signal the invariant.
🪄 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: 24e838c6-7cab-466d-b58a-367e3e3cc7ab
📒 Files selected for processing (18)
.env.exampleapp/api/aggregator.tsapp/api/kyc/tier3-verify/route.tsapp/api/v1/transactions/route.tsapp/api/v1/transactions/swap-precheck/route.tsapp/api/v1/wallets/deprecate/route.tsapp/components/KycModal.tsxapp/components/MainPageContent.tsxapp/components/PhoneVerificationModal.tsxapp/hooks/useSwapButton.tsapp/lib/swap-transaction-limit-server.tsapp/pages/TransactionForm.tsxapp/pages/TransactionPreview.tsxnext.config.mjssupabase/config.tomlsupabase/migrations/20260508140010_create_kyc_documents_storage_bucket.sqlsupabase/migrations/20260508140011_fix_increment_kyc_attempts_null_attempts.sqlsupabase/migrations/20260508140012_insert_swap_transaction_dry_run.sql
✅ Files skipped from review due to trivial changes (1)
- supabase/migrations/20260508140010_create_kyc_documents_storage_bucket.sql
🚧 Files skipped from review as they are similar to previous changes (7)
- app/api/kyc/tier3-verify/route.ts
- app/components/PhoneVerificationModal.tsx
- supabase/config.toml
- app/hooks/useSwapButton.ts
- app/pages/TransactionPreview.tsx
- app/pages/TransactionForm.tsx
- app/components/KycModal.tsx
Comment on lines
+137
to
+141
| SMILE_IDENTITY_BASE_URL=XXXXXX #get from docs | ||
| SMILE_IDENTITY_API_KEY=your_api_key_here | ||
| SMILE_IDENTITY_PARTNER_ID=your_partner_id_here | ||
| SMILE_ID_CALLBACK_URL= #optional callback url | ||
| SMILE_IDENTITY_SERVER=0 # 0 for sandbox, 1 for production |
Contributor
There was a problem hiding this comment.
Fix .env formatting issues in SmileID configuration.
Several formatting issues that may cause parsing errors:
- Line 140: Spaces around the
=sign (SMILE_ID_CALLBACK_URL=#optional``) will cause errors with most .env parsers. Remove spaces:SMILE_ID_CALLBACK_URL=#optional... - Inline comments: Comments after values (e.g.,
#get from docs,#optional callback url) may be included in the parsed value by some .env parsers. Move comments to separate lines above each variable for safety. - Missing quotes: Values like
XXXXXXand0should be quoted to avoid edge-case parsing issues.
📝 Suggested formatting fix
+# Get base URL from SmileID documentation
-SMILE_IDENTITY_BASE_URL=XXXXXX `#get` from docs
+SMILE_IDENTITY_BASE_URL="XXXXXX"
SMILE_IDENTITY_API_KEY=your_api_key_here
SMILE_IDENTITY_PARTNER_ID=your_partner_id_here
-SMILE_ID_CALLBACK_URL= `#optional` callback url
+# Optional callback URL
+SMILE_ID_CALLBACK_URL=""
-SMILE_IDENTITY_SERVER=0 # 0 for sandbox, 1 for production
+# 0 for sandbox, 1 for production
+SMILE_IDENTITY_SERVER="0"📝 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
| SMILE_IDENTITY_BASE_URL=XXXXXX #get from docs | |
| SMILE_IDENTITY_API_KEY=your_api_key_here | |
| SMILE_IDENTITY_PARTNER_ID=your_partner_id_here | |
| SMILE_ID_CALLBACK_URL= #optional callback url | |
| SMILE_IDENTITY_SERVER=0 # 0 for sandbox, 1 for production | |
| # Get base URL from SmileID documentation | |
| SMILE_IDENTITY_BASE_URL="XXXXXX" | |
| SMILE_IDENTITY_API_KEY=your_api_key_here | |
| SMILE_IDENTITY_PARTNER_ID=your_partner_id_here | |
| # Optional callback URL | |
| SMILE_ID_CALLBACK_URL="" | |
| # 0 for sandbox, 1 for production | |
| SMILE_IDENTITY_SERVER="0" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 137-137: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 138-138: [UnorderedKey] The SMILE_IDENTITY_API_KEY key should go before the SMILE_IDENTITY_BASE_URL key
(UnorderedKey)
[warning] 140-140: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 140-140: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 141-141: [UnorderedKey] The SMILE_IDENTITY_SERVER key should go before the SMILE_ID_CALLBACK_URL key
(UnorderedKey)
[warning] 141-141: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 137 - 141, The SmileID env entries have spacing,
inline-comment, and quoting issues; fix the SMILE_IDENTITY_* lines by removing
any spaces around the = (e.g., change SMILE_IDENTITY_CALLBACK_URL= to
SMILE_IDENTITY_CALLBACK_URL=), move all inline comments to their own lines above
each variable (e.g., add a comment line "# get from docs" before
SMILE_IDENTITY_BASE_URL), and quote values that may be ambiguous (wrap XXXXXX,
numeric flags like 0/1, and any API keys/IDs in quotes) so update
SMILE_IDENTITY_BASE_URL, SMILE_IDENTITY_API_KEY, SMILE_IDENTITY_PARTNER_ID,
SMILE_IDENTITY_CALLBACK_URL, and SMILE_IDENTITY_SERVER accordingly.
Comment on lines
+1015
to
+1062
| export const submitSmileIDData = async ( | ||
| payload: any, | ||
| accessToken: string, | ||
| ): Promise<SmileIDSubmissionResponse> => { | ||
| const startTime = Date.now(); | ||
|
|
||
| try { | ||
| // Track external API request (log metadata only, no PII) | ||
| trackServerEvent("External API Request", { | ||
| service: "next-api", | ||
| endpoint: "/api/kyc/smile-id", | ||
| method: "POST", | ||
| }); | ||
|
|
||
| // Call Next.js API route with JWT authentication | ||
| const response = await axios.post(`/api/kyc/smile-id`, payload, { | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| }); | ||
|
|
||
| // Track successful response | ||
| const responseTime = Date.now() - startTime; | ||
| trackApiResponse("/api/kyc/smile-id", "POST", 200, responseTime, { | ||
| service: "next-api", | ||
| }); | ||
|
|
||
| // Track business event | ||
| trackBusinessEvent("Smile ID Data Submitted", { | ||
| jobId: response.data.data?.jobId, | ||
| }); | ||
|
|
||
| return response.data; | ||
| } catch (error) { | ||
| const responseTime = Date.now() - startTime; | ||
|
|
||
| // Track API error | ||
| trackServerEvent("External API Error", { | ||
| service: "next-api", | ||
| endpoint: "/api/kyc/smile-id", | ||
| method: "POST", | ||
| error_message: error instanceof Error ? error.message : "Unknown error", | ||
| response_time_ms: responseTime, | ||
| }); | ||
|
|
||
| throw error; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if /api/kyc/smile-id route requires x-wallet-address header
rg -n "x-wallet-address" app/api/kyc/smile-id/route.ts 2>/dev/null || echo "File not found or no matches"
# Also check the route handler for wallet address requirements
fd -t f "route.ts" app/api/kyc/smile-id -x cat {} 2>/dev/null | head -50Repository: paycrest/noblocks
Length of output: 1605
Add x-wallet-address header to the request—it's required by the endpoint.
The /api/kyc/smile-id route handler (line 20) retrieves and validates the x-wallet-address header, returning a 401 Unauthorized response if missing. The current code only sends the Authorization header, causing requests to fail. Include the wallet address header in the axios request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/aggregator.ts` around lines 1015 - 1062, The submitSmileIDData
function is missing the required x-wallet-address header when calling
/api/kyc/smile-id; update the function signature (export const submitSmileIDData
= async (payload: any, accessToken: string, walletAddress: string):
Promise<SmileIDSubmissionResponse>) or extract the wallet address from payload
(e.g., payload.walletAddress) and include it in the axios POST headers as
'x-wallet-address': walletAddress; ensure the header is sent alongside
Authorization so the route handler that checks x-wallet-address will receive it.
Comment on lines
140
to
+146
| const { error: kycError } = await supabaseAdmin | ||
| .from("kyc_data") | ||
| .update({ wallet_address: newAddress.toLowerCase() }) | ||
| .eq("wallet_address", oldAddress.toLowerCase()) | ||
| .eq("user_id", userId); | ||
| .from("user_kyc_profiles") | ||
| .update({ | ||
| wallet_address: newAddress.toLowerCase(), | ||
| user_id: userId, | ||
| }) | ||
| .eq("wallet_address", oldAddress.toLowerCase()); |
Contributor
There was a problem hiding this comment.
Scope KYC migration update to the authenticated user to prevent cross-account reassignment.
Line 146 filters only by wallet_address, while Lines 142-145 also mutate user_id. This can reassign another user’s KYC profile if oldAddress is not uniquely bound to this user in practice.
🔒 Proposed fix
const { error: kycError } = await supabaseAdmin
.from("user_kyc_profiles")
.update({
wallet_address: newAddress.toLowerCase(),
- user_id: userId,
})
- .eq("wallet_address", oldAddress.toLowerCase());
+ .eq("wallet_address", oldAddress.toLowerCase())
+ .eq("user_id", userId);📝 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 { error: kycError } = await supabaseAdmin | |
| .from("kyc_data") | |
| .update({ wallet_address: newAddress.toLowerCase() }) | |
| .eq("wallet_address", oldAddress.toLowerCase()) | |
| .eq("user_id", userId); | |
| .from("user_kyc_profiles") | |
| .update({ | |
| wallet_address: newAddress.toLowerCase(), | |
| user_id: userId, | |
| }) | |
| .eq("wallet_address", oldAddress.toLowerCase()); | |
| const { error: kycError } = await supabaseAdmin | |
| .from("user_kyc_profiles") | |
| .update({ | |
| wallet_address: newAddress.toLowerCase(), | |
| }) | |
| .eq("wallet_address", oldAddress.toLowerCase()) | |
| .eq("user_id", userId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/wallets/deprecate/route.ts` around lines 140 - 146, The update on
user_kyc_profiles currently filters only by wallet_address and then writes a new
user_id (using supabaseAdmin .from("user_kyc_profiles").update(...)), which can
reassign KYC across accounts; narrow the WHERE clause to include the
authenticated user by adding a second equality filter for user_id (using the
userId variable) alongside .eq("wallet_address", oldAddress.toLowerCase()) so
the update only affects the calling user's KYC record and still normalize
addresses with toLowerCase().
Comment on lines
+5
to
+7
| UPDATE public.user_kyc_profiles | ||
| SET attempts = 0 | ||
| WHERE attempts IS NULL; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Harden the fix with a column constraint/default.
Line 5–7 backfills existing rows, but attempts can still become NULL again from future writes. Add a schema-level invariant (DEFAULT 0 + NOT NULL) to prevent regressions.
Proposed migration addition
UPDATE public.user_kyc_profiles
SET attempts = 0
WHERE attempts IS NULL;
+
+ALTER TABLE public.user_kyc_profiles
+ ALTER COLUMN attempts SET DEFAULT 0,
+ ALTER COLUMN attempts SET NOT NULL;📝 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
| UPDATE public.user_kyc_profiles | |
| SET attempts = 0 | |
| WHERE attempts IS NULL; | |
| UPDATE public.user_kyc_profiles | |
| SET attempts = 0 | |
| WHERE attempts IS NULL; | |
| ALTER TABLE public.user_kyc_profiles | |
| ALTER COLUMN attempts SET DEFAULT 0, | |
| ALTER COLUMN attempts SET NOT NULL; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@supabase/migrations/20260508140011_fix_increment_kyc_attempts_null_attempts.sql`
around lines 5 - 7, Add a schema-level invariant to prevent attempts from
becoming NULL again: after backfilling NULLs in public.user_kyc_profiles (the
existing UPDATE), ALTER the attempts column on table user_kyc_profiles to SET
DEFAULT 0 and then ALTER it to SET NOT NULL. Ensure you reference the column
name attempts and the table user_kyc_profiles so the migration first fixes
existing NULLs (already present) and then applies ALTER TABLE ... ALTER COLUMN
attempts SET DEFAULT 0; followed by ALTER TABLE ... ALTER COLUMN attempts SET
NOT NULL; to enforce the invariant for future writes.
Comment on lines
+19
to
+25
| UPDATE public.user_kyc_profiles | ||
| SET attempts = COALESCE(attempts, 0) + 1 | ||
| WHERE wallet_address = p_wallet_address | ||
| AND COALESCE(attempts, 0) < p_max_attempts | ||
| RETURNING attempts INTO v_attempts; | ||
|
|
||
| RETURN v_attempts; |
Contributor
There was a problem hiding this comment.
Disambiguate NULL return paths in increment_kyc_attempts.
On Line 19–25, NULL is returned for both “max attempts reached” and “wallet profile not found”. That can misclassify missing profiles as exhausted attempts.
Proposed function refinement
UPDATE public.user_kyc_profiles
SET attempts = COALESCE(attempts, 0) + 1
WHERE wallet_address = p_wallet_address
AND COALESCE(attempts, 0) < p_max_attempts
RETURNING attempts INTO v_attempts;
+ IF v_attempts IS NULL THEN
+ IF EXISTS (
+ SELECT 1
+ FROM public.user_kyc_profiles
+ WHERE wallet_address = p_wallet_address
+ ) THEN
+ RETURN NULL; -- exhausted
+ END IF;
+
+ RAISE EXCEPTION 'KYC profile not found for wallet: %', p_wallet_address
+ USING ERRCODE = 'P0002';
+ END IF;
+
RETURN v_attempts;📝 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
| UPDATE public.user_kyc_profiles | |
| SET attempts = COALESCE(attempts, 0) + 1 | |
| WHERE wallet_address = p_wallet_address | |
| AND COALESCE(attempts, 0) < p_max_attempts | |
| RETURNING attempts INTO v_attempts; | |
| RETURN v_attempts; | |
| UPDATE public.user_kyc_profiles | |
| SET attempts = COALESCE(attempts, 0) + 1 | |
| WHERE wallet_address = p_wallet_address | |
| AND COALESCE(attempts, 0) < p_max_attempts | |
| RETURNING attempts INTO v_attempts; | |
| IF v_attempts IS NULL THEN | |
| IF EXISTS ( | |
| SELECT 1 | |
| FROM public.user_kyc_profiles | |
| WHERE wallet_address = p_wallet_address | |
| ) THEN | |
| RETURN NULL; -- exhausted | |
| END IF; | |
| RAISE EXCEPTION 'KYC profile not found for wallet: %', p_wallet_address | |
| USING ERRCODE = 'P0002'; | |
| END IF; | |
| RETURN v_attempts; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@supabase/migrations/20260508140011_fix_increment_kyc_attempts_null_attempts.sql`
around lines 19 - 25, The function increment_kyc_attempts currently returns NULL
for both "profile not found" and "max attempts reached"; update it to
disambiguate these paths by, after the UPDATE INTO v_attempts, if v_attempts IS
NULL then run a SELECT EXISTS(SELECT 1 FROM public.user_kyc_profiles WHERE
wallet_address = p_wallet_address) INTO v_exists (or COUNT(*) INTO v_count) and
if no row exists RETURN a distinct sentinel (e.g., -1) to signal "profile not
found", otherwise RETURN NULL (or another sentinel like -2) to signal "max
attempts reached"; reference p_wallet_address, p_max_attempts and v_attempts in
the logic and ensure the function's callers/docs are updated to expect the new
sentinel.
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
1 participant
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.
Description
This PR extracts the tiered KYC implementation from
feat-implement-tiered-kyconto the latestmainto provide a clean, reviewable integration branch.The change introduces tiered KYC endpoints, phone OTP verification flows, SmileID submission/status handling, KYC-related UI components and context wiring, plus required DB migration and environment documentation updates. During extraction, core mainline exports and baseline dependency behavior were preserved to avoid unrelated regressions from the source branch. A syntax issue in
app/api/aggregator.tswas also corrected while porting.Potential impact areas include KYC gating on transaction flows, phone verification UX, and server-side KYC route behavior dependent on Supabase/Twilio/SmileID environment configuration.
References
feat-implement-tiered-kycTesting
pnpm installsuccessfully on the extracted branchpnpm buildand confirmed compile/type-check progression; build currently stops at page-data collection due missing localSUPABASE_URLenv in this worktreeapp/api/aggregator.ts,app/context/index.ts,app/types.ts,app/pages/TransactionForm.tsx,app/components/KycModal.tsx)Environment:
macOS (darwin 25.3.0)
Node.js 22.x
pnpm 10.27.0
Next.js 15.5.x
This change adds test coverage for new/changed/fixed functionality
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Improvements