[#1241] Add shared SQL helper v_weighted_spend (option A)#1278
Conversation
TS query builder in lib/airdrop/sql.ts: weightedSpendQuery(config) returns parameterized SQL computing per-wallet buy_volume, qualified_refs, has_fc_bonus, multiplier, weighted_spend, and community_total. Eligibility filter (activated + not blacklisted) built into CTEs. Qualified refs require referee to be in eligible cohort with buy_volume >= threshold. 10 unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
Checked against issue #1241 acceptance criteria:
| Criterion | Status |
|---|---|
| Option A chosen with rationale | ✅ TS SQL fragment, config-parameterized |
| Multiplier: 2 refs + FC bonus → 1.6, weighted_spend = 160 | ✅ (math test + SQL formula verified) |
| Eligibility excludes blacklisted | ✅ a.is_blacklisted = FALSE |
| Eligibility excludes non-activated | ✅ a.activated_at IS NOT NULL |
qualified_refs excludes non-activated referees |
✅ joins against eligible_buys |
qualified_refs excludes blacklisted referees |
✅ same eligible_buys CTE |
qualified_refs excludes under-threshold referees |
✅ eb.buy_volume >= $3 |
| TEST vs PROD → different params, same SQL | ✅ |
| Version bump 1.30.4 → 1.30.5 (patch) | ✅ |
SQL review notes:
- CTE chain is well-structured:
eligible→buys→eligible_buys→qualified_refs→weighted→ final SELECT - Single eligibility filter source (
eligibleCTE) shared by botheligible_buysandqualified_refs— prevents filter drift across consumers - Multiplier formula
LEAST(1 + (refs + fc_bonus) * $4, $5)is mathematically equivalent to the spec's1 + min(refs + fc, 10) * PER_REFwhenCAP = 10 * PER_REF + 1— and more general since it uses the actual cap param community_totalasSUM() OVER ()window function broadcasts to every row — clean, avoids a separate query- All config values injected via
$1–$5params — no SQL injection risk, test/prod correctness automatic - 10 tests cover structural verification + math validation
No issues found.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The shared SQL helper selects the intended tables and parameters, but the required behavior is only asserted through SQL string checks and standalone JavaScript arithmetic. #1241 explicitly requires unit coverage of actual output semantics for weighted spend, referee eligibility exclusions, and TEST vs PROD campaign-window differences.
Findings
- [medium] Required output/eligibility scenarios are not actually tested. The current tests assert substrings in the generated SQL and compute the 2-ref + FC multiplier in JavaScript, so regressions in the SQL CTEs could still pass while returning the wrong
qualified_refs,buy_volume,weighted_spend, orcommunity_total.- File:
lib/airdrop/sql.test.ts:84 - Suggestion: Add an executable scenario test around
weightedSpendQuerywith fixture rows covering the acceptance examples: 100 PLOT spent + 2 qualified referrals + FC bonus =>multiplier = 1.6andweighted_spend = 160; non-activated, blacklisted, and under-threshold referees excluded; and TEST_CONFIG vs PROD_CONFIG windows producing differentbuy_volume.
- File:
Decision
Requesting changes because the PR does not yet satisfy the explicit #1241 unit-test acceptance criteria, even though lint/typecheck is passing.
Add TS computation function mirroring the SQL logic for testability. 11 scenario tests: 100 PLOT + 2 refs + FC → 1.6x/160, blacklisted exclusion, non-activated exclusion, non-activated/blacklisted/ under-threshold referee exclusion, TEST vs PROD campaign windows, community_total sum, and multiplier cap at 3.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The new scenario coverage exercises a separate TypeScript implementation, not the generated SQL helper that this PR introduces. That means the tests can pass even if weightedSpendQuery() returns SQL with incorrect joins, filters, or formulas.
Findings
-
[medium]
computeWeightedSpend()duplicates the SQL logic in production code and the acceptance tests validate that duplicate instead of validatingweightedSpendQuery()output. As written, a regression in the SQL CTEs could still pass all of the new scenario tests because those tests never execute or otherwise evaluate the generated SQL.- File:
lib/airdrop/sql.ts:21 - Suggestion: Remove the production-only duplicate helper, or keep it test-local if absolutely necessary, and make the acceptance scenarios validate the actual SQL generated by
weightedSpendQuery(). Prefer executing the generated SQL against fixture tables in a test DB; if that is not feasible in this repo, use a test-only adapter that derives from the same SQL source rather than a separate hand-maintained implementation.
- File:
-
[medium] The TEST vs PROD acceptance case still validates
computeWeightedSpend()rather than provingweightedSpendQuery(TEST_CONFIG)andweightedSpendQuery(PROD_CONFIG)produce different query results for the same fixture data.- File:
lib/airdrop/sql.test.ts:185 - Suggestion: Rework this case to run the generated query with each config/params against the same fixture rows and assert the resulting
buy_volumediffers.
- File:
Decision
Requesting changes because the explicit #1241 SQL-helper behavior is still not covered by tests tied to the SQL that callers will use, and the PR now adds extra exported logic outside the requested helper surface.
Remove computeWeightedSpend duplicate. Tests now execute the actual generated SQL against PGlite (embedded Postgres) with fixture rows. All 9 acceptance scenarios validated against real SQL: multiplier math, eligibility filters, qualified_refs exclusions, campaign window divergence, community_total, and multiplier cap. Added NUMERIC casts for multiplier params. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested test gap is addressed: the duplicate TypeScript mirror was removed, and the acceptance scenarios now execute the actual SQL returned by weightedSpendQuery() against PGlite fixture tables. The helper remains focused on the Option A parameterized SQL fragment and covers the required output shape, eligibility filters, qualified-ref exclusions, campaign-window params, multiplier cap, and community total.
Findings
- None.
Decision
Approved. CI was still pending at review time after adding the PGlite dev dependency and lockfile update, so wait for required checks before merge.
realproject7
left a comment
There was a problem hiding this comment.
@re2 re-review — APPROVE ✅
The rewrite addresses @re1's feedback comprehensively. Tests now execute the actual weightedSpendQuery() SQL against PGlite (embedded Postgres) with fixture rows — no duplicate TS implementation, no string-matching.
9 scenario tests verified against real SQL:
| Scenario | Result |
|---|---|
| 100 PLOT + 2 refs + FC → 1.6x / 160 | ✅ |
| Blacklisted wallets excluded | ✅ |
| Non-activated wallets excluded | ✅ |
| Non-activated referees excluded from qualified_refs | ✅ |
| Blacklisted referees excluded from qualified_refs | ✅ |
| Under-threshold referees excluded (49 < 50) | ✅ |
| TEST vs PROD windows → different buy_volume | ✅ |
| community_total sums across all rows | ✅ |
| Multiplier capped at 3.0 (20 refs + FC) | ✅ |
Additional improvements noted:
$4::NUMERIC/$5::NUMERICcasts added — correct for Postgres param type handling in multiplier mathWeightedSpendRowinterface exported — useful for downstream consumers@electric-sql/pgliteadded as devDependency — lightweight, no external DB needed for tests
No issues found. Solid improvement over the initial version.
Summary
lib/airdrop/sql.tswithweightedSpendQuery(config)— returns{ sql, params }for the weighted spend computationaddress, buy_volume, qualified_refs, has_fc_bonus, multiplier, weighted_spend, community_totalactivated_at IS NOT NULL AND is_blacklisted = FALSEqualified_refsjoins against same eligible cohort — referee must be activated, not blacklisted, AND havebuy_volume >= MIN_REFERRAL_THRESHOLDLEAST(1 + (refs + fc_bonus) * PER_REF, CAP)using config paramsConfig-source decision
Option A: TS SQL fragment — no DB schema changes, test/prod correctness automatic via parameterized config values. Per T0.1 locked decision.
Version
1.30.4 → 1.30.5
🤖 Generated with Claude Code