Conversation
🦋 Changeset detectedLatest commit: 5f5835f The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces per-edition leaderboard caches with edition snapshots and adds per-event rev-share-cap accounting traces, a CSV export endpoint, enriched referral event fields, snapshot-aware builders/middleware, prerequisite checks, CSV formatter, tests, and OpenAPI docs. ChangesReferral Edition Snapshot + Accounting
Sequence DiagramsequenceDiagram
participant Client
participant API as ENSAPI Handler
participant Cache as Snapshot Cache
participant DB as Database
participant Builder as AwardModel Builder
participant Formatter as CSV Formatter
Client->>API: GET /v1/ensanalytics/accounting?edition=slug
API->>API: check ensAnalyticsPrerequisites
API->>Cache: cache.read(edition)
alt Cache returns Snapshot
Cache-->>API: ReferralEditionSnapshot
else Cache builds Snapshot
Cache->>DB: getReferralEvents(edition)
DB-->>DB: enrich events (name, actionType, txHash, registrant)
DB-->>Builder: ReferralEvent[]
Builder->>Builder: compute leaderboard + accountingRecords
Builder-->>Cache: store ReferralEditionSnapshot
Cache-->>API: ReferralEditionSnapshot
end
API->>API: assert awardModel == RevShareCap
API->>Formatter: formatAccountingCsv(accountingRecords)
Formatter-->>API: CSV string
API-->>Client: 200 text/csv (CSV payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the ENS referrals “rev-share-cap” pipeline to produce a per-event award accounting trace, exposes that trace via a new ENSApi CSV endpoint, and refactors leaderboard builders to return an edition “snapshot” object.
Changes:
- Introduce
ReferralEditionSnapshotbuilders (buildReferralEditionSnapshot*) bundling leaderboards with additional computed state (rev-share-cap includes per-event accounting records). - Add rev-share-cap accounting types + tests, and extend
ReferralEventto carry audit fields needed for reporting/export. - Add
GET /v1/ensanalytics/accounting?edition={slug}returning a CSV dump of per-event accounting for rev-share-cap editions.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ens-referrals/src/leaderboard.ts | Exposes new ReferralEditionSnapshot union alongside leaderboards. |
| packages/ens-referrals/src/index.ts | Re-exports rev-share-cap accounting types. |
| packages/ens-referrals/src/award-models/rev-share-cap/referral-event.ts | Extends ReferralEvent with name/action/txHash/registrant for audit/export. |
| packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts | Refactors rev-share-cap builder to return a snapshot + emits per-event accounting records. |
| packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.test.ts | Updates tests for the new snapshot-returning builder. |
| packages/ens-referrals/src/award-models/rev-share-cap/accounting.ts | Adds per-event accounting record + award trace interfaces. |
| packages/ens-referrals/src/award-models/rev-share-cap/accounting.test.ts | Adds targeted tests for per-event accounting trace behavior. |
| packages/ens-referrals/src/award-models/pie-split/leaderboard.ts | Refactors pie-split builder to return a snapshot wrapper. |
| apps/ensapi/src/middleware/referral-edition-snapshots-caches.middleware.ts | Renames/rewires middleware to provide snapshot caches instead of leaderboards. |
| apps/ensapi/src/lib/hono-factory.ts | Updates middleware variable typing to new snapshot-caches middleware. |
| apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/get-referral-edition-snapshot.ts | Replaces leaderboard builder dispatcher with snapshot builder dispatcher. |
| apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/get-referral-edition-snapshot.test.ts | Updates tests for snapshot return shape. |
| apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts | Adds CSV formatter for rev-share-cap accounting records. |
| apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.ts | Extends rev-share-cap event query to fetch audit fields + domain name. |
| apps/ensapi/src/index.ts | Updates graceful shutdown to destroy snapshot caches. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.ts | Uses snapshot caches for existing endpoints and adds /accounting CSV endpoint. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.ts | Updates cache mocks for snapshot shape + adds basic accounting endpoint tests. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.ts | Adds OpenAPI route definition for /accounting CSV endpoint. |
| apps/ensapi/src/cache/referral-edition-snapshots.cache.ts | Refactors cache to store snapshots (and use new builder). |
| .changeset/shiny-pandas-account.md | Changeset for ensapi: adds accounting CSV endpoint. |
| .changeset/purple-frogs-dream.md | Changeset for ens-referrals: adds snapshot + per-event accounting trace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ensapi/src/index.ts (1)
61-61:⚠️ Potential issue | 🟡 MinorStale log label after rename.
The log message still references the pre-rename
referralLeaderboardEditionsCache. For consistency with the rest of the PR-wide migration to snapshot-based caching (e.g., the import on Line 6 and the variableeditionsCaches), update the label to reflect the new name.📝 Suggested fix
- logger.info(`Destroyed referralLeaderboardEditionsCache for ${editionSlug}`); + logger.info(`Destroyed referralEditionSnapshotsCache for ${editionSlug}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/index.ts` at line 61, The log message uses an outdated label "referralLeaderboardEditionsCache"; update the logger.info call that references editionSlug to use the new snapshot-based name (e.g., "editionsCaches" or "editionSnapshots" consistent with the import and variable `editionsCaches`) so the message matches the PR-wide rename—find the logger.info(`Destroyed referralLeaderboardEditionsCache for ${editionSlug}`) and change the text to reflect the new cache name.apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.ts (1)
134-159:⚠️ Potential issue | 🟠 MajorReplace
innerJoinwithleftJointo detect missing data explicitly instead of silently dropping referral events.The query chains
registrarActions → registrationLifecycles (on node) → subgraph_domain, both viainnerJoin. This means anyregistrarActionrow without a correspondingregistrationLifecyclesorsubgraph_domain.namewill vanish from results without surfacing an error. The runtime checks in the map function (lines 164–172) only validate rows the query returns; they cannot catch rows silently dropped by the joins.While the schema documentation states registrationLifecycles are guaranteed to exist and
subgraph_domain.namewill "in practice never be null," these guarantees are not enforced at the schema level. If an indexer bug or data gap violates these assumptions, the leaderboard will silently omit referral events rather than fail loudly.Consider using
leftJoinon both tables and adding explicit error throws if the joins fail to produce expected rows. This ensures data integrity is validated explicitly and any gaps surface clearly rather than causing silent revenue loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.ts` around lines 134 - 159, Change the two innerJoin calls on ensIndexerSchema.registrationLifecycles and ensIndexerSchema.subgraph_domain to leftJoin so registrarActions rows are not silently dropped, then after executing the query (the mapping logic that currently validates rows returned by the query) add explicit null checks for registrationLifecycles and subgraph_domain (e.g., verify the joined lifecycle and domain.name exist for each row) and throw a descriptive error when either is missing so missing joins surface loudly; adjust any downstream logic that assumed non-null values accordingly (references: the query built from ensIndexerSchema.registrarActions, the joins to ensIndexerSchema.registrationLifecycles and ensIndexerSchema.subgraph_domain, and the post-query mapping/validation code that checks decodedReferrer).apps/ensapi/src/cache/referral-edition-snapshots.cache.ts (1)
41-178: 🧹 Nitpick | 🔵 TrivialStale "leaderboard" wording in log messages and JSDoc after the snapshot rename.
The cache now produces and caches
ReferralEditionSnapshot(which contains bothleaderboardandaccountingRecords), but the human-readable strings throughout this file still say "leaderboard". This hurts grep-ability/observability (the logger category isreferral-edition-snapshots-cachebut the messages talk about leaderboards) and the JSDoc no longer accurately describes the cache contents.Affected spots: lines 41–44 (JSDoc), 57 (
@returns), 87, 90, 97, 107, 112, 125, 168, and 177.♻️ Suggested wording updates
/** * The list of {`@link` OmnichainIndexingStatusId} values that are supported for generating - * referrer leaderboards. + * referral edition snapshots. * - * Other values indicate that we are not ready to generate leaderboards yet. + * Other values indicate that we are not ready to generate edition snapshots yet. */ @@ - * `@returns` A function that builds the leaderboard for the given edition + * `@returns` A function that builds the {`@link` ReferralEditionSnapshot} for the given edition @@ - logger.error( - { error: indexingStatus, editionSlug }, - `Failed to read indexing status cache while generating referral leaderboard for ${editionSlug}. Cannot proceed without valid indexing status.`, - ); - throw new Error( - `Unable to generate referral leaderboard for ${editionSlug}. indexingStatusCache must have been successfully initialized.`, - ); + logger.error( + { error: indexingStatus, editionSlug }, + `Failed to read indexing status cache while generating referral edition snapshot for ${editionSlug}. Cannot proceed without valid indexing status.`, + ); + throw new Error( + `Unable to generate referral edition snapshot for ${editionSlug}. indexingStatusCache must have been successfully initialized.`, + ); @@ - `Unable to generate referrer leaderboard for ${editionSlug}. Omnichain indexing status is currently ${omnichainIndexingStatus} but must be ${supportedOmnichainIndexingStatuses.join(" or ")}.`, + `Unable to generate referral edition snapshot for ${editionSlug}. Omnichain indexing status is currently ${omnichainIndexingStatus} but must be ${supportedOmnichainIndexingStatuses.join(" or ")}.`, @@ - `Unable to generate referrer leaderboard for ${editionSlug}. Latest indexed block ref for chain ${editionConfig.rules.subregistryId.chainId} is null.`, + `Unable to generate referral edition snapshot for ${editionSlug}. Latest indexed block ref for chain ${editionConfig.rules.subregistryId.chainId} is null.`, @@ - `Building referrer leaderboard for ${editionSlug} with rules:\n${JSON.stringify( + `Building referral edition snapshot for ${editionSlug} with rules:\n${JSON.stringify( @@ - `Successfully built referrer leaderboard for ${editionSlug} with ${snapshot.leaderboard.referrers.size} referrers`, + `Successfully built referral edition snapshot for ${editionSlug} with ${snapshot.leaderboard.referrers.size} referrers and ${snapshot.accountingRecords.length} accounting records`, @@ - logger.info(`Initialized leaderboard cache for ${editionSlug}`); + logger.info(`Initialized referral edition snapshot cache for ${editionSlug}`); @@ /** - * Gets the cached instance of referral leaderboard editions caches. + * Gets the cached instance of referral edition snapshots caches. * Returns null if not yet initialized.As per coding guidelines: "Do not add JSDoc
@returnstags that merely restate the method summary; remove such redundancy during PR review." If the updated@returnsoncreateEditionSnapshotBuilderends up being just a restatement of the summary, drop it instead of rewording.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/cache/referral-edition-snapshots.cache.ts` around lines 41 - 178, Update wording to refer to "ReferralEditionSnapshot" / "edition snapshot" instead of "leaderboard" in the JSDoc and all log messages: adjust the JSDoc for createEditionSnapshotBuilder to describe that the cache holds ReferralEditionSnapshot (remove the redundant `@returns` if it only repeats the summary), change logger.debug, logger.info and error strings inside createEditionSnapshotBuilder to say "snapshot" or "edition snapshot" (e.g., the debug that currently says "Edition is immutably closed, returning cached data without re-fetching", the info that logs the rules, and the info after building that references snapshot.leaderboard.referrers.size should say snapshot/edition snapshot and can still reference snapshot.leaderboard.referrers.size in code), and change the initializeReferralEditionSnapshotsCaches init log ("Initialized leaderboard cache for") to "Initialized edition snapshot cache for" (or similar); ensure the references to functions/classes remain the same: createEditionSnapshotBuilder, initializeReferralEditionSnapshotsCaches, cachedInstance, and getReferralEditionSnapshot so the replacements are localized to message/JSDoc strings only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.ts`:
- Around line 987-997: The helper mockSnapshotCache returns an object cast to
ReferralEditionSnapshot but omits required fields like leaderboard and
accountingRecords (required by ReferralEditionSnapshotPieSplit and
ReferralEditionSnapshotRevShareCap); update mockSnapshotCache to either (a)
document this limitation with a brief comment stating it intentionally produces
minimal snapshots only valid for early-exit awardModel checks, or (b) extend its
signature to accept optional partial overrides for leaderboard and
accountingRecords and merge them into the returned object so the mock can
produce structurally valid ReferralEditionSnapshot instances when tests exercise
success paths; reference mockSnapshotCache, ReferralEditionSnapshot,
ReferralEditionSnapshotPieSplit, ReferralEditionSnapshotRevShareCap, and
SWRCache when making the change.
- Around line 986-1034: Add tests to cover the /accounting success and
cache-error 503 paths: reuse mockSnapshotCache and setupAccountingMocks to
inject a cache map via
editionsCachesMiddleware.referralEditionSnapshotsCachesMiddleware; add a test
where mockSnapshotCache returns awardModel
ReferralProgramAwardModels.RevShareCap and the mocked
ReferralEditionSnapshot.read() resolves to an object with accountingRecords
(assert response status 200, Content-Type "text/csv; charset=utf-8",
Content-Disposition includes filename "accounting-${edition}.csv", and body
contains CSV header row plus one sample record row produced by
formatAccountingCsv); add another test where setupAccountingMocks injects a map
or cache whose value is an Error or whose read() resolves to an Error and assert
the endpoint returns 503. Ensure you reference the
app.request("/accounting?edition=...") call and the
ReferralEditionSnapshot.read() behavior when stubbing responses.
In `@apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.ts`:
- Around line 338-383: Add response caching headers, operational logging, and
avoid building the entire CSV in memory: after the rev-share check and before
returning the body in getAccountingCsvRoute, log the edition and record count
via logger.info({ edition, recordCount: cached.accountingRecords.length }), and
set a long-lived Cache-Control (e.g. "public, max-age=31536000, immutable") when
the snapshot is known immutable (use the cached snapshot metadata/flag available
on the cached object), by calling c.header("Cache-Control", "..."); finally
replace the immediate c.body(formatAccountingCsv(cached.accountingRecords), 200)
with a streaming response that converts cached.accountingRecords into CSV rows
(e.g. a ReadableStream or Node stream) so you don’t build the whole CSV in
memory — reuse formatAccountingCsv for row formatting or implement an async
iterator that writes rows to the stream and call c.body(stream, 200); keep
existing error handling and headers (Content-Type/Content-Disposition) intact.
In `@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.ts`:
- Around line 174-184: The exhaustiveness check using "const _exhaustive: never
= record.actionType" is unsafe if the DB-typed column widens; instead perform a
runtime validation against the known enum values by checking
Object.values(RegistrarActionTypes).includes(record.actionType as
RegistrarActionType) in the switch/default path (or before the switch) and, if
it returns false, throw the same Error (the one referencing getReferralEvents
and record.id); remove the _exhaustive assignment so the guard is a pure runtime
check independent of static narrowing.
In
`@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts`:
- Around line 74-82: The CSV produced by formatAccountingCsv can contain
non-ASCII ENS names and should include a UTF-8 BOM to avoid Excel mojibake;
update formatAccountingCsv so it prepends "\uFEFF" before the header row (i.e.,
before CSV_COLUMNS.map((c) => c.header).join(",")) while leaving the rest of the
join logic intact and still returning the final string with "\r\n" line endings;
reference CSV_COLUMNS and formatAccountingCsv to locate where to add the BOM.
- Around line 4-13: The CSV-escaping helper csvCell should be applied to every
exported cell to avoid future CSV injection/corruption; update the CSV writer so
that all output values (not just name and disqualificationReason) are passed
through csvCell — e.g., wrap actionType, hex/hash/address columns,
integer/string counts, and any other columns the formatter emits with csvCell
before joining/printing — leaving csvCell implementation unchanged and ensuring
the join logic still produces the same column order.
In `@packages/ens-referrals/src/award-models/rev-share-cap/accounting.test.ts`:
- Around line 264-287: Update the test description and inline comment to remove
references to a "wrapper" or "with-accounting function" and reflect that the
test now directly verifies the leaderboard returned by
buildReferralEditionSnapshotRevShareCap; rename the it(...) string to something
like "leaderboard in snapshot matches expected spot-checks" and delete or
replace the misleading comment "// Verify that the wrapper and the
with-accounting function produce the same leaderboard." so the test (using
leaderboard and rules variables and function
buildReferralEditionSnapshotRevShareCap) clearly documents it's spot-checking
fields of the returned leaderboard.
In `@packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts`:
- Around line 80-89: The new interface ReferralEditionSnapshotRevShareCap lacks
per-field JSDoc consistent with its sibling ReferrerLeaderboardRevShareCap;
update ReferralEditionSnapshotRevShareCap to add JSDoc on each field: document
awardModel (assert it equals leaderboard.awardModel and rules.awardModel),
document leaderboard (type ReferrerLeaderboardRevShareCap and that its
awardModel must match), and document accountingRecords (assert its length equals
the number of processed onchain events and that entries are in chronological
onchain order), mirroring the detailed `@invariant-style` comments used for
ReferrerLeaderboardRevShareCap so the file’s comment style stays consistent.
In `@packages/ens-referrals/src/leaderboard.ts`:
- Around line 17-22: Add a short doc-comment to ReferralEditionSnapshot
mirroring ReferrerLeaderboard that notes the union is discriminated by the
awardModel field; update the comment above the ReferralEditionSnapshot type to
mention "Discriminated by awardModel" (or similar) so consumers know to switch
on awardModel when handling ReferralEditionSnapshot variants like
ReferralEditionSnapshotPieSplit and ReferralEditionSnapshotRevShareCap.
---
Outside diff comments:
In `@apps/ensapi/src/cache/referral-edition-snapshots.cache.ts`:
- Around line 41-178: Update wording to refer to "ReferralEditionSnapshot" /
"edition snapshot" instead of "leaderboard" in the JSDoc and all log messages:
adjust the JSDoc for createEditionSnapshotBuilder to describe that the cache
holds ReferralEditionSnapshot (remove the redundant `@returns` if it only repeats
the summary), change logger.debug, logger.info and error strings inside
createEditionSnapshotBuilder to say "snapshot" or "edition snapshot" (e.g., the
debug that currently says "Edition is immutably closed, returning cached data
without re-fetching", the info that logs the rules, and the info after building
that references snapshot.leaderboard.referrers.size should say snapshot/edition
snapshot and can still reference snapshot.leaderboard.referrers.size in code),
and change the initializeReferralEditionSnapshotsCaches init log ("Initialized
leaderboard cache for") to "Initialized edition snapshot cache for" (or
similar); ensure the references to functions/classes remain the same:
createEditionSnapshotBuilder, initializeReferralEditionSnapshotsCaches,
cachedInstance, and getReferralEditionSnapshot so the replacements are localized
to message/JSDoc strings only.
In `@apps/ensapi/src/index.ts`:
- Line 61: The log message uses an outdated label
"referralLeaderboardEditionsCache"; update the logger.info call that references
editionSlug to use the new snapshot-based name (e.g., "editionsCaches" or
"editionSnapshots" consistent with the import and variable `editionsCaches`) so
the message matches the PR-wide rename—find the logger.info(`Destroyed
referralLeaderboardEditionsCache for ${editionSlug}`) and change the text to
reflect the new cache name.
In `@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.ts`:
- Around line 134-159: Change the two innerJoin calls on
ensIndexerSchema.registrationLifecycles and ensIndexerSchema.subgraph_domain to
leftJoin so registrarActions rows are not silently dropped, then after executing
the query (the mapping logic that currently validates rows returned by the
query) add explicit null checks for registrationLifecycles and subgraph_domain
(e.g., verify the joined lifecycle and domain.name exist for each row) and throw
a descriptive error when either is missing so missing joins surface loudly;
adjust any downstream logic that assumed non-null values accordingly
(references: the query built from ensIndexerSchema.registrarActions, the joins
to ensIndexerSchema.registrationLifecycles and ensIndexerSchema.subgraph_domain,
and the post-query mapping/validation code that checks decodedReferrer).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ab1fd3ce-fb34-46b8-ab77-4d0356b0aeb9
📒 Files selected for processing (21)
.changeset/purple-frogs-dream.md.changeset/shiny-pandas-account.mdapps/ensapi/src/cache/referral-edition-snapshots.cache.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.tsapps/ensapi/src/index.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/get-referral-edition-snapshot.test.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/get-referral-edition-snapshot.tsapps/ensapi/src/lib/hono-factory.tsapps/ensapi/src/middleware/referral-edition-snapshots-caches.middleware.tspackages/ens-referrals/src/award-models/pie-split/leaderboard.tspackages/ens-referrals/src/award-models/rev-share-cap/accounting.test.tspackages/ens-referrals/src/award-models/rev-share-cap/accounting.tspackages/ens-referrals/src/award-models/rev-share-cap/leaderboard.test.tspackages/ens-referrals/src/award-models/rev-share-cap/leaderboard.tspackages/ens-referrals/src/award-models/rev-share-cap/referral-event.tspackages/ens-referrals/src/index.tspackages/ens-referrals/src/leaderboard.ts
Comment on lines
+74
to
+82
| export function formatAccountingCsv( | ||
| records: ReadonlyArray<ReferralAccountingRecordRevShareCap>, | ||
| ): string { | ||
| const lines = [ | ||
| CSV_COLUMNS.map((c) => c.header).join(","), | ||
| ...records.map((r) => CSV_COLUMNS.map((c) => c.value(r)).join(",")), | ||
| ]; | ||
| return `${lines.join("\r\n")}\r\n`; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a UTF-8 BOM for Excel compatibility.
Names returned by ENS (r.name is InterpretedName) can contain non-ASCII characters (emoji, Cyrillic, etc.). When users open a UTF-8 CSV directly in Excel without an explicit import step, Excel will mis-decode bytes and show mojibake. Prepending a UTF-8 BOM ("\uFEFF") before the header row is the standard mitigation, with no impact on RFC-4180-compliant parsers. Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts`
around lines 74 - 82, The CSV produced by formatAccountingCsv can contain
non-ASCII ENS names and should include a UTF-8 BOM to avoid Excel mojibake;
update formatAccountingCsv so it prepends "\uFEFF" before the header row (i.e.,
before CSV_COLUMNS.map((c) => c.header).join(",")) while leaving the rest of the
join logic intact and still returning the final string with "\r\n" line endings;
reference CSV_COLUMNS and formatAccountingCsv to locate where to add the BOM.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the accounting trace logic, null-checks, and CSV formatting are all correct and well-tested. The race algorithm correctly maintains the hasQualified latch, captures pre-event award-pool state, and handles all qualification branches (not yet qualified, newly qualifying, already qualified, pool exhausted, admin disqualified, warning). The LEFT JOIN + post-query null-check pattern in the database layer gives deterministic, loud failures when required plugins are absent. The discriminated-union narrowing on awardModel in the accounting handler is type-safe. Test coverage addresses all edge cases. The one inline comment is a column-header clarity nit with no effect on correctness. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Handler as "GET /accounting"
participant Middleware as "Middlewares"
participant Cache as "SWRCache per edition"
participant Builder as "createEditionSnapshotBuilder"
participant DB as "ENSDb (Drizzle)"
participant Race as "buildReferralEditionSnapshotRevShareCap"
Client->>Handler: "?edition=slug"
Handler->>Middleware: "check prerequisites + init caches"
Middleware-->>Handler: "referralEditionSnapshotsCaches (Map)"
Handler->>Cache: "editionCache.read()"
alt cache stale or uninitialized
Cache->>Builder: "builder(cachedResult?)"
Builder->>Builder: "check immutability + plugin prerequisites"
Builder->>DB: "getReferralEvents(rules) via LEFT JOIN"
DB-->>Builder: "ReferralEvent[]"
Builder->>Race: "buildReferralEditionSnapshotRevShareCap(events, rules)"
Race-->>Builder: "ReferralEditionSnapshotRevShareCap { leaderboard, accountingRecords[] }"
Builder-->>Cache: "store snapshot"
end
Cache-->>Handler: "ReferralEditionSnapshot"
Handler->>Handler: "check awardModel === RevShareCap"
Handler->>Handler: "formatAccountingCsv(accountingRecords)"
Handler-->>Client: "text/csv attachment"
Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'origin' in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
|
@greptile |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts (1)
10-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNeutralize spreadsheet-formula prefixes in
csvCell.RFC-4180 quoting alone does not stop Excel/Sheets from evaluating cells that start with
=,+,-, or@.nameanddisqualificationReasoncan be user-controlled, so opening the exported CSV can trigger formula injection.Suggested hardening
function csvCell(value: string): string { - if (value.includes(",") || value.includes('"') || value.includes("\n") || value.includes("\r")) { - return `"${value.replace(/"/g, '""')}"`; + const sanitized = + value[0] === "=" || value[0] === "+" || value[0] === "-" || value[0] === "@" + ? `'${value}` + : value; + + if ( + sanitized.includes(",") || + sanitized.includes('"') || + sanitized.includes("\n") || + sanitized.includes("\r") + ) { + return `"${sanitized.replace(/"/g, '""')}"`; } - return value; + return sanitized; }🤖 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 `@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts` around lines 10 - 15, csvCell currently only quotes per RFC-4180 which doesn't prevent spreadsheet apps from evaluating cells that start with formula-triggering characters; update the csvCell function to detect if value begins with "=", "+", "-", or "@" and neutralize it (e.g., prefix with a single quote or a safe marker) before applying the existing quoting/escaping logic so that user-controlled fields like name and disqualificationReason cannot trigger formula injection; ensure the neutralization runs before replacing double quotes and that csvCell still returns RFC-4180-compliant quoted cells when needed.apps/ensapi/src/cache/referral-edition-snapshots.cache.ts (1)
43-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the redundant
@returnstags.These blocks just paraphrase the summary/signature, so they add noise without new information.
As per coding guidelines, "Do not add JSDoc
@returnstags that merely restate the method summary; remove such redundancy during PR review".Also applies to: 148-150, 182-183
🤖 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 `@apps/ensapi/src/cache/referral-edition-snapshots.cache.ts` around lines 43 - 45, Remove the redundant JSDoc `@returns` tags that merely restate the method signature for the edition-snapshot builders: delete the `@returns` lines in the JSDoc block describing the function that takes "editionConfig - The edition configuration" and returns "A function that builds the edition snapshot for the given edition", and also remove the analogous `@returns` tags in the other two JSDoc blocks referenced later (the blocks around lines with similar edition snapshot builders). Keep the descriptive summary and param tags, but eliminate each redundant `@returns` entry so the documentation no longer repeats the signature.
🤖 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.
Duplicate comments:
In `@apps/ensapi/src/cache/referral-edition-snapshots.cache.ts`:
- Around line 43-45: Remove the redundant JSDoc `@returns` tags that merely
restate the method signature for the edition-snapshot builders: delete the
`@returns` lines in the JSDoc block describing the function that takes
"editionConfig - The edition configuration" and returns "A function that builds
the edition snapshot for the given edition", and also remove the analogous
`@returns` tags in the other two JSDoc blocks referenced later (the blocks around
lines with similar edition snapshot builders). Keep the descriptive summary and
param tags, but eliminate each redundant `@returns` entry so the documentation no
longer repeats the signature.
In
`@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts`:
- Around line 10-15: csvCell currently only quotes per RFC-4180 which doesn't
prevent spreadsheet apps from evaluating cells that start with
formula-triggering characters; update the csvCell function to detect if value
begins with "=", "+", "-", or "@" and neutralize it (e.g., prefix with a single
quote or a safe marker) before applying the existing quoting/escaping logic so
that user-controlled fields like name and disqualificationReason cannot trigger
formula injection; ensure the neutralization runs before replacing double quotes
and that csvCell still returns RFC-4180-compliant quoted cells when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 400a31c4-52e4-4862-8633-ff6fa234e9df
📒 Files selected for processing (5)
apps/ensapi/src/cache/referral-edition-snapshots.cache.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.tspackages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts
lightwalker-eth
approved these changes
May 6, 2026
Member
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Goader Hey, great work here 🚀, let's merge this as soon as you're ready. Feedback I've shared is not important now. Please follow up on it in a separate future PR. Thanks!
| incrementalRevenueContribution: PriceEth; | ||
|
|
||
| /** | ||
| * FQDN of the name the referral applies to. |
Member
There was a problem hiding this comment.
Suggested change
| * FQDN of the name the referral applies to. | |
| * Name that was registered or renewed through the referral. |
| /** | ||
| * Ethereum address of the registrant that paid for / performed the action. | ||
| */ | ||
| registrant: Address; |
Member
There was a problem hiding this comment.
Please make this an AccountId that contains both an accountId and a chainId to uniquely identify the registrant. The chainId of the registrant will match the chainId of the registryId in the rules, but there might be other strategies to populate the chainId field we should use here.
Goal: Working to make our data models more mature and aligned with first-principles.
While making this update, we should also update the referrer field to be an AccountId too.
| transactionHash: Hash; | ||
|
|
||
| /** | ||
| * Ethereum address of the registrant that paid for / performed the action. |
Member
There was a problem hiding this comment.
Suggested change
| * Ethereum address of the registrant that paid for / performed the action. | |
| * The registrant who executed the registration or renewal. |
| name: InterpretedName; | ||
|
|
||
| /** | ||
| * Type of registrar action. |
Member
There was a problem hiding this comment.
Suggested change
| * Type of registrar action. | |
| * Type of registrar action, either a registration or renewal. |
| /** | ||
| * Transaction hash of the onchain registrar action that produced this referral. | ||
| */ | ||
| transactionHash: Hash; |
Member
There was a problem hiding this comment.
Please check this data type we added on ENSAwards: ensawards.org/data/shared/transactionRef.ts
Please:
- move the idea of a
TransactionRefinto theens-referralspackage. - Create an issue on the ENSAwards github for upgrading to the next ENSNode release. Inside of that issue identify how ENSAwards should delete it's definition of
TransactionRefand switch to using the definition exported byens-referrals.
Goal: Make our data models more mature and aligned with first-principles.
Please see my comment below on the registrant field for details about where we should get the chainId field value from that a TransactionRef needs.
| const sortedEvents = sortReferralEvents(events); | ||
|
|
||
| // Index admin actions by referrer; `rules.adminActions` is validated to have at most one action per referrer. | ||
| const adminActionByReferrer = new Map<NormalizedAddress, AdminAction>(); |
Member
There was a problem hiding this comment.
I shared feedback in another comment suggesting that action.referrer should change to an AccountId. To continue making this map work it's therefore necessary to transform action.referrer to a string using the function stringifyAccountId.
Therefore the data type for the key in a map like this should be an AccountIdString.
| accumulatedRevenueContribution: referrerState.totalRevenueContribution, | ||
| incrementalBaseRevenueContribution, | ||
| accumulatedBaseRevenueContribution, | ||
| awardPoolRemaining: awardPoolRemainingBefore, |
Member
There was a problem hiding this comment.
Suggested change
| awardPoolRemaining: awardPoolRemainingBefore, | |
| awardPoolRemainingBefore, |
Wdyt? This is a suggestion to rename the field in the definition of TentativeReferralAwardRevShareCap.
| }; | ||
|
|
||
| accountingRecords.push({ | ||
| registrarActionId: event.id, |
Member
There was a problem hiding this comment.
Suggested change
| registrarActionId: event.id, | |
| referralId: event.id, |
I think we can just proactively rename this now.
Comment on lines
+74
to
+82
| export function formatAccountingCsv( | ||
| records: ReadonlyArray<ReferralAccountingRecordRevShareCap>, | ||
| ): string { | ||
| const lines = [ | ||
| CSV_COLUMNS.map((c) => c.header).join(","), | ||
| ...records.map((r) => CSV_COLUMNS.map((c) => c.value(r)).join(",")), | ||
| ]; | ||
| return `${lines.join("\r\n")}\r\n`; | ||
| } |
| name: asInterpretedName("test.eth"), | ||
| actionType: "registration", | ||
| transactionHash: "0x0000000000000000000000000000000000000000000000000000000000000000" as Hash, | ||
| registrant: "0xdddddddddddddddddddddddddddddddddddddddd" as Address, |
Member
There was a problem hiding this comment.
This field should be of type NormalizedAddress. When we take a hardcoded constant and want to build a NormalizedAddress value from it we should always call toNormalizedAddress.
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.ts`:
- Around line 1228-1238: The test's expectedHeaderRow string ("Incremental
Duration (s)") is out of sync with the CSV_COLUMNS definition used by
formatAccountingCsv (which uses "Incremental Duration (seconds)"); update the
test by changing the expectedHeaderRow to match the CSV_COLUMNS header (use
"Incremental Duration (seconds)") or, better, import/derive the header from
CSV_COLUMNS used by formatAccountingCsv so the test always matches the source;
reference the expectedHeaderRow variable in this test and
formatAccountingCsv/CSV_COLUMNS in the source when making the change.
In
`@apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts`:
- Line 74: The CSV header "Incremental Duration (seconds)" in CSV_COLUMNS
doesn't match the test expectation "Incremental Duration (s)"; update the test's
expected header in the ENS analytics API test (the assertion that checks the
/accounting CSV startsWith the header string) to use "Incremental Duration
(seconds)" so the test matches the source-of-truth CSV_COLUMNS entry, keeping
unit style consistent with the other currency columns.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 21d60425-35b5-488f-b2df-901e429ffc6d
📒 Files selected for processing (2)
apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/format-accounting-csv.ts
Member
Author
|
@greptile |
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
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.
Award Accounting
pushes forward: #1797
Summary
rev-share-capeditionrev-share-capeditions)Why
Testing
Notes for Reviewer (Optional)
registrarsandsubgraphplugins activated to be able to gather the required information (otherwise newinnerJoins will drop everything silently). Should we add them as required for all the ENSAnalytics APIs?registrarActionIdto the CSV?Pre-Review Checklist (Blocking)