Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: a42ea5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes cursor encoding, adds paginated Event queries and filters, extends Event schema/DB and registration.start, wires event recording across indexer handlers, updates GraphQL schema/resolvers/tests, and applies large ABI/type polishing and datasource config changes. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(0,128,0,0.5)
participant Client as GraphQL Client
end
rect rgba(0,0,255,0.5)
participant Resolver as resolveFindEvents
end
rect rgba(128,0,128,0.5)
participant DB as Database
end
rect rgba(255,165,0,0.5)
participant Cursor as cursors
end
Client->>Resolver: Query <entity>.events(args, pagination)
Resolver->>Cursor: decode(after|before)
Cursor-->>Resolver: decoded cursor state
Resolver->>DB: SELECT events with where/through/join, ORDER BY composite key, LIMIT+1
DB-->>Resolver: rows
Resolver->>Cursor: encode(nextCursor)
Cursor-->>Resolver: cursor string
Resolver-->>Client: Connection { edges, pageInfo }
sequenceDiagram
rect rgba(0,128,0,0.5)
participant Indexer as Indexer Handler
end
rect rgba(255,0,0,0.5)
participant EventDB as event-db-helpers
end
rect rgba(128,128,0,0.5)
participant Events as events Table
end
rect rgba(0,0,255,0.5)
participant Join as domainEvent/resolverEvent/permissionsEvent
end
Indexer->>EventDB: ensureDomainEvent(context, event, id)
EventDB->>Events: ensureEvent(context, event) (insert/upsert)
Events-->>EventDB: event.id
EventDB->>Join: insert (ownerId, eventId)
Join-->>EventDB: link created
EventDB-->>Indexer: acknowledged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class ENSv2 “Event” storage + GraphQL exposure so Domains/Resolvers/Registries can surface an on-chain audit trail via cursor pagination, aligning with the long-term “history” needs described in #1674.
Changes:
- Expanded the
eventstable to store full log metadata (block/tx indices, topics, data) and added join tables (domain_events,resolver_events,registry_events). - Added ENSIndexer helpers/handlers to persist events and attach ENSv1 registry events to
Domainhistory. - Added GraphQL
*.eventsconnections plus new keyset cursor utilities (superjson base64 cursors) and pagination plumbing.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-schema/src/schemas/ensv2.schema.ts | Expands event storage and adds join tables for entity↔event relationships. |
| packages/datasources/src/abis/ensv2/Registry.ts | Updates ENSv2 Registry ABI event/function shapes (sender/indexed fields, new events). |
| packages/datasources/src/abis/ensv2/EnhancedAccessControl.ts | ABI param rename (rolesBitmap → roleBitmap). |
| packages/datasources/src/abis/ensv2/ETHRegistrar.ts | Large ABI refresh (constructor, errors, events, view/mutating fns). |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Adds event-to-domain-history wiring and indexes additional ENSv1 registry events. |
| apps/ensindexer/src/lib/ensv2/event-db-helpers.ts | Implements ensureEvent (richer metadata) + ensureDomainEvent. |
| apps/ensapi/src/graphql-api/schema/event.ts | Exposes new Event fields (blockNumber, txIndex, to, topics, data). |
| apps/ensapi/src/graphql-api/schema/domain.ts | Adds Domain.events connection. |
| apps/ensapi/src/graphql-api/schema/resolver.ts | Adds Resolver.events connection. |
| apps/ensapi/src/graphql-api/schema/registry.ts | Adds Registry.events connection. |
| apps/ensapi/src/graphql-api/lib/find-events/find-events-resolver.ts | New reusable resolver for paginated event connections via join tables. |
| apps/ensapi/src/graphql-api/lib/find-events/event-cursor.ts | Defines composite event cursor encode/decode. |
| apps/ensapi/src/graphql-api/lib/cursors.ts | Centralizes base64(superjson) cursor encoding/decoding. |
| apps/ensapi/src/graphql-api/schema/constants.ts | Moves cursor import + centralizes default/max page sizes. |
| apps/ensapi/src/graphql-api/lib/connection-helpers.ts | Updates to use new shared cursors util. |
| apps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.ts | Switches domain pagination to shared page-size constants + DomainCursors. |
| apps/ensapi/src/graphql-api/lib/find-domains/domain-cursor.ts | Uses shared cursors util; renames helper to DomainCursors. |
| apps/ensapi/src/graphql-api/lib/find-domains/domain-cursor.test.ts | Updates tests to use DomainCursors. |
| apps/ensapi/src/graphql-api/schema/cursors.ts | Removes old cursor helper (now replaced by lib/cursors). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![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: 8
🤖 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/graphql-api/lib/cursors.ts`:
- Around line 14-17: The thrown Error message in the cursor decoding catch block
is too specific ("failed to decode event cursor") for a shared utility; update
the message to be generic (e.g., "Invalid cursor: failed to decode cursor. The
cursor may be malformed or from an incompatible query.") in the catch inside the
cursor decoding function (the catch that currently throws the "Invalid cursor:
failed to decode event cursor..." error – locate the cursor decode utility,
e.g., decodeCursor/parseCursor or the catch in cursors.ts) so it no longer
references "event".
In `@apps/ensapi/src/graphql-api/lib/find-events/event-cursor.ts`:
- Around line 17-19: EventCursors.decode currently returns
cursors.decode<EventCursor>(cursor) without runtime validation; mirror the
pattern used in DomainCursors.decode by wrapping the decoded value in a
validation/guard and throwing a clear error if it doesn't match the EventCursor
shape. Update the EventCursors object (specifically the decode implementation)
to call cursors.decode, validate the result against the EventCursor structure
(or use an existing type guard/validator), and throw a descriptive error when
validation fails, similar to the TODO noted in domain-cursor.ts and
DomainCursors.decode.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 247-257: Domain.events currently lacks where/order args (left as
TODO); add the same connection args used by the registrations connection and
pass them into resolveFindEvents. Update the Domain.events field to accept
filtering/sorting args (e.g., where, order, first/after) matching the connection
input types used by registrations, ensure the resolver call
resolveFindEvents(schema.domainEvent, eq(schema.domainEvent.domainId,
parent.id), args) forwards those args unchanged, and add/update unit tests for
events filtering/ordering and schema types for EventRef to reflect the new args.
In `@apps/ensindexer/src/lib/ensv2/event-db-helpers.ts`:
- Around line 28-33: The current invariant throws an Error when event.log.topics
contains null, which is blocking CI; change this to log a non-fatal warning and
skip processing that log instead of throwing so indexing continues. Replace the
throw in the block that checks event.log.topics.some(topic => topic === null)
with a warning call (e.g., processLogger.warn or a module logger) that includes
the same message/context and the toJson(event.log.topics) payload, then
return/continue from the enclosing function to skip that malformed event;
alternatively only throw in a dev-only branch (NODE_ENV==='test' or an explicit
feature flag) if you must preserve the invariant in local dev. Ensure you update
all references to event.log.topics and keep the descriptive message for
debugging.
- Around line 67-69: The insert in ensureDomainEvent is not idempotent and will
fail on duplicate (domainId,eventId); update ensureDomainEvent to use the same
dedup pattern as ensureEvent by performing the insert into schema.domainEvent
with conflict handling (e.g., add an onConflictDoNothing() / equivalent on the
context.db.insert call) so repeated calls for the same domainId and eventId are
no-ops and do not throw.
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts`:
- Around line 131-159: Both handleNewTTL and handleNewResolver call
makeENSv1DomainId(node) and persist history via ensureDomainEvent even when node
=== ROOT_NODE; add an early return to skip handling when node equals ROOT_NODE
to avoid creating history for an unmodeled root domain. Specifically, in both
functions (handleNewTTL and handleNewResolver) check if event.args.node ===
ROOT_NODE (or compare node to ROOT_NODE) and return immediately before computing
domainId or calling ensureDomainEvent.
In `@packages/ensnode-schema/src/schemas/ensv2.schema.ts`:
- Around line 132-149: The join tables domainEvent, resolverEvent, and
registryEvent currently only define composite primary keys (entityId, eventId),
which orders the index by entity first and hurts performance for joins on
eventId; add a secondary index on eventId for each join table by updating the
onchainTable definitions (domainEvent, resolverEvent, registryEvent) to include
an index referencing t.eventId (e.g., add an index/secondaryIndex entry using
t.eventId) so that joins like .innerJoin(schema.event, eq(joinTable.eventId,
schema.event.id)) can use an eventId-prefixed index.
- Around line 83-90: The doc comment describing the Events table is cut off
("These join tables may store additional"); update the comment in
ensv2.schema.ts (the block describing Events, DomainEvent, ResolverEvent,
Registration, Renewal) to complete that sentence—e.g., state that join tables
may store additional relationship metadata (such as event-specific fields,
timestamps, blockNumber/transactionHash, or link attributes) so readers
understand what extra data DomainEvent/ResolverEvent can hold; keep the rest of
the paragraph unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96533aea-2bf7-4f52-bd5c-7bc1feb37f7d
📒 Files selected for processing (19)
apps/ensapi/src/graphql-api/lib/connection-helpers.tsapps/ensapi/src/graphql-api/lib/cursors.tsapps/ensapi/src/graphql-api/lib/find-domains/domain-cursor.test.tsapps/ensapi/src/graphql-api/lib/find-domains/domain-cursor.tsapps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.tsapps/ensapi/src/graphql-api/lib/find-events/event-cursor.tsapps/ensapi/src/graphql-api/lib/find-events/find-events-resolver.tsapps/ensapi/src/graphql-api/schema/constants.tsapps/ensapi/src/graphql-api/schema/cursors.tsapps/ensapi/src/graphql-api/schema/domain.tsapps/ensapi/src/graphql-api/schema/event.tsapps/ensapi/src/graphql-api/schema/registry.tsapps/ensapi/src/graphql-api/schema/resolver.tsapps/ensindexer/src/lib/ensv2/event-db-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tspackages/datasources/src/abis/ensv2/ETHRegistrar.tspackages/datasources/src/abis/ensv2/EnhancedAccessControl.tspackages/datasources/src/abis/ensv2/Registry.tspackages/ensnode-schema/src/schemas/ensv2.schema.ts
💤 Files with no reviewable changes (1)
- apps/ensapi/src/graphql-api/schema/cursors.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensapi/src/graphql-api/lib/find-events/find-events-resolver.ts
Outdated
Show resolved
Hide resolved
…e integration tests, etc
…ilable, indicating the start timestamp of the Registration.
…he set of Events for which this Account is the sender (i.e. `Transaction.from`).
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts
Dismissed
Show dismissed
Hide dismissed
apps/ensapi/src/graphql-api/lib/find-events/find-events-resolver.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 83 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensapi/src/test/integration/find-domains/domain-pagination-queries.ts (1)
14-34:⚠️ Potential issue | 🟠 MajorFetch
registration.startin the shared domain-pagination fixture.This PR moves ordering to the materialized
Registration.start, but the shared fragment/type still only carriesregistration.event.timestamp. That means the domain pagination suites are not asserting the field the resolver now orders by, so a brokenstartbackfill or stale ORDER BY can still pass.Suggested update
const PaginatedDomainFragment = gql` fragment PaginatedDomainFragment on Domain { id name label { interpreted } registration { + start expiry - event { timestamp } } } `; export type PaginatedDomainResult = { id: string; name: Name | null; label: { interpreted: InterpretedLabel }; registration: { + start: string; expiry: string | null; - event: { timestamp: string }; } | null; };Then update
apps/ensapi/src/test/integration/find-domains/test-domain-pagination.tsto compareregistration.startfor the START-based order assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/test/integration/find-domains/domain-pagination-queries.ts` around lines 14 - 34, The shared GraphQL fragment PaginatedDomainFragment and the TypeScript type PaginatedDomainResult must include registration.start so tests can assert the ORDER BY key; update PaginatedDomainFragment to fetch registration { start expiry event { timestamp } } and extend PaginatedDomainResult.registration to include start: string | null, then update the domain pagination test assertions to compare registration.start (instead of or in addition to event.timestamp) for START-based ordering checks.
♻️ Duplicate comments (3)
packages/ensnode-sdk/src/graphql-api/example-queries.ts (2)
347-352:⚠️ Potential issue | 🟠 MajorSepoliaV2 resolver lookup should use non-throwing pattern.
The
getDatasourceContractcall throws during module evaluation ifReverseResolverRoot.DefaultPublicResolver5is absent for SepoliaV2. This breaks the entire examples module even if the user never selects SepoliaV2. Other SepoliaV2 lookups in this file (lines 6-16) usemaybeGetDatasourceContractdefensively.Suggested fix
Add a non-throwing lookup at the top with other datasource constants:
const SEPOLIA_V2_DEFAULT_PUBLIC_RESOLVER = maybeGetDatasourceContract( ENSNamespaceIds.SepoliaV2, DatasourceNames.ReverseResolverRoot, "DefaultPublicResolver5", );Then conditionally include the SepoliaV2 entry:
- [ENSNamespaceIds.SepoliaV2]: { - resolver: getDatasourceContract( - ENSNamespaceIds.SepoliaV2, - DatasourceNames.ReverseResolverRoot, - "DefaultPublicResolver5", - ), - }, + ...(SEPOLIA_V2_DEFAULT_PUBLIC_RESOLVER + ? { [ENSNamespaceIds.SepoliaV2]: { resolver: SEPOLIA_V2_DEFAULT_PUBLIC_RESOLVER } } + : {}),,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts` around lines 347 - 352, The SepoliaV2 entry currently calls getDatasourceContract which throws during module evaluation if ReverseResolverRoot.DefaultPublicResolver5 is missing; change it to use a non-throwing lookup by adding a top-level constant using maybeGetDatasourceContract for ENSNamespaceIds.SepoliaV2, DatasourceNames.ReverseResolverRoot, "DefaultPublicResolver5" (e.g. SEPOLIA_V2_DEFAULT_PUBLIC_RESOLVER) and then update the map entry that currently calls getDatasourceContract to conditionally include the SepoliaV2 entry only if that constant is defined, leaving other entries unchanged.
141-164: 🧹 Nitpick | 🔵 TrivialDemonstrate cursor pagination and
wherefilters in the events query.Per PR objectives, the events API supports cursor-based pagination and
wherefilters (topic0_in,timestamp_gte,timestamp_lte,from). This example query uses the bareeventsconnection, which misses the opportunity to showcase these new capabilities in the playground.Suggested enhancement
-query DomainEvents($name: Name!) { +query DomainEvents($name: Name!, $first: Int, $after: String, $where: DomainEventsWhereInput) { domain(by: {name: $name}) { - events { + events(first: $first, after: $after, where: $where) { totalCount + pageInfo { hasNextPage endCursor } edges { node { from to topics data timestamp transactionHash } } } } }`, - variables: { default: { name: "newowner.eth" } }, + variables: { default: { name: "newowner.eth", first: 10 } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts` around lines 141 - 164, The DomainEvents example should demonstrate cursor pagination and where filters: update the GraphQL query named DomainEvents to call the events connection with arguments (e.g., events(first: $first, after: $after, where: $where)) and return pageInfo (endCursor, hasNextPage) and edges.node as before; add variables for first, after, and where (where can include topic0_in, timestamp_gte, timestamp_lte, from) and update the example's variables object (currently { default: { name: "newowner.eth" } }) to include these new variables so the playground shows paginated results and filtering; reference the DomainEvents query and the events field when making these changes.apps/ensindexer/src/lib/ensv2/event-db-helpers.ts (1)
69-96: 🧹 Nitpick | 🔵 TrivialConsider returning
eventIdfrom the relation helpers.This was noted in a previous review and remains valid. All three helpers compute
eventIdbut discard it. Returning it would allow callers to avoid a second call toensureEventwhen they need the ID.♻️ Suggested pattern
-export async function ensureDomainEvent(context: Context, event: LogEventBase, domainId: DomainId) { +export async function ensureDomainEvent(context: Context, event: LogEventBase, domainId: DomainId): Promise<string> { const eventId = await ensureEvent(context, event); await context.db.insert(schema.domainEvent).values({ domainId, eventId }).onConflictDoNothing(); + return eventId; }Apply the same pattern to
ensureResolverEventandensurePermissionsEvent.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensv2/event-db-helpers.ts` around lines 69 - 96, The three helper functions ensureDomainEvent, ensureResolverEvent, and ensurePermissionsEvent compute eventId via ensureEvent but don't return it; change each to return the computed eventId after performing the insert (i.e., call const eventId = await ensureEvent(...) then await context.db.insert(...).onConflictDoNothing(); and finally return eventId) so callers can reuse the id without calling ensureEvent again.
🤖 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/graphql-api/schema/permissions.integration.test.ts`:
- Around line 31-64: The test currently only checks events.length > 0; update
the test that uses PermissionsEvents (and the pagination helper call) to also
iterate the flattened events array and assert each node is scoped to
V2_ETH_REGISTRY by checking the event's permission target field (e.g.,
for/contract/account id) — reference the PermissionsEvents query, the
flattenConnection(result.permissions.events) result stored in events, and
V2_ETH_REGISTRY; add an expect inside the loop that compares the event node's
contract/for/account id to V2_ETH_REGISTRY so the test fails if unrelated/global
events are returned.
In `@apps/ensapi/src/graphql-api/schema/resolver.integration.test.ts`:
- Around line 32-64: Add a focused test that exercises Resolver.events filtering
by invoking the GraphQL query (use the existing ResolverEvents or create a
ResolverEventsWithWhere variant) against SOME_OWNED_RESOLVER with an
EventsWhereInput (e.g., topic0_in or timestamp_gte/timestamp_lte or from) and
assert the returned flattened events respect the filter (non-empty when expected
and/or that each event matches the where criteria). Hook this test into the same
describe block as the other Resolver.tests and/or add a parallel
pagination-filtered test similar to how ResolverEventsPaginated and
testEventPagination are used so regressions in topic0_in, timestamp_gte/lte, or
from wiring are caught.
In `@apps/ensapi/src/test/integration/find-domains/test-domain-pagination.ts`:
- Around line 105-127: The ordering oracle still uses REGISTRATION_TIMESTAMP
derived from domain.registration?.event.timestamp; change it to use the
materialized Registration.start instead: update the getSortValue helper (and any
constant like REGISTRATION_TIMESTAMP) to return domain.registration?.start (or
the materialized Registration.start field) and ensure assertOrdering and the
test's selected fields fetch registration.start rather than
domain.registration.event.timestamp so comparisons validate the new materialized
key.
In `@apps/ensapi/src/test/integration/graphql-utils.ts`:
- Around line 42-58: The collector collectForward should detect cursor cycles
and require a non-null advancing cursor when pageInfo.hasNextPage is true:
update collectForward to maintain a Set of seen cursors, assert that when
page.pageInfo.hasNextPage is true the page.pageInfo.endCursor is non-null,
assert the new endCursor is not already in the seen set (to catch A->B->A
cycles), and add each seen cursor before advancing; apply the same guards to the
corresponding backward collector (the one around lines 63-81) using
pageInfo.startCursor and hasPreviousPage and the same seen-cursor check so the
helpers fail fast instead of hanging when paginators misbehave.
In `@apps/ensindexer/src/lib/heal-addr-reverse-subname-label.ts`:
- Around line 65-76: The current try/catch wraps both the RPC call and the local
healing logic so exceptions from maybeHealLabelByAddrReverseSubname are
swallowed; change it so only the RPC call is inside the try: await
context.client.getTransactionReceipt({ hash: event.transaction.hash }) should be
wrapped and assign the result to a local variable, the catch should only swallow
RPC errors, and then call maybeHealLabelByAddrReverseSubname(labelHash,
receipt.contractAddress) outside the try block so any exceptions from
maybeHealLabelByAddrReverseSubname propagate (or rethrow non-RPC errors instead
of swallowing them).
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts`:
- Around line 279-281: Move the call to ensureDomainEvent(context, event,
domainId) so it runs after the state mutations in this handler: remove the
current call placed before the conditional and place it at the end of the
conditional block (after the db.update(...) calls that mutate state). Ensure the
event logging happens after all updates complete (consistent with NameWrapped,
BaseRegistrar:NameRegistered, and handleTransfer patterns) so the call to
ensureDomainEvent uses the final state of the domain.
In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts`:
- Around line 36-38: SOME_OWNED_RESOLVER is defined in lowercase; update its
value to the checksummed (EIP-55) address to match the other devnet constants
(e.g., DEVNET_DEPLOYER, DEVNET_OWNER). Replace the string for
SOME_OWNED_RESOLVER with the checksummed version (you can produce it via
ethers.utils.getAddress or any EIP-55 formatter) so the file uses consistent
address formatting.
---
Outside diff comments:
In `@apps/ensapi/src/test/integration/find-domains/domain-pagination-queries.ts`:
- Around line 14-34: The shared GraphQL fragment PaginatedDomainFragment and the
TypeScript type PaginatedDomainResult must include registration.start so tests
can assert the ORDER BY key; update PaginatedDomainFragment to fetch
registration { start expiry event { timestamp } } and extend
PaginatedDomainResult.registration to include start: string | null, then update
the domain pagination test assertions to compare registration.start (instead of
or in addition to event.timestamp) for START-based ordering checks.
---
Duplicate comments:
In `@apps/ensindexer/src/lib/ensv2/event-db-helpers.ts`:
- Around line 69-96: The three helper functions ensureDomainEvent,
ensureResolverEvent, and ensurePermissionsEvent compute eventId via ensureEvent
but don't return it; change each to return the computed eventId after performing
the insert (i.e., call const eventId = await ensureEvent(...) then await
context.db.insert(...).onConflictDoNothing(); and finally return eventId) so
callers can reuse the id without calling ensureEvent again.
In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts`:
- Around line 347-352: The SepoliaV2 entry currently calls getDatasourceContract
which throws during module evaluation if
ReverseResolverRoot.DefaultPublicResolver5 is missing; change it to use a
non-throwing lookup by adding a top-level constant using
maybeGetDatasourceContract for ENSNamespaceIds.SepoliaV2,
DatasourceNames.ReverseResolverRoot, "DefaultPublicResolver5" (e.g.
SEPOLIA_V2_DEFAULT_PUBLIC_RESOLVER) and then update the map entry that currently
calls getDatasourceContract to conditionally include the SepoliaV2 entry only if
that constant is defined, leaving other entries unchanged.
- Around line 141-164: The DomainEvents example should demonstrate cursor
pagination and where filters: update the GraphQL query named DomainEvents to
call the events connection with arguments (e.g., events(first: $first, after:
$after, where: $where)) and return pageInfo (endCursor, hasNextPage) and
edges.node as before; add variables for first, after, and where (where can
include topic0_in, timestamp_gte, timestamp_lte, from) and update the example's
variables object (currently { default: { name: "newowner.eth" } }) to include
these new variables so the playground shows paginated results and filtering;
reference the DomainEvents query and the events field when making these changes.
🪄 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: 5e113ec1-2ebb-4c7a-81ee-fc11009140af
📒 Files selected for processing (16)
apps/ensapi/src/graphql-api/schema/account.integration.test.tsapps/ensapi/src/graphql-api/schema/domain.integration.test.tsapps/ensapi/src/graphql-api/schema/permissions.integration.test.tsapps/ensapi/src/graphql-api/schema/permissions.tsapps/ensapi/src/graphql-api/schema/query.integration.test.tsapps/ensapi/src/graphql-api/schema/registry.integration.test.tsapps/ensapi/src/graphql-api/schema/resolver.integration.test.tsapps/ensapi/src/test/integration/find-domains/domain-pagination-queries.tsapps/ensapi/src/test/integration/find-domains/test-domain-pagination.tsapps/ensapi/src/test/integration/find-events/event-pagination-queries.tsapps/ensapi/src/test/integration/find-events/test-event-pagination.tsapps/ensapi/src/test/integration/graphql-utils.tsapps/ensindexer/src/lib/ensv2/event-db-helpers.tsapps/ensindexer/src/lib/heal-addr-reverse-subname-label.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tspackages/ensnode-sdk/src/graphql-api/example-queries.ts
Comment on lines
+42
to
+58
| export async function collectForward<T>(fetchPage: FetchPage<T>, pageSize: number): Promise<T[]> { | ||
| const all: T[] = []; | ||
| let after: string | undefined; | ||
|
|
||
| while (true) { | ||
| const page = await fetchPage({ first: pageSize, after }); | ||
| all.push(...flattenConnection(page)); | ||
|
|
||
| if (!page.pageInfo.hasNextPage) break; | ||
|
|
||
| const nextCursor = page.pageInfo.endCursor ?? undefined; | ||
| expect(nextCursor, "endCursor must advance when hasNextPage is true").not.toBe(after); | ||
| after = nextCursor; | ||
| } | ||
|
|
||
| return all; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard the shared collectors against cursor cycles and missing cursors.
These loops only check that the next cursor differs from the immediately previous one. A buggy paginator can still alternate cursors (A -> B -> A) or return hasNextPage/hasPreviousPage with a later-page null cursor, and this helper will hang instead of failing fast. Track seen cursors and require a non-null cursor whenever more pages are advertised.
Suggested hardening
export async function collectForward<T>(fetchPage: FetchPage<T>, pageSize: number): Promise<T[]> {
const all: T[] = [];
let after: string | undefined;
+ const seenCursors = new Set<string>();
while (true) {
const page = await fetchPage({ first: pageSize, after });
all.push(...flattenConnection(page));
if (!page.pageInfo.hasNextPage) break;
- const nextCursor = page.pageInfo.endCursor ?? undefined;
- expect(nextCursor, "endCursor must advance when hasNextPage is true").not.toBe(after);
+ const nextCursor = page.pageInfo.endCursor;
+ if (nextCursor == null) {
+ throw new Error("endCursor must be present when hasNextPage is true");
+ }
+ expect(seenCursors.has(nextCursor), `cursor cycle detected: ${nextCursor}`).toBe(false);
+ seenCursors.add(nextCursor);
after = nextCursor;
}
return all;
}
export async function collectBackward<T>(fetchPage: FetchPage<T>, pageSize: number): Promise<T[]> {
const all: T[] = [];
let before: string | undefined;
+ const seenCursors = new Set<string>();
while (true) {
const page = await fetchPage({ last: pageSize, before });
all.unshift(...flattenConnection(page));
if (!page.pageInfo.hasPreviousPage) break;
- const nextCursor = page.pageInfo.startCursor ?? undefined;
- expect(nextCursor, "startCursor must advance when hasPreviousPage is true").not.toBe(before);
+ const nextCursor = page.pageInfo.startCursor;
+ if (nextCursor == null) {
+ throw new Error("startCursor must be present when hasPreviousPage is true");
+ }
+ expect(seenCursors.has(nextCursor), `cursor cycle detected: ${nextCursor}`).toBe(false);
+ seenCursors.add(nextCursor);
before = nextCursor;
}
return all;
}Also applies to: 63-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/test/integration/graphql-utils.ts` around lines 42 - 58, The
collector collectForward should detect cursor cycles and require a non-null
advancing cursor when pageInfo.hasNextPage is true: update collectForward to
maintain a Set of seen cursors, assert that when page.pageInfo.hasNextPage is
true the page.pageInfo.endCursor is non-null, assert the new endCursor is not
already in the seen set (to catch A->B->A cycles), and add each seen cursor
before advancing; apply the same guards to the corresponding backward collector
(the one around lines 63-81) using pageInfo.startCursor and hasPreviousPage and
the same seen-cursor check so the helpers fail fast instead of hanging when
paginators misbehave.
apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts
Outdated
Show resolved
Hide resolved
Member
|
@greptile |
lightwalker-eth
approved these changes
Mar 12, 2026
Member
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Great work! 💪
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 83 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
apps/ensindexer/src/lib/ensv2/event-db-helpers.ts (1)
69-96: 🧹 Nitpick | 🔵 TrivialConsider returning
eventIdfrom relation helpers (optional).These helpers compute
eventIdinternally but discard it. Callers that also need the ID (e.g.,ExpiryExtendedhandler which callsensureDomainEventthenensureEventagain for renewal) incur an extra round-trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensv2/event-db-helpers.ts` around lines 69 - 96, The relation helpers ensureDomainEvent, ensureResolverEvent, and ensurePermissionsEvent compute eventId via ensureEvent but currently discard it, causing callers to re-call ensureEvent and incur extra DB round-trips; update each helper to return the computed eventId (preserve current DB inserts and .onConflictDoNothing() behavior) and update callers (e.g., the ExpiryExtended handler) to use the returned eventId instead of calling ensureEvent again to avoid duplicate lookups.apps/ensapi/src/graphql-api/schema/resolver.integration.test.ts (1)
21-66:⚠️ Potential issue | 🟠 MajorAdd at least one
Resolver.events(where: ...)integration case.This suite validates default and pagination paths but still doesn’t exercise filter wiring (
topic0_in,timestamp_gte/lte,from) for Resolver events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/resolver.integration.test.ts` around lines 21 - 66, The tests cover default and pagination paths but lack an integration case that exercises Resolver.events filters; add at least one test that calls the ResolverEvents query with a where input (e.g., topic0_in, timestamp_gte/timestamp_lte, from) and asserts the returned events are correctly filtered. Locate the existing ResolverEvents/ResolverEventsPaginated test scaffolding and add a new it/test case that sends variables including a where object (using DEVNET_NAME_WITH_OWNED_RESOLVER for name) and validate using flattenConnection or by checking node properties that only matching events are returned; ensure the new test references Resolver.events and the same EventFragment so it integrates with the current query shape.packages/ensnode-sdk/src/graphql-api/example-queries.ts (1)
145-163: 🧹 Nitpick | 🔵 TrivialShow pagination +
wherein theDomainEventsexample query.This example still demonstrates only the bare connection, so consumers don’t see how to use the newly added event filtering/pagination surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts` around lines 145 - 163, Update the DomainEvents example to show how to pass pagination and filtering by adding variables and using them in the events connection; change the query signature for DomainEvents to accept ($name: Name!, $first: Int, $after: String, $where: EventWhere) and call events(first: $first, after: $after, where: $where) so the response still returns totalCount and edges; also update the variables object (the variables.default currently used for name) to include sensible defaults for first/after/where (e.g., { name: "newowner.eth", first: 50, after: null, where: { /* example filter */ } }) so consumers see both pagination and where usage.
🤖 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/graphql-api/schema/account.integration.test.ts`:
- Line 131: Replace the placeholder describe.todo with real, executable
integration tests that exercise Account.events filtering via the
AccountEventsWhereInput: add a table-driven describe block that runs the GraphQL
query used elsewhere in this test suite (reuse the same test helper used in this
file) and asserts results for topic0_in, timestamp_gte, and timestamp_lte
combinations; for each case provide input where events should be returned and
where they should be excluded, using the existing fixtures/events from this test
file to verify correct inclusion/exclusion, and name the tests clearly (e.g.,
"filters by topic0_in" and "filters by timestamp_gte/timestamp_lte") so CI will
run them instead of leaving describe.todo.
In `@apps/ensapi/src/graphql-api/schema/domain.integration.test.ts`:
- Line 124: Replace the placeholder describe.todo("Domain.events filtering
(EventsWhereInput)") with real tests that exercise the GraphQL Domain.events
query using the EventsWhereInput filters; write tests that seed several Domain
event records (varying type, owner, timestamps, and metadata), then run GraphQL
queries against the Domain.events field with different EventsWhereInput
permutations (e.g., by event type, by owner, by createdAt range, and by metadata
key/value) and assert returned items and counts match expectations and
ordering/pagination semantics; reference the Domain.events resolver/field and
EventsWhereInput input type in your tests and reuse the test file's existing
test utilities to seed data and execute GraphQL queries.
In `@apps/ensapi/src/graphql-api/schema/permissions.integration.test.ts`:
- Line 71: Replace the placeholder describe.todo with real, executable tests
that verify Permissions.events filtering via the EventsWhereInput: write tests
that create sample events/fixtures, invoke the Permissions.events GraphQL query
(or resolver) with specific EventsWhereInput filter cases (e.g., by actor, verb,
object, date range) and assert the returned event list includes expected items
and excludes others; ensure you use the existing integration test helpers/client
used elsewhere in the suite to run the query and clean up fixtures.
- Line 57: The assertion compares event.address.toLowerCase() to
V2_ETH_REGISTRY.address but only lowercases the left side; update the test to
normalize both values (e.g., lowercase both) before comparing so the assertion
uses event.address.toLowerCase() === V2_ETH_REGISTRY.address.toLowerCase();
modify the expectation in the test that references event.address and
V2_ETH_REGISTRY.address accordingly.
---
Duplicate comments:
In `@apps/ensapi/src/graphql-api/schema/resolver.integration.test.ts`:
- Around line 21-66: The tests cover default and pagination paths but lack an
integration case that exercises Resolver.events filters; add at least one test
that calls the ResolverEvents query with a where input (e.g., topic0_in,
timestamp_gte/timestamp_lte, from) and asserts the returned events are correctly
filtered. Locate the existing ResolverEvents/ResolverEventsPaginated test
scaffolding and add a new it/test case that sends variables including a where
object (using DEVNET_NAME_WITH_OWNED_RESOLVER for name) and validate using
flattenConnection or by checking node properties that only matching events are
returned; ensure the new test references Resolver.events and the same
EventFragment so it integrates with the current query shape.
In `@apps/ensindexer/src/lib/ensv2/event-db-helpers.ts`:
- Around line 69-96: The relation helpers ensureDomainEvent,
ensureResolverEvent, and ensurePermissionsEvent compute eventId via ensureEvent
but currently discard it, causing callers to re-call ensureEvent and incur extra
DB round-trips; update each helper to return the computed eventId (preserve
current DB inserts and .onConflictDoNothing() behavior) and update callers
(e.g., the ExpiryExtended handler) to use the returned eventId instead of
calling ensureEvent again to avoid duplicate lookups.
In `@packages/ensnode-sdk/src/graphql-api/example-queries.ts`:
- Around line 145-163: Update the DomainEvents example to show how to pass
pagination and filtering by adding variables and using them in the events
connection; change the query signature for DomainEvents to accept ($name: Name!,
$first: Int, $after: String, $where: EventWhere) and call events(first: $first,
after: $after, where: $where) so the response still returns totalCount and
edges; also update the variables object (the variables.default currently used
for name) to include sensible defaults for first/after/where (e.g., { name:
"newowner.eth", first: 50, after: null, where: { /* example filter */ } }) so
consumers see both pagination and where usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43a5e37a-7a59-4a19-b414-9de08f3f72e9
📒 Files selected for processing (15)
apps/ensapi/src/graphql-api/lib/find-events/find-events-resolver.tsapps/ensapi/src/graphql-api/schema/account.integration.test.tsapps/ensapi/src/graphql-api/schema/account.tsapps/ensapi/src/graphql-api/schema/domain.integration.test.tsapps/ensapi/src/graphql-api/schema/domain.tsapps/ensapi/src/graphql-api/schema/permissions.integration.test.tsapps/ensapi/src/graphql-api/schema/permissions.tsapps/ensapi/src/graphql-api/schema/registration.tsapps/ensapi/src/graphql-api/schema/resolver.integration.test.tsapps/ensapi/src/test/integration/find-domains/domain-pagination-queries.tsapps/ensapi/src/test/integration/find-domains/test-domain-pagination.tsapps/ensapi/src/test/integration/find-events/event-pagination-queries.tsapps/ensindexer/src/lib/ensv2/event-db-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tspackages/ensnode-sdk/src/graphql-api/example-queries.ts
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.
closes #1674
I was trying to debug some event decoding issues, which necessitated making sure that the devnet and my ABIs were in sync with each other, so this also includes an update to the commit of devnet that we're integration testing against.
Reviewer Focus (Read This First)
the main areas to review:
ensureDomainEvent,ensureResolverEvent,ensurePermissionsEventnow push events to the appropriate join tablestopic0_in,timestamp_gte/lte,fromfilters. thethrough: { table, scope }pattern for narrowing via join tablesProblem & Motivation
Registration.startwas previously removed from the GraphQL API in favor of joining throughregistration.event.timestamp, making queries slower and more complexWhat Changed (Concrete)
Registration.startmaterialized — stored on the registration row, exposed in GraphQL, used directly for ordering (removes event table join from with-ordering-metadata).Account.eventsfield added. where filters (topic0_in, timestamp_gte/lte, from) on all *.events connections. resolveFindEvents queries event table directly, optionally narrowed via join table.Design & Planning
resolveFindEventsdesign iterated through: join-table-first → scope-first → always-query-event-table with optional join. final design keeps the query plan simple and avoids conditional FROM clause complexity.wherefilter design follows the find-domains pattern (input types → internal filter shape → SQL conditions)Self-Review
with-ordering-metadataafter materializingregistration.startthrough: { table, scope }reads clearly at callsitescursors.tsin schema/ replaced by sharedcursorslibDownstream & Consumer Impact
Registration.startfield added (non-breaking, new field)Account.eventsconnection added (non-breaking, new field)Domain.events,Resolver.events,Permissions.eventsnow acceptwherearg (non-breaking, optional)Eventtype gainsblockNumber,transactionIndex,to,topics,datafields (non-breaking)EventsWhereInput,AccountEventsWhereInputTesting Evidence
resolveFindEventsRisk Analysis
Pre-Review Checklist (Blocking)