Flip dependency relationship between the ens-referrals and ensnode-sdk packages
#1542
Conversation
🦋 Changeset detectedLatest commit: dbef3e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR flips the dependency relationship between the ens-referrals and ensnode-sdk packages to better align package responsibilities. The ensnode-sdk package now contains shared types and utility code that ens-referrals depends on, reversing the previous dependency direction.
Changes:
- Moved
EncodedReferrertypes and encoding/decoding logic fromens-referralstoensnode-sdk - Consolidated duplicate types (
ChainId,AccountId,UnixTimestamp,Duration) by havingens-referralsimport them fromensnode-sdk - Created new
ENSReferralsClientinens-referralspackage for referral leaderboard APIs (moved fromensnode-sdk) - Removed
ensanalyticsAPI endpoints fromENSNodeClientin favor of the new specialized client
Reviewed changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates dependency graph to reflect the flipped relationship |
| packages/ensnode-sdk/src/registrars/encoded-referrer.ts | Moved encoded referrer logic from ens-referrals with updated documentation |
| packages/ensnode-sdk/src/registrars/zod-schemas.ts | Updates import path for encoded referrer utilities |
| packages/ensnode-sdk/src/registrars/registrar-action.ts | Updates import paths and documentation comments |
| packages/ensnode-sdk/src/registrars/index.ts | Exports new encoded-referrer module |
| packages/ensnode-sdk/src/client.ts | Removes ENSAnalytics API methods (moved to ens-referrals) |
| packages/ensnode-sdk/src/index.ts | Removes ensanalytics export |
| packages/ensnode-sdk/package.json | Removes ens-referrals dependency |
| packages/ens-referrals/src/time.ts | Imports UnixTimestamp and Duration from ensnode-sdk |
| packages/ens-referrals/src/rules.ts | Imports AccountId and UnixTimestamp from ensnode-sdk |
| packages/ens-referrals/src/client.ts | New ENSReferralsClient with referral leaderboard API methods |
| packages/ens-referrals/src/api/* | Updates imports to use local and ensnode-sdk types |
| packages/ens-referrals/src/index.ts | Adds exports for api and client modules |
| packages/ens-referrals/src/internal.ts | New internal exports for zod schemas |
| packages/ens-referrals/package.json | Adds ensnode-sdk dependency and internal export path |
| packages/ensnode-schema/src/schemas/registrars.schema.ts | Updates documentation comments |
| apps/ensapi/src/handlers/ensanalytics-api.ts | Updates imports to use ens-referrals package |
| apps/ensapi/src/handlers/ensanalytics-api.test.ts | Updates imports to use ens-referrals package |
| apps/ensapi/src/lib/ensanalytics/referrer-leaderboard/mocks.ts | Updates imports to use ens-referrals package |
| .changeset/rich-shirts-spend.md | Documents the breaking changes |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/ens-referrals/package.json:40
- The
publishConfig.exportssection is missing the/internalexport path. This means when the package is published to npm, consumers won't be able to import from@namehash/ens-referrals/internaleven though it's defined in the developmentexportsfield. Add the internal export configuration to match the pattern used in the main export.
"publishConfig": {
"access": "public",
"exports": {
".": {
"import": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
},
"require": {
"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
}
}
},
"main": "./dist/index.cjs",
"module": "./dist/index.js",
"types": "./dist/index.d.ts"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…de-sdk-ens-referrals
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummarySuccessfully flipped dependency relationship between Key Changes:
Impact:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant ENSAwards as ENS Awards App
participant ENSReferralsClient as ENSReferralsClient<br/>(ens-referrals)
participant ENSNodeClient as ENSNodeClient<br/>(ensnode-sdk)
participant ENSApi as ENSNode API<br/>(ensapi)
Note over ENSAwards,ENSApi: Before: Single Client, Circular Dependency Issue
Note over ENSAwards,ENSApi: After: Separate Clients, Clean Dependencies
ENSAwards->>ENSReferralsClient: new ENSReferralsClient()
ENSAwards->>ENSNodeClient: new ENSNodeClient()
ENSAwards->>ENSReferralsClient: getReferrerLeaderboardPage({page, recordsPerPage})
ENSReferralsClient->>ENSApi: GET /ensanalytics/referrers
ENSApi-->>ENSReferralsClient: SerializedReferrerLeaderboardPageResponse
ENSReferralsClient->>ENSReferralsClient: deserializeReferrerLeaderboardPageResponse()
ENSReferralsClient-->>ENSAwards: ReferrerLeaderboardPageResponse
ENSAwards->>ENSReferralsClient: getReferrerDetail({referrer: "0x..."})
ENSReferralsClient->>ENSApi: GET /api/ensanalytics/referrers/:address
ENSApi-->>ENSReferralsClient: SerializedReferrerDetailResponse
ENSReferralsClient->>ENSReferralsClient: deserializeReferrerDetailResponse()
ENSReferralsClient-->>ENSAwards: ReferrerDetailResponse
ENSAwards->>ENSNodeClient: resolveRecords(name, selection)
ENSNodeClient->>ENSApi: GET /api/resolve/records/:name
ENSApi-->>ENSNodeClient: ResolveRecordsResponse
ENSNodeClient-->>ENSAwards: records
Note over ENSReferralsClient,ENSNodeClient: Type Flow: ChainId, AccountId,<br/>UnixTimestamp, Duration<br/>flow from ensnode-sdk → ens-referrals
Note over ENSApi: ENSApi uses both packages<br/>temporarily until migration complete
|
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Goader Great job! Nice work 🚀
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis pull request flips the dependency relationship between Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
lightwalker-eth
deleted the
feat/flip-dependency-ensnode-sdk-ens-referrals
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ens-referrals/package.json (1)
20-41: Missing./internalexport inpublishConfigmay break consumers after publishing.The
"./internal"entry point is defined in the developmentexports(line 22) but is absent frompublishConfig.exports. When the package is published, consumers importing from@namehash/ens-referrals/internalwill encounter module resolution errors.If this entry point is intended to be public, add the corresponding mapping to
publishConfig.exports:Proposed fix
"publishConfig": { "access": "public", "exports": { ".": { "import": { "types": "./dist/index.d.ts", "default": "./dist/index.js" }, "require": { "types": "./dist/index.d.cts", "default": "./dist/index.cjs" } + }, + "./internal": { + "import": { + "types": "./dist/internal.d.ts", + "default": "./dist/internal.js" + }, + "require": { + "types": "./dist/internal.d.cts", + "default": "./dist/internal.cjs" + } } },If
./internalis intentionally for monorepo-only consumption, consider documenting this or using a different naming convention to signal internal use.
🤖 Fix all issues with AI agents
In `@packages/ens-referrals/src/client.ts`:
- Around line 137-151: The code unsafely asserts responseData to
SerializedReferrerLeaderboardPageResponse before deserialization; instead remove
the type assertion and explicitly validate responseData with the Zod-backed
deserializer or perform an explicit safeParse call so the shape is checked
before treating it as the expected type—update the call site around responseData
and deserializeReferrerLeaderboardPageResponse (or add a pre-check using the
same schema used by deserializeReferrerLeaderboardPageResponse) and add a short
comment clarifying that validation happens there.
- Around line 15-23: Replace the duplicated DEFAULT_ENSNODE_API_URL constant and
ClientOptions interface in this file by importing them from the shared module in
ensnode-sdk: remove the local export of DEFAULT_ENSNODE_API_URL and the local
ClientOptions interface and add imports for DEFAULT_ENSNODE_API_URL and
ClientOptions from the ensnode-sdk package, then update any local references to
use the imported symbols so the file uses the shared definitions.
- Around line 127-132: getReferrerLeaderboardPage builds the URL with the wrong
path; change the path string in the URL constructor from
`/ensanalytics/referrers` to `/api/ensanalytics/referrers` so it matches
getReferrerDetail and the rest of the SDK; update the URL creation in
packages/ens-referrals/src/client.ts inside the getReferrerLeaderboardPage logic
where new URL(...) is called and ensure subsequent query param handling (page,
recordsPerPage) remains unchanged.
♻️ Duplicate comments (6)
packages/ensnode-sdk/src/api/name-tokens/response.ts (1)
3-3: Unused import:NameTokenOwnershipTypes.
NameTokenOwnershipTypesis only referenced in JSDoc comments (lines 105-112) but never used in runtime code or type definitions. Consider removing it from the import statement.packages/ens-referrals/tsconfig.json (1)
3-5: Comment could be more descriptive.The inline comment explaining
rootDiris terse. Consider expanding it to explain why this setting resolves the ambiguity, as noted in a prior review..changeset/rich-shirts-spend.md (1)
1-7: Version bump for@namehash/ens-referralsshould bemajor.This change removes previously exported types (
UnixTimestamp,Duration,AccountId,ChainId) from@namehash/ens-referrals. Consumers importing these types will need to update their imports to use@ensnode/ensnode-sdkinstead. Per semantic versioning, removing public exports constitutes a breaking change requiring a major version bump.Proposed fix
--- -"@namehash/ens-referrals": minor +"@namehash/ens-referrals": major "@ensnode/ensnode-sdk": minor "ensapi": patch ---packages/ens-referrals/src/internal.ts (1)
1-15: Misleading documentation about NPM package inclusion.Line 8 states "This file can never be included in the NPM package" but this contradicts the package.json export configuration. The file must be included in the published package for the
@namehash/ens-referrals/internalimport path to function.packages/ensnode-sdk/src/registrars/registrar-action.ts (1)
154-155: Documentation describes decoding from a null value.The comment states the referrer is "decoded from encodedReferrer using strict left-zero-padding validation" but
encodedReferrerisnullfor this interface. The documentation should clarify thatdecodedReferreris null becauseencodedReferreris null.packages/ens-referrals/src/client.ts (1)
50-254: Missing test coverage for ENSReferralsClient.The new client class lacks unit tests. Other modules in this package have corresponding test files (e.g.,
leaderboard-page.test.ts). Tests should cover URL construction, success/error response handling, and JSON parsing edge cases.
| export const DEFAULT_ENSNODE_API_URL = "https://api.alpha.ensnode.io" as const; | ||
|
|
||
| /** | ||
| * Configuration options for ENS Referrals API client | ||
| */ | ||
| export interface ClientOptions { | ||
| /** The ENSNode API URL */ | ||
| url: URL; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider importing shared constants from ensnode-sdk.
DEFAULT_ENSNODE_API_URL and ClientOptions are duplicated from packages/ensnode-sdk/src/client.ts. Since ens-referrals now depends on ensnode-sdk, consider importing these to avoid duplication and ensure consistency.
♻️ Proposed refactor
+import {
+ DEFAULT_ENSNODE_API_URL,
+ type ClientOptions,
+} from "@ensnode/ensnode-sdk";
+
import {
deserializeReferrerDetailResponse,
deserializeReferrerLeaderboardPageResponse,
...
} from "./api";
-/**
- * Default ENSNode API endpoint URL
- */
-export const DEFAULT_ENSNODE_API_URL = "https://api.alpha.ensnode.io" as const;
-
-/**
- * Configuration options for ENS Referrals API client
- */
-export interface ClientOptions {
- /** The ENSNode API URL */
- url: URL;
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const DEFAULT_ENSNODE_API_URL = "https://api.alpha.ensnode.io" as const; | |
| /** | |
| * Configuration options for ENS Referrals API client | |
| */ | |
| export interface ClientOptions { | |
| /** The ENSNode API URL */ | |
| url: URL; | |
| } | |
| import { | |
| DEFAULT_ENSNODE_API_URL, | |
| type ClientOptions, | |
| } from "@ensnode/ensnode-sdk"; | |
| import { | |
| deserializeReferrerDetailResponse, | |
| deserializeReferrerLeaderboardPageResponse, | |
| } from "./api"; |
🤖 Prompt for AI Agents
In `@packages/ens-referrals/src/client.ts` around lines 15 - 23, Replace the
duplicated DEFAULT_ENSNODE_API_URL constant and ClientOptions interface in this
file by importing them from the shared module in ensnode-sdk: remove the local
export of DEFAULT_ENSNODE_API_URL and the local ClientOptions interface and add
imports for DEFAULT_ENSNODE_API_URL and ClientOptions from the ensnode-sdk
package, then update any local references to use the imported symbols so the
file uses the shared definitions.
| const url = new URL(`/ensanalytics/referrers`, this.options.url); | ||
|
|
||
| if (request?.page) url.searchParams.set("page", request.page.toString()); | ||
| if (request?.recordsPerPage) | ||
| url.searchParams.set("recordsPerPage", request.recordsPerPage.toString()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file and see both methods in context
cat -n packages/ens-referrals/src/client.ts | sed -n '120,140p'Repository: namehash/ensnode
Length of output: 943
🏁 Script executed:
# Now let's check the getReferrerDetail method around line 232
cat -n packages/ens-referrals/src/client.ts | sed -n '225,240p'Repository: namehash/ensnode
Length of output: 711
🏁 Script executed:
# Let's also check the full structure to understand URL construction
cat -n packages/ens-referrals/src/client.ts | sed -n '1,50p'Repository: namehash/ensnode
Length of output: 1600
🏁 Script executed:
# Search for all URL path patterns in this file
rg -n "new URL|url.searchParams|\/ensanalytics|\/api" packages/ens-referrals/src/client.ts -C 2Repository: namehash/ensnode
Length of output: 196
🏁 Script executed:
# Let's search for other URL patterns in the file to see if there's a consistent pattern
cat -n packages/ens-referrals/src/client.ts | grep -E "new URL|\/api|ensanalytics" -A 1 -B 1Repository: namehash/ensnode
Length of output: 968
🏁 Script executed:
# Check if there are other similar methods and their URL patterns
rg "new URL" packages/ens-referrals/src/client.ts -A 2Repository: namehash/ensnode
Length of output: 527
🏁 Script executed:
# Let's also check the ensnode-sdk client to see what pattern it uses
cat -n packages/ensnode-sdk/src/client.ts | head -100Repository: namehash/ensnode
Length of output: 3377
🏁 Script executed:
# Search for URL patterns in ensnode-sdk to understand the convention
rg "new URL" packages/ensnode-sdk/src/client.ts -A 2 -B 1Repository: namehash/ensnode
Length of output: 1649
Fix inconsistent API path in getReferrerLeaderboardPage.
Line 127 uses /ensanalytics/referrers but should use /api/ensanalytics/referrers to match getReferrerDetail (line 232) and the standard pattern used throughout the codebase (all methods in ensnode-sdk use /api/ prefix).
🤖 Prompt for AI Agents
In `@packages/ens-referrals/src/client.ts` around lines 127 - 132,
getReferrerLeaderboardPage builds the URL with the wrong path; change the path
string in the URL constructor from `/ensanalytics/referrers` to
`/api/ensanalytics/referrers` so it matches getReferrerDetail and the rest of
the SDK; update the URL creation in packages/ens-referrals/src/client.ts inside
the getReferrerLeaderboardPage logic where new URL(...) is called and ensure
subsequent query param handling (page, recordsPerPage) remains unchanged.
| let responseData: unknown; | ||
| try { | ||
| responseData = await response.json(); | ||
| } catch { | ||
| throw new Error("Malformed response data: invalid JSON"); | ||
| } | ||
|
|
||
| // The API can return errors with 500 status, but they're still in the | ||
| // PaginatedAggregatedReferrersResponse format with responseCode: 'error' | ||
| // So we don't need to check response.ok here, just deserialize and let | ||
| // the caller handle the responseCode | ||
|
|
||
| return deserializeReferrerLeaderboardPageResponse( | ||
| responseData as SerializedReferrerLeaderboardPageResponse, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unsafe type assertion on unknown response data.
The responseData as SerializedReferrerLeaderboardPageResponse assertion bypasses type checking. While the subsequent deserializeReferrerLeaderboardPageResponse validates via Zod, a more explicit approach would improve clarity.
♻️ Consider explicit unknown handling
The current pattern works because the deserialize functions perform Zod validation. Consider documenting this reliance or adding a comment clarifying that validation occurs in the deserialize step.
- return deserializeReferrerLeaderboardPageResponse(
- responseData as SerializedReferrerLeaderboardPageResponse,
- );
+ // Zod validation occurs within deserializeReferrerLeaderboardPageResponse
+ return deserializeReferrerLeaderboardPageResponse(
+ responseData as SerializedReferrerLeaderboardPageResponse,
+ );🤖 Prompt for AI Agents
In `@packages/ens-referrals/src/client.ts` around lines 137 - 151, The code
unsafely asserts responseData to SerializedReferrerLeaderboardPageResponse
before deserialization; instead remove the type assertion and explicitly
validate responseData with the Zod-backed deserializer or perform an explicit
safeParse call so the shape is checked before treating it as the expected
type—update the call site around responseData and
deserializeReferrerLeaderboardPageResponse (or add a pre-check using the same
schema used by deserializeReferrerLeaderboardPageResponse) and add a short
comment clarifying that validation happens there.
Flip Dependency Relationship: ensnode-sdk ↔ ens-referrals
closes: #1519
Reviewer Focus (Read This First)
What reviewers should focus on
ENSReferralsClientclass inpackages/ens-referrals/src/client.ts- mirrorsENSNodeClientdesign patternsens-referralsorensnode-sdk)Problem & Motivation
Why this exists
ens-referralspackage was duplicating types (ChainId,AccountId,UnixTimestamp,Duration) fromensnode-sdkto avoid circular dependenciesens-referralsandensnode-sdkpackages #1519What Changed (Concrete)
What actually changed
Package Dependency Changes
ens-referralsdependency fromensnode-sdk/package.jsonensnode-sdkdependency toens-referrals/package.jsonCode Moved from ensnode-sdk to ens-referrals
packages/ensnode-sdk/src/ensanalytics/directorygetReferrerLeaderboardPage()andgetReferrerDetail()fromENSNodeClientNew Client in ens-referrals
ENSReferralsClientclass with both API methodspackages/ens-referrals/src/api/directory with all the serialization, deserialization and validationpackages/ens-referrals/src/internal.tsfor internal zod schema exports (similarly to how it's done inensnode-sdk; let me know if we wanted to drop Zod dependency)Type Consolidation
ChainId,AccountId,UnixTimestampandDurationdefinitionsensnode-sdk, including ENSApiDesign & Planning
How this approach was chosen
The new
ENSReferralsClientfollows the exact same design asENSNodeClient, with all the APIs having the same request/response schemas not to introduce any breaking changes on that level.ens-referralsandensnode-sdkpackages #1519 provided complete requirements and goalsENSNodeClientimplementation patterns exactlySelf-Review
What you caught yourself
ens-referralsaccurateAsOfinregistrar-actions/endpoint, only inregistrar-actions/parent-node@lightwalker-ethens-referralsCross-Codebase Alignment
Related code you checked
ENSNodeClientfor consistency with newENSReferralsClientEncodedReferrerensnode-schemato update docs (definitely requires your opinion)@namehash/ens-referrals,@namehash/ensnode-sdk,EncodedReferrer,ENS Awards,Awards,ensanalytics,ChainId,AccountId,UnixTimestamp,Duration,ENSNodeClientens-referrals, whileUnixTimestampis now imported fromensnode-sdk. Not sure if we want to make it more consistent by using Zod everywhere, or drop it altogether. So far, I left it unchanged, not moving intoensnode-sdk, which didn't seem to match, and it already had some Zod schemas for Unix timestamps.accurateAsOffieldDownstream & Consumer Impact
Who this affects and how
ENSNodeClientdropped mentions of ENS Analytics, removed any mentions of ENS Awards where possible inensnode-sdkand other ENSNode core code.ENSReferralsClientto parallelENSNodeClientnaming convention, some docs using "subjective interpretation for ENS Awards" to "required left-zero-padding", mentioned earlier that this would benefit from your opinion on how it should be phrasedTesting Evidence
How this was validated
EncodedReferrermoved toensnode-sdkScope Reductions
What you intentionally didn't do
Did not update ENSAwards site in this PR
Risk Analysis
How this could go wrong
ens-referrals, they all require updatingmain, and then update ENSAwards to use snapshot release with the new client?Pre-Review Checklist (Blocking)
Summary by CodeRabbit
Release Notes
New Features
ENSReferralsClientfor accessing referral leaderboard and referrer detail APIs with dedicated methodsgetReferrerLeaderboardPage()andgetReferrerDetail().Refactoring
ENSNodeClienttoENSReferralsClient.✏️ Tip: You can customize this high-level summary in your review settings.