ENSNode GraphQL API: Ergonomic Permissions#1745
Merged
Conversation
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds domain-level permissions connection and filtering, consolidates permission types to Changes
Sequence DiagramsequenceDiagram
participant Client as GraphQL Client
participant Domain as ENSv2Domain Resolver
participant Registry as Registry Loader
participant DB as Permissions DB Query
participant Pager as Cursor Pagination
Client->>Domain: query ENSv2Domain.permissions(where?)
Domain->>Registry: load registry for domain (chainId,address)
Registry-->>Domain: registry info
Domain->>DB: query permissions_user (chainId,address,resource[,user])
DB-->>Domain: rows + totalCount
Domain->>Pager: build cursor-paginated connection (edges, pageInfo, totalCount)
Pager-->>Client: return PermissionsUserRef connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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
Adds an ENSv2Domain-level GraphQL connection to list permission-user entries (“roles”) scoped to the domain’s registry resource, with an optional user filter, to support more ergonomic permissions access patterns in ENSNode’s GraphQL API.
Changes:
- Add
ENSv2Domain.rolesconnection that queriespermissions_usersscoped by(chainId, address, resource=tokenId)with cursor pagination. - Introduce
PermissionsUserWhereInput(currently withuser: Address) for optional filtering. - Extend imports to use
paginateByandID_PAGINATED_CONNECTION_ARGSfor ID-based pagination.
💡 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: 2
🤖 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/domain.ts`:
- Around line 375-379: PermissionsUserWhereInput is missing a description while
other input types have one; update the builder.inputType call for
PermissionsUserWhereInput to include a descriptive "description" property
(matching the style/format used by
DomainIdInput/DomainsWhereInput/AccountDomainsWhereInput) so the input type has
a clear description in the schema.
- Around line 342-343: The error thrown when a registry lookup fails uses an
unhelpful message ("never"); update the failing branch after
RegistryRef.getDataloader(context).load(parent.registryId) to throw a
descriptive Error that includes context (e.g., the missing registryId and
optionally the parent/domain id or name) so logs show which registry lookup
failed; locate the check using RegistryRef.getDataloader and parent.registryId
in domain.ts and replace the throw new Error("never") with a clear message like
"Registry not found for registryId=<...> (domainId=<...>)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47b00f16-7a4c-450d-b66c-accdce9d4f2c
📒 Files selected for processing (1)
apps/ensapi/src/graphql-api/schema/domain.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensapi/src/graphql-api/schema/account.ts (1)
130-162:⚠️ Potential issue | 🟠 MajorThese connections lost the contract identity clients need.
PermissionsUserRefonly exposesid,resource,user, androlesinapps/ensapi/src/graphql-api/schema/permissions.ts:196-239. After switchingregistryPermissionsandresolverPermissionsto that type, callers can no longer tell which registry or resolver each permission came from, so entries from different contracts become indistinguishable wheneverresourcevalues collide. Keep a contract-scoped wrapper type here, or expose the contract onPermissionsUserbefore changing these return types.Also applies to: 167-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/account.ts` around lines 130 - 162, registryPermissions (and similarly resolverPermissions) now return PermissionsUserRef which lacks contract identity, making permissions from different contracts indistinguishable; restore contract-scoped identity by either creating and returning a wrapper type (e.g., RegistryPermissionsUserRef or ResolverPermissionsUserRef) that includes the contract identifier (registry.chainId and registry.address) alongside the PermissionsUser fields, or add contract fields to PermissionsUserRef itself; update the GraphQL type declarations (where PermissionsUserRef is defined) and adjust the resolver in registryPermissions/resolverPermissions to populate the new contract fields from schema.registry (use the same join variables already present) so callers can tell which contract each permission row belongs to.
♻️ Duplicate comments (2)
apps/ensapi/src/graphql-api/schema/domain.ts (2)
342-343:⚠️ Potential issue | 🟡 MinorReplace the
"never"invariant message.If this branch fires, the current error gives no registry or domain context for debugging.
♻️ Proposed fix
const registry = await RegistryRef.getDataloader(context).load(parent.registryId); - if (!registry) throw new Error("never"); + if (!registry) { + throw new Error( + `Invariant(ENSv2Domain.roles): Registry not found for registryId=${parent.registryId} domainId=${parent.id}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/domain.ts` around lines 342 - 343, The invariant error "never" in the RegistryRef.getDataloader(...).load(parent.registryId) branch is unhelpful; update the thrown Error in that if (!registry) block to include useful context such as parent.registryId and parent.name (or parent.id/domain identifier) and a short message like "Registry not found" so logs/errors show which registry/domain failed; locate the check in the resolver where RegistryRef.getDataloader and parent.registryId are used and replace the generic message with a formatted one containing those identifiers.
375-379:⚠️ Potential issue | 🟡 MinorAdd a description to
PermissionsUserWhereInput.This is the only nearby input type without schema docs, so it sticks out in introspection.
As per coding guidelines, "Maintain comment consistency within a file; if most types, schemas, or declarations lack comments, do not add a comment to a single one."♻️ Proposed fix
export const PermissionsUserWhereInput = builder.inputType("PermissionsUserWhereInput", { + description: "Filter for permissions user queries.", fields: (t) => ({ user: t.field({ type: "Address" }), }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/domain.ts` around lines 375 - 379, PermissionsUserWhereInput is missing a schema description while the surrounding input types have docs; add a short description to the input type (for example "Filter permissions by user address") by supplying the description option on the builder.inputType call for PermissionsUserWhereInput and ensure the field user remains of type "Address"—if the rest of the file actually lacks descriptions, instead leave it unchanged to preserve consistency.
🤖 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/domain.ts`:
- Around line 331-367: Replace the single roles field with two fields
registryRoles and resolverRoles (or add them alongside roles) so the schema
matches issue `#1676`: implement registryRoles using the same logic as the
existing resolve block (use
RegistryRef.getDataloader(context).load(parent.registryId), build scope with
eq(schema.permissionsUser.chainId, registry.chainId),
eq(schema.permissionsUser.address, registry.address),
eq(schema.permissionsUser.resource, parent.tokenId), and optional
args.where.user), and return lazyConnection with totalCount using
db.$count(schema.permissionsUser, scope) and connection using
resolveCursorConnection + paginateBy/orderPaginationBy as in the current code;
implement resolverRoles similarly but derive the resolver identifier from the
Domain parent object (e.g., parent.resolverId / parent.resolverAddress — use the
actual field on Domain), build a scope that filters permissions by that resolver
address/identifier (and chainId) and the same resource/tokenId, and reuse
lazyConnection + resolveCursorConnection pagination logic so clients can query
resolver-backed permissions separately from registry-backed permissions.
---
Outside diff comments:
In `@apps/ensapi/src/graphql-api/schema/account.ts`:
- Around line 130-162: registryPermissions (and similarly resolverPermissions)
now return PermissionsUserRef which lacks contract identity, making permissions
from different contracts indistinguishable; restore contract-scoped identity by
either creating and returning a wrapper type (e.g., RegistryPermissionsUserRef
or ResolverPermissionsUserRef) that includes the contract identifier
(registry.chainId and registry.address) alongside the PermissionsUser fields, or
add contract fields to PermissionsUserRef itself; update the GraphQL type
declarations (where PermissionsUserRef is defined) and adjust the resolver in
registryPermissions/resolverPermissions to populate the new contract fields from
schema.registry (use the same join variables already present) so callers can
tell which contract each permission row belongs to.
---
Duplicate comments:
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 342-343: The invariant error "never" in the
RegistryRef.getDataloader(...).load(parent.registryId) branch is unhelpful;
update the thrown Error in that if (!registry) block to include useful context
such as parent.registryId and parent.name (or parent.id/domain identifier) and a
short message like "Registry not found" so logs/errors show which
registry/domain failed; locate the check in the resolver where
RegistryRef.getDataloader and parent.registryId are used and replace the generic
message with a formatted one containing those identifiers.
- Around line 375-379: PermissionsUserWhereInput is missing a schema description
while the surrounding input types have docs; add a short description to the
input type (for example "Filter permissions by user address") by supplying the
description option on the builder.inputType call for PermissionsUserWhereInput
and ensure the field user remains of type "Address"—if the rest of the file
actually lacks descriptions, instead leave it unchanged to preserve consistency.
🪄 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: 43307fe7-f4c2-4bed-99bd-dde810806a7a
📒 Files selected for processing (5)
apps/ensapi/src/graphql-api/lib/lazy-connection.tsapps/ensapi/src/graphql-api/schema/account-registries-permissions.tsapps/ensapi/src/graphql-api/schema/account-resolver-permissions.tsapps/ensapi/src/graphql-api/schema/account.tsapps/ensapi/src/graphql-api/schema/domain.ts
💤 Files with no reviewable changes (2)
- apps/ensapi/src/graphql-api/schema/account-resolver-permissions.ts
- apps/ensapi/src/graphql-api/schema/account-registries-permissions.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR refactors the ENSNode GraphQL API permissions layer to be more ergonomic: it replaces composite DTO wrapper types ( Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL
participant DB
Note over Client,DB: Account.registryPermissions
Client->>GraphQL: account { registryPermissions { edges { node { registry { id } resource roles } } } }
GraphQL->>DB: SELECT permissionsUser.* FROM permissionsUser INNER JOIN registry ON (chainId/address match) WHERE user = $account ORDER BY id
DB-->>GraphQL: permissionsUser rows
GraphQL-->>Client: RegistryPermissionsUser[] (registry resolved via makeRegistryId)
Note over Client,DB: ENSv2Domain.permissions (new)
Client->>GraphQL: domain { permissions(where: { user: $addr }) { edges { node { user contract resource roles } } } }
GraphQL->>DB: SELECT permissionsUser.* FROM permissionsUser INNER JOIN registry ON (chainId/address match AND registry.id = $registryId) WHERE resource = $tokenId [AND user = $addr]
DB-->>GraphQL: permissionsUser rows
GraphQL-->>Client: PermissionsUser[] (with id, contract, user, resource, roles)
Note over Client,DB: Account.permissions (optional contract filter)
Client->>GraphQL: account { permissions(in: { chainId: 1, address: "0x..." }) { edges { node { contract { chainId address } } } } }
GraphQL->>DB: SELECT * FROM permissionsUser WHERE user = $account AND chainId = $chainId AND address = $address
DB-->>GraphQL: permissionsUser rows
GraphQL-->>Client: PermissionsUser[] (with contract field)
Last reviewed commit: f9f9d89 |
![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 6 out of 6 changed files in this pull request and generated 7 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.
Pull request overview
Copilot reviewed 8 out of 8 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.
Comment on lines
+144
to
+149
| contract: t.field({ | ||
| description: "The contract within which these Permissions are granted.", | ||
| type: AccountIdRef, | ||
| nullable: false, | ||
| resolve: ({ chainId, address }) => ({ chainId, address }), | ||
| }), |
There was a problem hiding this comment.
The new non-null contract field on permissions models is a public schema change (it affects PermissionsResource and PermissionsUser). There are integration tests in this schema package, but none assert contract { chainId address } resolves correctly; adding at least one integration test that queries these new fields would help prevent regressions in ID/contract derivation.
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 Nice work looks good 👍
Member
|
@greptile |
…c permission connection types
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 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: 3
♻️ Duplicate comments (1)
apps/ensapi/src/graphql-api/schema/domain.ts (1)
331-378:⚠️ Potential issue | 🟠 MajorExpose the requested
registryRoles/resolverRolesAPI instead of a singlepermissionsconnection.This resolver only scopes
permissionsUserthroughschema.registry, so it can only return registry-backed roles. The PR contract here isregistryRoles(where: PermissionsUserWhereInput)andresolverRoles(where: PermissionsUserWhereInput); shippingpermissions(where: DomainPermissionsWhereInput)leaves resolver roles unimplemented and changes the public GraphQL schema shape clients will generate against.Also applies to: 386-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/domain.ts` around lines 331 - 378, The current ENSv2Domain.permissions resolver only returns registry-backed roles and exposes a single permissions connection (permissions: PermissionsUserRef with DomainPermissionsWhereInput) which omits resolver-backed roles and changes the public schema; change the API to expose two separate fields registryRoles(where: PermissionsUserWhereInput) and resolverRoles(where: PermissionsUserWhereInput), implement the existing resolver logic as registryRoles (reuse scope/join using schema.permissionsUser and schema.registry, lazyConnection, resolveCursorConnection, paginateBy, orderPaginationBy), and add a resolverRoles implementation that scopes schema.permissionsUser by resource and filters by resolver-specific join/condition (instead of registry join) so both role sources are returned and the public GraphQL shape matches the PR contract.
🤖 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/registry-permissions-user.ts`:
- Around line 8-10: Replace the standalone JSDoc with a GraphQL description
attached to the schema implementation: remove the top-level JSDoc and add the
text as the description in RegistryPermissionsUserRef.implement({ description:
"..." }) so the type is documented in the GraphQL schema itself and file comment
style remains consistent; ensure the description string matches the original
JSDoc meaning (e.g., "Represents a PermissionsUser whose contract is a Registry,
providing a semantic `registry` field.").
In `@apps/ensapi/src/graphql-api/schema/resolver-permissions-user.ts`:
- Around line 8-10: The JSDoc for the PermissionsUser should be moved into the
GraphQL type description or removed: update
ResolverPermissionsUserRef.implement({ description: ... }) to include the JSDoc
text (or delete the JSDoc comment entirely) so the description appears in schema
introspection and matches file comment consistency; locate the existing JSDoc at
the top of the file and either paste that sentence into the description property
of ResolverPermissionsUserRef.implement or remove the standalone JSDoc block.
In `@apps/ensapi/src/graphql-api/schema/scalars.ts`:
- Around line 140-198: These six identical builder.scalarType definitions
(PermissionsId, PermissionsResourceId, PermissionsUserId, RegistrationId,
RenewalId, ResolverRecordsId) should be factored into a reusable helper (e.g.,
makeIdScalar or createIdScalar) that accepts the scalar name and branded type
and returns the scalarType config; implement the helper near the other scalar
helpers, then replace each builder.scalarType(...) call with a short call like
createIdScalar(builder, "PermissionsId", <BrandedType>) or similar, ensuring the
helper wires description, serialize, and parseValue using
z.coerce.string().transform(...) to cast to the branded type and preserves the
original symbol names (PermissionsId, PermissionsResourceId, PermissionsUserId,
RegistrationId, RenewalId, ResolverRecordsId) so callers and exports remain
unchanged.
---
Duplicate comments:
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 331-378: The current ENSv2Domain.permissions resolver only returns
registry-backed roles and exposes a single permissions connection (permissions:
PermissionsUserRef with DomainPermissionsWhereInput) which omits resolver-backed
roles and changes the public schema; change the API to expose two separate
fields registryRoles(where: PermissionsUserWhereInput) and resolverRoles(where:
PermissionsUserWhereInput), implement the existing resolver logic as
registryRoles (reuse scope/join using schema.permissionsUser and
schema.registry, lazyConnection, resolveCursorConnection, paginateBy,
orderPaginationBy), and add a resolverRoles implementation that scopes
schema.permissionsUser by resource and filters by resolver-specific
join/condition (instead of registry join) so both role sources are returned and
the public GraphQL shape matches the PR contract.
🪄 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: e6d81e79-d806-4334-83a9-f9c458730c26
📒 Files selected for processing (11)
apps/ensapi/src/graphql-api/builder.tsapps/ensapi/src/graphql-api/lib/lazy-connection.tsapps/ensapi/src/graphql-api/schema/account.tsapps/ensapi/src/graphql-api/schema/domain.tsapps/ensapi/src/graphql-api/schema/permissions.tsapps/ensapi/src/graphql-api/schema/registration.tsapps/ensapi/src/graphql-api/schema/registry-permissions-user.tsapps/ensapi/src/graphql-api/schema/renewal.tsapps/ensapi/src/graphql-api/schema/resolver-permissions-user.tsapps/ensapi/src/graphql-api/schema/resolver-records.tsapps/ensapi/src/graphql-api/schema/scalars.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 #1676