Implement identity-scoped referral and cashback claim checks - #659
Implement identity-scoped referral and cashback claim checks#659Dprof-in-tech wants to merge 10 commits into
Conversation
- Introduced `resolveOwnIdentityFingerprint` and `resolveIdentityScope` functions to ensure referral and cashback claims are validated against a user's verified identity across multiple wallets. - Updated referral and cashback claim routes to utilize these new identity checks, preventing multiple claims from the same identity. - Added new columns for identity phone and ID key in the referrals and referral claims tables, along with unique indexes to enforce identity-scoped constraints. - Enhanced error handling for identity verification failures during claim processes, ensuring users receive appropriate feedback. This update strengthens the integrity of the referral and cashback systems by enforcing identity-based limits on claims.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds canonical identity fingerprints to referral records, enforces identity-based referral constraints, and reserves cashback claims through an atomic RPC across identity-linked wallets. It also records transfer attempts and reaps eligible stale claims. ChangesIdentity-scoped rewards
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/blockfest/cashback/route.ts (1)
275-333: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftPooled cashback limits are enforced via a racy check-then-insert, not atomically.
claimCount/totalClaimedare computed acrossscopeWalletsand compared againstMAX_CLAIMS_PER_WALLET/MAX_CASHBACK_PER_WALLETbefore the pending claim is inserted (Step 11, Line 366). Nothing serializes this per-identity: several sibling wallets sharing the same identity scope can send concurrent requests, each reading the same (stale)claimCount/totalClaimedbefore any prior request's insert commits, and all pass the check. This is precisely the multi-wallet abuse pattern this PR is meant to close — pooling the read without pooling the write doesn't prevent concurrent bypass, it just narrows the window slightly compared to the pre-existing single-wallet self-race.Since a passing check directly triggers an on-chain USDC transfer, a successful race results in real over-payment beyond the per-identity cap. Consider serializing per-identity claims — e.g. a Postgres advisory lock keyed on a stable identity key (from
resolveIdentityScope().identityKeys) held for the duration of the check+insert, or restructuring the count/amount check as part of a single atomic transaction/stored procedure rather than separateSELECTs followed by anINSERT.🤖 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/blockfest/cashback/route.ts` around lines 275 - 333, Serialize the pooled limit check and pending-claim insert in the claim handler so concurrent requests sharing the same resolved identity cannot pass stale checks simultaneously. Use a stable key from resolveIdentityScope().identityKeys for a Postgres advisory lock held through the claimCount/totalClaimed validation and Step 11 insert, or move those operations into an equivalent atomic transaction/stored procedure while preserving the existing limit responses.
🧹 Nitpick comments (2)
app/lib/kyc-identity.ts (1)
88-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fingerprint-derivation logic vs.
resolveIdentityScope.The phone/
hasId/idKeyderivation here (Lines 103-115) is byte-for-byte the same shape as the own-profile portion ofresolveIdentityScope(Lines 135-136, 165-177). Consider extracting a sharedderiveFingerprint(profile)helper used by both, so the two functions can't drift (e.g. if theidKeyformat ever changes).♻️ Suggested extraction
+function deriveFingerprint(profile: { + phone_number?: string | null; + id_country?: string | null; + id_type?: string | null; + id_number?: string | null; +} | null): { phone: string | null; idKey: string | null } { + const phone = profile?.phone_number || null; + const hasId = !!(profile?.id_country && profile?.id_type && profile?.id_number); + return { + phone, + idKey: hasId ? `${profile!.id_country}:${profile!.id_type}:${profile!.id_number}` : null, + }; +} + export async function resolveOwnIdentityFingerprint( walletAddress: string, ): Promise<{ phone: string | null; idKey: string | null }> { ... - const phone = profile?.phone_number || null; - const hasId = !!( - profile?.id_country && - profile?.id_type && - profile?.id_number - ); - - return { - phone, - idKey: hasId - ? `${profile!.id_country}:${profile!.id_type}:${profile!.id_number}` - : null, - }; + return deriveFingerprint(profile); }🤖 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/kyc-identity.ts` around lines 88 - 116, Extract the shared phone, hasId, and idKey derivation from resolveOwnIdentityFingerprint and the own-profile portion of resolveIdentityScope into a common deriveFingerprint(profile) helper. Update both functions to use this helper while preserving the current null handling and idKey format.supabase/migrations/20260730120000_identity_scoped_referrals.sql (1)
26-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMake these indexes concurrent.
CREATE UNIQUE INDEXblocks writes while it builds. The Supabase CLI handlesCREATE INDEX CONCURRENTLYin migrations, so switch all four indexes to concurrent creation to avoid stalling referral submissions/claims during deploy.🤖 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/20260730120000_identity_scoped_referrals.sql` around lines 26 - 45, Update the four unique index definitions—referrals_identity_phone_unique, referrals_identity_id_key_unique, referral_claims_referee_identity_phone_unique, and referral_claims_referee_identity_id_key_unique—to use concurrent creation while preserving their existing uniqueness conditions and partial WHERE clauses.Source: Linters/SAST tools
🤖 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/blockfest/cashback/route.ts`:
- Line 10: Update the identity resolution in the cashback route to use the same
wallet identity key as the KYC and referral flow, rather than the smart-wallet
address returned by getSmartWalletAddressFromPrivyUserId(). Ensure
resolveIdentityScope() receives the regular wallet address stored in
user_kyc_profiles so existing KYC rows are matched and the caller fallback is
not incorrectly used.
---
Outside diff comments:
In `@app/api/blockfest/cashback/route.ts`:
- Around line 275-333: Serialize the pooled limit check and pending-claim insert
in the claim handler so concurrent requests sharing the same resolved identity
cannot pass stale checks simultaneously. Use a stable key from
resolveIdentityScope().identityKeys for a Postgres advisory lock held through
the claimCount/totalClaimed validation and Step 11 insert, or move those
operations into an equivalent atomic transaction/stored procedure while
preserving the existing limit responses.
---
Nitpick comments:
In `@app/lib/kyc-identity.ts`:
- Around line 88-116: Extract the shared phone, hasId, and idKey derivation from
resolveOwnIdentityFingerprint and the own-profile portion of
resolveIdentityScope into a common deriveFingerprint(profile) helper. Update
both functions to use this helper while preserving the current null handling and
idKey format.
In `@supabase/migrations/20260730120000_identity_scoped_referrals.sql`:
- Around line 26-45: Update the four unique index
definitions—referrals_identity_phone_unique, referrals_identity_id_key_unique,
referral_claims_referee_identity_phone_unique, and
referral_claims_referee_identity_id_key_unique—to use concurrent creation while
preserving their existing uniqueness conditions and partial WHERE clauses.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cde2d764-d976-48a4-bf49-7af43840cb51
📒 Files selected for processing (5)
app/api/blockfest/cashback/route.tsapp/api/referral/claim/route.tsapp/api/referral/submit/route.tsapp/lib/kyc-identity.tssupabase/migrations/20260730120000_identity_scoped_referrals.sql
… address resolution - Added new functions to map embedded/EOA wallet addresses to their corresponding smart wallet addresses, facilitating accurate cashback claims. - Updated the cashback claim route to utilize these new mappings, ensuring claims are processed against the correct identity scope. - Improved error handling for identity resolution failures during cashback claims, enhancing user feedback and system reliability. This update strengthens the cashback claim process by bridging the gap between EOA and smart wallet address spaces.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/api/blockfest/cashback/route.ts (2)
290-295: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the pooled quota check atomic with claim reservation.
Both queries read only completed claims, then the pending claim is inserted later. Concurrent requests for wallets in the same identity scope can both pass these checks and exceed the 10-claim or $500 identity-wide limits. Reserve quota atomically in the database before transferring funds, and include pending reservations in the enforcement path.
Also applies to: 329-333
🤖 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/blockfest/cashback/route.ts` around lines 290 - 295, Update the pooled quota logic around the claim-count query and the corresponding amount check to reserve identity-wide quota atomically in the database before any funds transfer. Include existing pending reservations in the limit calculations, and ensure concurrent requests sharing scopeWallets cannot both pass the 10-claim or $500 checks; use the reservation result as the authoritative enforcement outcome.
290-295: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the limit contract from “per wallet” to “per identity.”
These filters now pool claims across
scopeWallets, but the constants, error messages, and response details still describe limits “per wallet.” Rename/update the user-facing contract so it matches the enforced identity-scoped behavior.Also applies to: 329-333
🤖 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/blockfest/cashback/route.ts` around lines 290 - 295, Update the cashback limit contract in the route’s limit-checking logic to consistently describe identity-scoped limits rather than wallet-scoped limits. Revise the relevant limit constants, error messages, and response details near the claim-count check and the additional block around the second referenced section, while preserving the existing enforcement across scopeWallets.
🧹 Nitpick comments (1)
app/lib/privy.ts (1)
82-112: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the Privy lookup fan-out.
This helper starts one
getUserByWalletAddressrequest per identity wallet viaPromise.all, and the cashback route invokes it for every claim. Large identity scopes can create a burst of external calls and turn rate limiting or latency into claim failures. Add bounded concurrency, caching, or a batch/server-side mapping; verify the deployed Privy SDK’s limits and batch support.🤖 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 82 - 112, Bound the external lookup fan-out in getSmartWalletAddressesForWallets instead of issuing every getUserByWalletAddress call concurrently via Promise.all. Use the deployed Privy SDK’s supported batching or a bounded-concurrency strategy, and reuse cached mappings where appropriate, while preserving deduplication and skipping wallets without a linked smart wallet.
🤖 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/blockfest/cashback/route.ts`:
- Around line 231-244: Move the existing claim lookup by transaction_id ahead of
the Step 7.5 identity-scope fan-out so retries return the stored claim without
requiring identity resolution. Update the surrounding flow in the route handler
to run getWalletAddressFromPrivyUserId, resolveIdentityScope, and
getSmartWalletAddressesForWallets only after confirming no existing claim was
found, while preserving the current new-claim behavior.
---
Outside diff comments:
In `@app/api/blockfest/cashback/route.ts`:
- Around line 290-295: Update the pooled quota logic around the claim-count
query and the corresponding amount check to reserve identity-wide quota
atomically in the database before any funds transfer. Include existing pending
reservations in the limit calculations, and ensure concurrent requests sharing
scopeWallets cannot both pass the 10-claim or $500 checks; use the reservation
result as the authoritative enforcement outcome.
- Around line 290-295: Update the cashback limit contract in the route’s
limit-checking logic to consistently describe identity-scoped limits rather than
wallet-scoped limits. Revise the relevant limit constants, error messages, and
response details near the claim-count check and the additional block around the
second referenced section, while preserving the existing enforcement across
scopeWallets.
---
Nitpick comments:
In `@app/lib/privy.ts`:
- Around line 82-112: Bound the external lookup fan-out in
getSmartWalletAddressesForWallets instead of issuing every
getUserByWalletAddress call concurrently via Promise.all. Use the deployed Privy
SDK’s supported batching or a bounded-concurrency strategy, and reuse cached
mappings where appropriate, while preserving deduplication and skipping wallets
without a linked smart wallet.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2143f67-3db9-4c21-adc1-6325a09eded5
📒 Files selected for processing (2)
app/api/blockfest/cashback/route.tsapp/lib/privy.ts
- Moved identity scope resolution logic to occur after checking for existing claims, ensuring that retries on processed transactions do not incur additional costs or errors. - Enhanced error handling for identity resolution failures, providing clearer feedback to users in case of issues during cashback claim verification. - Updated comments for clarity and to reflect the new order of operations in the cashback claim process. This update optimizes the cashback claim workflow by ensuring efficient identity resolution while maintaining user experience during claim retries.
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/api/blockfest/cashback/route.ts`:
- Around line 261-277: Update the cashback quota flow surrounding scopeWallets
so quota validation and pending-claim insertion execute atomically within one
database transaction. Acquire a stable advisory or equivalent lock derived from
the resolved identity scope before reading completed claims, then perform the
quota checks and insert the pending claim using that same transaction; ensure
concurrent sibling-wallet requests cannot bypass the shared count or $500 limit.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 076837d3-0e04-48d1-b6ca-4dbe6fabec20
📒 Files selected for processing (1)
app/api/blockfest/cashback/route.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
chibie
left a comment
chibie
left a comment
There was a problem hiding this comment.
Overview
This closes a real abuse hole: one person with several KYC-linked wallets could accept multiple referral codes (N welcome rewards) and reset the BlockFest cashback caps per wallet. It's fixed in three layers — identity-fingerprint columns + partial unique indexes on referrals/referral_claims, fingerprint stamping in the submit/claim routes, and pooling the cashback caps across an identity's sibling smart wallets via a new insert_cashback_claim_if_within_quota RPC.
The design is sound: fail-closed on identity lookup errors, isIdentityConflict correctly checked before the broader isUniqueViolation, the new ALREADY_REFERRED → 409 mapping, and the EOA→smart-wallet address-space bridge in app/lib/privy.ts is well reasoned and nicely documented. But there's one blocker.
🔴 Blocking
insert_cashback_claim_if_within_quota is not defined anywhere in the repo.
The cashback route now routes every claim through supabaseAdmin.rpc("insert_cashback_claim_if_within_quota", ...), but the PR's only migration (20260730120000_identity_scoped_referrals.sql) covers just the referral columns/indexes, and the function doesn't exist in any migration on main either. As written, every cashback claim will 500 with "function does not exist" — unless it was created by hand in the Supabase dashboard, in which case the schema drifts and other environments (local, staging, previews) are broken.
It needs to ship as a migration. Until it does, none of the load-bearing semantics the route depends on can be reviewed:
- which
statusvalues count toward the quota (does apendingrow from a failed transfer permanently consume allowance?) - the advisory-lock behavior over
p_identity_keys— the comment promises locks prevent concurrent siblings bypassing the cap, but that's unverifiable - the remaining-allowance adjustment math that used to live in TS
- whether
total_claimedcomes back as a JSON number —rpcData.total_claimed?.toFixed(2)throws if PostgREST hands back anumericas a string, which it commonly does - how a duplicate
transaction_idrace is surfaced (see below)
🟠 High
No backfill — existing rows don't participate in the new constraints.
All pre-migration referrals/referral_claims rows have NULL fingerprints, and the partial indexes exclude NULLs. So an identity that already collected its referee reward before this deploys can still be referred again through a fresh sibling wallet: the new row's fingerprint won't conflict with the old NULL row, and the double reward goes through. A backfill populating identity_phone/identity_id_key from user_kyc_profiles (joining on wallet_address) would close it for the existing user base — which is presumably where the actual abusers already are.
Worth noting the cashback side does not have this gap, since it pools by querying over the resolved sibling-wallet set at claim time rather than relying on stamped columns. That asymmetry makes the referral side's reliance on unstamped history stand out more.
Friendly duplicate-transaction handling was dropped from the cashback route.
The old insert path mapped a 23505 on transaction_id to "A claim for this transaction already exists. Please refresh and try again." The step-8 idempotency check is non-atomic, so two concurrent submissions of the same transaction can still race into the RPC — and unless the (missing) function handles that conflict explicitly, the loser now gets a raw 500 CLAIM_CREATION_FAILED instead. Whatever the RPC ends up being, it should return a distinguishable duplicate-transaction result that the route maps back to the old message.
🟡 Minor
isIdentityConflictmatches constraint names againsterror.message. Works with PostgREST today, but it's brittle — the message text is not a stable API. Gating onerror.code === "23505"first and checking bothmessageanddetailsfor the index names would be safer.- User-facing copy still says "per wallet" (
MAX_CLAIMS_REACHED,MAX_CASHBACK_REACHED) even though the caps are now pooled per identity. A user whose sibling wallet exhausted the cap gets told they hit their wallet limit having claimed nothing on this wallet. Confusing, and a support-ticket generator. getSmartWalletAddressesForWalletsdoes one Privy call per sibling, unbounded concurrency, on every claim. Fine at current sibling counts, but combined with fail-closed semantics one Privy hiccup or rate-limit blocks all cashback claims. Probably worth a retry or a short cache.- Indexes are built non-
CONCURRENTLY(unavoidable inside a transactional migration) — fine if these tables are small, just be aware writes block during the build. - No tests. The race-prone paths here — concurrent sibling claims, identity-conflict vs. same-claim-conflict disambiguation — are exactly what regresses silently.
Verdict
The approach is good and the referral-side changes look correct, but this isn't shippable while the RPC it depends on lives outside the repo. Add the function as a migration, ideally the fingerprint backfill alongside it, and this is in good shape.
chibie
left a comment
chibie
left a comment
There was a problem hiding this comment.
Line-level notes for the points in my earlier review, anchored to the code they refer to.
- Updated the cashback claim route to handle numeric fields as strings to preserve precision during serialization. - Improved error handling for duplicate transactions, providing clearer feedback to users when a claim already exists. - Enhanced messaging for maximum claims and cashback limits, clarifying that limits apply across all wallets linked to a verified identity. - Added logic to delete any pending claims if a malformed response is received, ensuring no allowance is consumed unnecessarily. This update optimizes the cashback claim workflow and improves user experience by providing clearer error messages and handling edge cases more effectively.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/lib/privy.ts (1)
96-102: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe cache never evicts expired entries.
smartWalletCachegrows with every distinct wallet address and keeps expired entries forever. On a long-lived server process the map retains one entry per wallet ever looked up. A bounded map, or a prune step on write, keeps memory flat.♻️ Proposed change: prune expired entries on write
const SMART_WALLET_CACHE_TTL_MS = 5 * 60 * 1000; +const SMART_WALLET_CACHE_MAX_ENTRIES = 5000; const smartWalletCache = new Map< string, { value: string | null; expiresAt: number } >(); + +function cacheSmartWallet( + address: string, + value: string | null, + now: number, +): void { + if (smartWalletCache.size >= SMART_WALLET_CACHE_MAX_ENTRIES) { + for (const [key, entry] of smartWalletCache) { + if (entry.expiresAt <= now) smartWalletCache.delete(key); + } + // Still full after pruning expired entries: drop the oldest insertion. + if (smartWalletCache.size >= SMART_WALLET_CACHE_MAX_ENTRIES) { + const oldest = smartWalletCache.keys().next().value; + if (oldest !== undefined) smartWalletCache.delete(oldest); + } + } + smartWalletCache.set(address, { + value, + expiresAt: now + SMART_WALLET_CACHE_TTL_MS, + }); +}Then replace the inline
smartWalletCache.set(...)at lines 119-122 withcacheSmartWallet(address, value, Date.now()).🤖 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 96 - 102, Prune expired entries whenever the smart-wallet cache is written so smartWalletCache does not retain addresses indefinitely. Add a cacheSmartWallet helper near the cache definitions that removes entries whose expiresAt is no longer valid before storing the new address/value with its TTL, then replace the inline smartWalletCache.set call in the lookup flow with cacheSmartWallet(address, value, Date.now()).
🤖 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/blockfest/cashback/route.ts`:
- Around line 330-338: Update the rpcResult handling before the typed property
reads in the cashback route to explicitly detect a null RPC payload and route it
through the existing CLAIM_CREATION_FAILED response path. Do not rely on the
type cast to provide runtime protection; preserve the current handling for
non-null RPC results and avoid allowing a null value to reach the first property
access.
In `@supabase/migrations/20260804120000_cashback_claim_quota_function.sql`:
- Around line 95-107: After calculating v_adjusted in the cashback claim
function, reject values equal to or below zero before inserting into
blockfest_cashback_claims by returning an amount_too_small error; retain the
existing insert and duplicate_transaction handling for positive amounts. Update
the route’s RPC error mapping to translate amount_too_small into a 400 response
instead of allowing the !rpcData.id fallback to produce a 500.
- Line 116: Update the privileges for
public.insert_cashback_claim_if_within_quota by revoking all PUBLIC access
before granting EXECUTE to service_role, ensuring anon and authenticated cannot
invoke the SECURITY DEFINER function through PostgREST.
- Around line 41-52: Update the SECURITY DEFINER function declaration for the
cashback claim quota function to include the function-level attribute SET
search_path = public, pg_temp. Remove the body-level set_config call while
preserving the existing function logic and unqualified table references.
---
Nitpick comments:
In `@app/lib/privy.ts`:
- Around line 96-102: Prune expired entries whenever the smart-wallet cache is
written so smartWalletCache does not retain addresses indefinitely. Add a
cacheSmartWallet helper near the cache definitions that removes entries whose
expiresAt is no longer valid before storing the new address/value with its TTL,
then replace the inline smartWalletCache.set call in the lookup flow with
cacheSmartWallet(address, value, Date.now()).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1af305c6-3b87-4a63-8ec8-6b08a7e5ab8f
📒 Files selected for processing (7)
app/api/blockfest/cashback/route.tsapp/api/referral/claim/route.tsapp/api/referral/submit/route.tsapp/lib/kyc-identity.tsapp/lib/privy.tssupabase/migrations/20260730120000_identity_scoped_referrals.sqlsupabase/migrations/20260804120000_cashback_claim_quota_function.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- app/api/referral/submit/route.ts
- app/api/referral/claim/route.ts
- Added checks for invalid transaction amounts in the cashback claim route to prevent NaN values from affecting the quota function. - Implemented error responses for cases where the transaction amount is too small to earn cashback, ensuring users receive clear feedback. - Updated the cashback claim quota function in the database to handle malformed amounts and prevent unnecessary claim slot consumption. These improvements strengthen the integrity of the cashback claim process and enhance user experience by providing clearer error messages and handling edge cases effectively.
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 `@supabase/migrations/20260804120000_cashback_claim_quota_function.sql`:
- Around line 58-63: Update the validation branch in the cashback quota function
around p_amount to explicitly reject all non-finite numeric values, including
NaN, positive Infinity, and negative Infinity, before the quota calculation.
Preserve the existing amount_too_small response for null, non-positive, or
non-finite inputs, and remove reliance on p_amount <> p_amount as the NaN check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cae0683-49b0-4ee0-a9a0-7e13881778df
📒 Files selected for processing (3)
app/api/blockfest/cashback/route.tsapp/lib/privy.tssupabase/migrations/20260804120000_cashback_claim_quota_function.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/privy.ts
- app/api/blockfest/cashback/route.ts
- Updated the cashback claim quota function to include additional checks for invalid amounts, specifically handling NaN and Infinity values. - Improved error handling to ensure that malformed amounts do not affect the claim process, providing clearer feedback to users. These changes strengthen the robustness of the cashback claim system by preventing invalid inputs from causing issues during processing.
|
@coderabbitai if all comments here have been addressed, mark them as resolved |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="paycrest/noblocks"
pr="659"
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
comments(first: 20) {
nodes {
author { login }
body
url
path
line
originalLine
}
}
}
}
}
}
}' \
--jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai" or .author.login == "coderabbitai[bot]"))
| {
id,
isResolved,
isOutdated,
comments: [
.comments.nodes[]
| select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]")
| {url, path, line, originalLine, body}
]
}
'Length of output: 25954
You are interacting with an AI system. |
chibie
left a comment
chibie
left a comment
There was a problem hiding this comment.
Re-review — 5345e71, 890ccc8, 1d1e757
Every item from the previous round is addressed, and the quota function is genuinely well built. Verified against the new code rather than the commit messages:
Blocker cleared. insert_cashback_claim_if_within_quota now ships as 20260804120000_cashback_claim_quota_function.sql, and the semantics I couldn't check before hold up: advisory locks are taken on sorted identity keys (no deadlock against insert_swap_transaction_if_within_limit, which uses the same hashtext(key) convention), pending + completed consume the allowance so the reservation exists before the transfer settles, and the locks are transaction-scoped so they're released when the RPC returns rather than being held across the on-chain transfer. REVOKE … FROM PUBLIC + GRANT … TO service_role on a SECURITY DEFINER function is the right call and easy to forget.
Backfill. Correctly ordered — backfill, then dedup, then CREATE UNIQUE INDEX — and the dedup keeps the payout rows while freeing the slot, with completed winning over pending/failed for claims. The normalization is genuinely consistent between resolveOwnIdentityFingerprint and the SQL (upper(btrim(…)) on all three id parts, whitespace stripped from the number only, NULLIF(btrim(phone), '')), which is the part most likely to silently diverge.
Also confirmed: duplicate_transaction → 409 with the original message restored; Number(…) coercion on every numeric field; the orphan row is deleted when the RPC response is malformed; isIdentityConflict gated on SQLSTATE 23505 first; IDENTITY_CHECK_FAILED in the submit route; cache + allSettled in getSmartWalletAddressesForWallets, and I checked the SDK typing — getUserByWalletAddress(): Promise<User | null>, so the "404 → null" comment the fail-closed logic rests on is accurate. The NaN/Infinity guards at both layers weren't asked for and are a good catch: p_amount <> p_amount genuinely does not catch NaN in Postgres.
I withdrew one finding I was about to raise: v_adjusted <= 0 looked like it could conflate "allowance exhausted" with "transaction too small", but since every stored amount is 2-decimal and the cap is an integer, the remaining allowance is always a multiple of 0.01 — so that branch is only reachable for genuinely sub-$0.50 transactions and the message is accurate.
Remaining comments are below. One is a gap in the threat model the PR sets out to close and I think is worth addressing before merge; the rest are operational hardening and two nits.
|
will address now |
Addresses the review feedback on #659. Identity-level self-referral guard (app/api/referral/claim/route.ts): the fingerprint indexes stop a sibling wallet collecting a second referee reward, but the referrer side stamps no fingerprint — by design, since a referrer legitimately earns on many referrals. That left one person able to refer their own sibling wallets and collect a referrer reward per wallet, which address-equality in submit/route.ts cannot see. Both sides now reject when the two parties resolve to the same identity scope, fail closed on lookup error, and map SELF_REFERRAL to 403. Placed after the completed-claim short-circuit so already-paid claims stay idempotent. Stranded pending reservations (20260804120100): 'pending' rows count against the pooled caps, and only the route's catch block releases them. An invocation killed mid-flight stranded its reservation permanently across every wallet sharing that identity. A pg_cron sweep now fails rows older than 30 minutes that never recorded a tx_hash. Also: CHECK constraint (NOT VALID) so an unparseable amount cannot make the quota function's amount::NUMERIC cast fail every claim for an identity; bounded the smart-wallet cache with TTL eviction on read and a size cap; corrected the search_path comment, which described pg_temp as excluded when listing it last is what demotes it behind public; and softened the 'failed' quota note, which implied a retry that Step 8's idempotency lookup does not allow. Tests: resolveOwnIdentityFingerprint was untested — added coverage for the normalization that must stay in sync with the migration backfill, plus the fail-closed contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
supabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sql (1)
31-33: 🩺 Stability & Availability | 🔵 TrivialVerify a nonblocking index deployment path.
If live cashback writes can occur during this migration, regular
CREATE INDEXblocks inserts, updates, and deletes until the build completes. Use a separate concurrent-index migration when the runner permits it. Otherwise deploy during a maintenance window with a lock-timeout plan. PostgreSQL documents that concurrent builds avoid write blocking but cannot run inside a transaction block. (postgresql.org)🤖 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/20260804120100_reap_stale_pending_cashback_claims.sql` around lines 31 - 33, Update the idx_cashback_claims_pending_reap deployment to use a concurrent index creation path when the migration runner permits statements outside transaction blocks; otherwise separate it into a compatible migration or document a maintenance-window and lock-timeout deployment plan. Preserve the existing partial-index predicate and target table.Source: Linters/SAST tools
🤖 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/referral/claim/route.ts`:
- Around line 382-391: The referral eligibility scope lookup must use canonical
identity keys so equivalent phone or ID values with differing casing or
whitespace cannot bypass self-referral detection. Update user_kyc_profiles
storage and queries used by resolveIdentityScope to normalize and persist
canonical phone and identity fields, backfill existing profiles, and add
divergent-spelling coverage for resolveIdentityScope; keep
resolveOwnIdentityFingerprint’s later insert normalization separate.
In `@supabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sql`:
- Around line 16-22: The stale-claim reaper must not use a null tx_hash alone to
release quota, because a transfer may have been broadcast before persistence
failed. Add and persist a durable pre-broadcast state before the cashback route
calls writeContract, then update the reaper to select only claims that never
reached that state. Reconcile claims marked as broadcast-attempted before
marking them failed or releasing shared quota.
---
Nitpick comments:
In `@supabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sql`:
- Around line 31-33: Update the idx_cashback_claims_pending_reap deployment to
use a concurrent index creation path when the migration runner permits
statements outside transaction blocks; otherwise separate it into a compatible
migration or document a maintenance-window and lock-timeout deployment plan.
Preserve the existing partial-index predicate and target table.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f868447-3aab-4c19-8f1a-3b4c93c3e0e1
📒 Files selected for processing (5)
__tests__/kycIdentity.test.tsapp/api/referral/claim/route.tsapp/lib/privy.tssupabase/migrations/20260804120000_cashback_claim_quota_function.sqlsupabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/privy.ts
Addresses both CodeRabbit findings on the previous commit.
Canonical ID key (20260805120000, app/lib/kyc-identity.ts): the id_*
columns hold raw input — smile-id/route.ts falls back to the
client-supplied number — but resolveIdentityScope matched them with exact
equality, so one document stored two ways ("NG:PASSPORT:A123456" vs
"ng:passport:a 123 456") failed to pool. Everything downstream inherited
it: separate monthly allowances, separate cashback allowances, siblings
serializing on different advisory locks (reopening the race the quota
function exists to close), and the new self-referral guard missing a
referrer that is really the referee's own wallet. Sibling lookups and lock
keys now go through a generated identity_id_key column, with the single TS
copy of the expression extracted as buildIdentityIdKey. Generated rather
than written so it cannot drift and needs no backfill. Phone was already
E.164-canonical on write and is unchanged.
The KYC write path's own per-document uniqueness checks still compare the
raw triple; retargeting those would tighten enrollment, which is a KYC
behaviour change that does not belong here. Noted in the migration.
Pre-broadcast marker (20260804120100, cashback route): tx_hash is only
persisted after writeContract returns, so a null hash did not prove the
transfer never happened — the reaper could release quota for a claim that
was actually paid. The route now stamps transfer_attempted_at immediately
before broadcasting and aborts if that write fails; the sweep reaps only
unstamped rows. Stamped-but-hashless rows keep their reservation for
manual reconciliation, which is the conservative side to err on.
Note for deploy: 20260805120000 must be applied BEFORE the code ships —
resolveIdentityScope filters on the new column. Old code is fine against
the new schema, so migrate-then-deploy is the safe order.
Tests: divergent-spelling coverage for resolveIdentityScope (asserting the
query uses the canonical column and no raw-triple filter survives) plus
buildIdentityIdKey units. 344 pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/blockfest/cashback/route.ts`:
- Around line 492-512: Update the writeContract error handling in the cashback
claim flow so a claim with transfer_attempted_at set is not marked failed after
an indeterminate broadcast error. Keep the claim pending or assign the existing
reconciliation status until a chain lookup confirms that no transfer occurred,
while preserving failure handling for errors proven to occur before broadcast.
In `@supabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sql`:
- Around line 39-46: Before enabling the stale-claim reaper, protect existing
pending claims whose new transfer_attempted_at value is NULL: reconcile them
manually and stamp the marker or otherwise exclude them from the reaper
predicate. Update the migration’s initialization/scheduling flow around
blockfest_cashback_claims so only claims created after marker introduction are
eligible for automatic failure, preserving historical reservations until
reconciliation.
In `@supabase/migrations/20260805120000_canonical_identity_id_key.sql`:
- Around line 63-69: The generated-column expression in
supabase/migrations/20260805120000_canonical_identity_id_key.sql lines 63-69
must trim the same whitespace characters as buildIdentityIdKey. Update country
and type normalization to remove surrounding whitespace including tabs, retain
number whitespace removal, and keep the resulting key aligned with the
TypeScript helper. In app/lib/kyc-identity.ts lines 97-99, preserve exact parity
with the SQL expression and add a regression case covering tab-padded country
and type values.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4abdfb4e-b29f-4a07-8a64-942515d98f91
📒 Files selected for processing (5)
__tests__/kycIdentity.test.tsapp/api/blockfest/cashback/route.tsapp/lib/kyc-identity.tssupabase/migrations/20260804120100_reap_stale_pending_cashback_claims.sqlsupabase/migrations/20260805120000_canonical_identity_id_key.sql
…ation Addresses the three findings on fdac43f. Indeterminate broadcast (cashback route): the marker stopped the *reaper* releasing quota for a possibly-paid claim, but the route's own catch still set 'failed' unconditionally — which releases the same reservation. A node can accept and broadcast a transfer while the client loses the response, so an error out of writeContract proves nothing. The catch now only marks 'failed' for errors thrown before the broadcast (token lookup, RPC config, key validation, the marker write itself); once writeContract is entered the claim stays 'pending' with its reservation and is logged for manual review. Consistent with how the reaper already treats stamped rows. Pre-marker rows (20260804120100): adding the column leaves every existing pending claim NULL, and for those NULL means "unknown", not "never broadcast" — the marker did not exist when they ran. The sweep would have treated exactly the unknowable rows as safe. They are now stamped with their created_at at migration time, excluding them until a human clears them. SQL/TS whitespace parity (20260805120000, 20260730120000, kyc-identity): btrim() with no second argument strips spaces only, so "\tng\t" canonicalized to NG in the app and to a tab-padded key in the generated column — one identity, two keys, defeating the point. All three copies now strip exactly [[:space:]] (the six ASCII whitespace characters), applied throughout rather than at the ends so internal spacing collapses too. The class is spelled out rather than using \s or trim(), which in JS also match NBSP and the Unicode separators that Postgres leaves alone. Tests: tab/newline/vertical-tab parity regression. 345 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Description
resolveOwnIdentityFingerprintandresolveIdentityScopefunctions to ensure referral and cashback claims are validated against a user's verified identity across multiple wallets.This update strengthens the integrity of the referral and cashback systems by enforcing identity-based limits on claims.
References
closes https://paycrest-io.atlassian.net/browse/KAN-722
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Bug Fixes
Data Integrity