Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughThis PR migrates address normalization from lowercase-based helpers to normalized-address APIs, shifts name types from NormalizedName to InterpretedName, consolidates UnixTimestamp imports to enssdk, introduces ENS_ROOT_NAME/ENS_ROOT_NODE, and updates many event-handler and schema typings accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR continues the ENS SDK refactor by standardizing address normalization across the stack, tightening the “interpreted name” model, and updating downstream packages/apps to use the new types/helpers.
Changes:
- Introduces
NormalizedAddress+ normalization helpers and updates GraphQL/Zod/address usage to consistently use lowercase EVM addresses. - Refactors ENS name utilities around
InterpretedName(new parent/name-hierarchy helpers, removal ofNormalizedNamebrand usage) and adds a UI helper component to enforce interpreted names. - Renames root constants (
ROOT_NODE→ENS_ROOT_NODE, addsENS_ROOT_NAME) and migrates manyUnixTimestampimports to come fromenssdk.
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | Swap UnixTimestamp type import to enssdk. |
| packages/enssdk/src/omnigraph/graphql.ts | Map GraphQL Address scalar to NormalizedAddress. |
| packages/enssdk/src/omnigraph/generated/schema.graphql | Update scalar docs to reference enssdk + lowercase address semantics. |
| packages/enssdk/src/lib/types/evm.ts | Add NormalizedAddress type and clarify EVM scalar docs. |
| packages/enssdk/src/lib/types/ens.ts | Remove NormalizedName branded type export. |
| packages/enssdk/src/lib/parse-reverse-name.ts | Normalize reverse-derived addresses via toNormalizedAddress. |
| packages/enssdk/src/lib/normalization.ts | Drop NormalizedName type-guard; tweak error messages. |
| packages/enssdk/src/lib/names.ts | Refactor hierarchy/parent helpers to use InterpretedName + ENS_ROOT_NAME. |
| packages/enssdk/src/lib/names.test.ts | Update tests for new name helpers/constants. |
| packages/enssdk/src/lib/interpreted-names-and-labels.ts | Add nameToInterpretedName; adjust casting helpers + docs. |
| packages/enssdk/src/lib/constants.ts | Add ENS_ROOT_NAME / ENS_ROOT_NODE; type ROOT_RESOURCE. |
| packages/enssdk/src/lib/address.ts | Add isNormalizedAddress/toNormalizedAddress/asNormalizedAddress. |
| packages/enssdk/src/lib/address.test.ts | Add tests for new address normalization helpers. |
| packages/ensnode-sdk/src/tokenscope/name-token.ts | Use getParentInterpretedName and handle root-parent null. |
| packages/ensnode-sdk/src/shared/zod-schemas.ts | Rename lowercase-address schema to normalized-address schema. |
| packages/ensnode-sdk/src/shared/types.ts | Source UnixTimestamp from enssdk for BlockRef. |
| packages/ensnode-sdk/src/shared/interpretation/interpret-record-values.ts | Use InterpretedName + toNormalizedAddress for record interpretation. |
| packages/ensnode-sdk/src/shared/datetime.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/shared/cache/ttl-cache.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/shared/cache/swr-cache.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/registrars/zod-schemas.ts | Use normalized-address schema for registrar payloads. |
| packages/ensnode-sdk/src/registrars/registration-lifecycle.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/omnigraph-api/example-queries.ts | Normalize hardcoded example addresses. |
| packages/ensnode-sdk/src/indexing-status/realtime-indexing-status-projection.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/cross-chain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/response.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/request.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/filters.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/name-tokens/response.ts | Import UnixTimestamp from enssdk. |
| packages/enskit/src/react/omnigraph/index.ts | Re-export omnigraph components. |
| packages/enskit/src/react/omnigraph/components/index.tsx | Barrel export for new component. |
| packages/enskit/src/react/omnigraph/components/ensure-interpreted-name.tsx | Add EnsureInterpretedName helper component. |
| packages/ens-referrals/src/v1/referrer-metrics.ts | Normalize referrers via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts | Normalize referrer comparisons via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts | Normalize event referrers via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.ts | Switch to normalized-address schema + uniqueness by normalized form. |
| packages/ens-referrals/src/v1/award-models/pie-split/api/zod-schemas.ts | Switch to normalized-address schema. |
| packages/ens-referrals/src/v1/api/zod-schemas.ts | Switch to normalized-address schema for requests. |
| packages/ens-referrals/src/v1/address.ts | Remove local normalizeAddress helper. |
| packages/ens-referrals/src/referrer-metrics.ts | Normalize referrers via toNormalizedAddress. |
| packages/ens-referrals/src/api/zod-schemas.ts | Switch to normalized-address schema. |
| packages/ens-referrals/src/address.ts | Remove local normalizeAddress helper. |
| packages/datasources/src/invariants.test.ts | Adjust address-lowercase invariant comment/check. |
| examples/enskit-react-example/src/DomainView.tsx | Use EnsureInterpretedName + new parent-name helper in example UI. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/ThreeDNSToken.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Resolver.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Registrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/plugins/subgraph/handlers/Registry.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensindexer/src/plugins/registrars/shared/lib/registrar-events.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensindexer/src/plugins/registrars/shared/lib/registrar-action.ts | Add clarifying doc comment re: toLowerCase() keying. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv2Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ETHRegistrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/EnhancedAccessControl.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE + normalize address types. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/lib/trace-transaction-helpers.ts | Normalize extracted trace addresses via toNormalizedAddress. |
| apps/ensindexer/src/lib/tokenscope/seaport-types.ts | Type Seaport event addresses as NormalizedAddress. |
| apps/ensindexer/src/lib/subgraph/subgraph-helpers.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensindexer/src/lib/heal-addr-reverse-subname-label.ts | Treat event owner address as NormalizedAddress. |
| apps/ensindexer/src/config/validations.ts | Use isNormalizedAddress for contract-config validation. |
| apps/ensapi/src/omnigraph-api/schema/scalars.ts | Parse Address scalar via normalized-address schema; update docs. |
| apps/ensapi/src/omnigraph-api/schema/permissions.integration.test.ts | Update tests to expect normalized addresses/topics casing. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Update tests to expect normalized topics/from casing. |
| apps/ensapi/src/omnigraph-api/schema/account.integration.test.ts | Normalize devnet addresses and update assertions. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensapi/src/omnigraph-api/builder.ts | Update Address scalar output typing to NormalizedAddress. |
| apps/ensapi/src/lib/resolution/resolve-with-universal-resolver.ts | Require InterpretedName for resolution call execution. |
| apps/ensapi/src/lib/resolution/resolve-calls-and-results.ts | Tweak error recording logic around reverts vs unexpected errors. |
| apps/ensapi/src/lib/resolution/packetToBytes.test.ts | Add test coverage for encoded-labelhash DNS encoding behavior. |
| apps/ensapi/src/lib/resolution/forward-resolution.ts | Enforce InterpretedName invariant via asInterpretedName. |
| apps/ensapi/src/lib/protocol-acceleration/find-resolver.ts | Use InterpretedName throughout resolver-finding logic. |
| apps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensapi/src/lib/handlers/params.schema.ts | Use normalized-address schema for address param parsing. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.ts | Use normalized-address schema for referrer params. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api-v1.routes.ts | Use normalized-address schema for referrer params. |
| apps/ensapi/src/handlers/api/meta/realtime-api.test.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.ts | Use normalized-address schema for decodedReferrer filter. |
| apps/ensapi/src/handlers/api/explore/name-tokens-api.ts | Swap root constants + parent-name helper usage. |
| apps/ensadmin/src/components/indexing-status/backfill-status.tsx | Move UnixTimestamp type import to enssdk. |
| apps/ensadmin/src/app/mock/display-identity/page.tsx | Normalize selected addresses via toNormalizedAddress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function nameToInterpretedName(name: Name): InterpretedName { | ||
| return interpretedLabelsToInterpretedName( | ||
| name | ||
| .split(".") | ||
| .map((label) => { | ||
| if (isEncodedLabelHash(label)) return label; | ||
| if (isNormalizedLabel(label)) return label; | ||
| return encodeLabelHash(labelhashLiteralLabel(asLiteralLabel(label))); | ||
| }) | ||
| .map(asInterpretedLabel), | ||
| ); |
There was a problem hiding this comment.
nameToInterpretedName("") currently returns a single encoded-labelhash label (it splits into [""] and encodes the empty label) rather than the ENS root InterpretedName (""). This also means inputs with empty segments like "foo..eth" or trailing dots get silently reinterpreted instead of being treated as malformed. Consider special-casing the root name (return "") and rejecting/throwing on empty label segments (or explicitly defining how empty labels should be represented) so callers like EnsureInterpretedName don't redirect to an unintended name.
There was a problem hiding this comment.
@shrugs This looks like a nice catch from Copilot. If the user input name is "" then the returned InterpretedName should also be "".
| /** | ||
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. | ||
| */ | ||
| export function asLiteralLabel(label: Label): LiteralLabel { | ||
| return label as LiteralLabel; | ||
| } |
There was a problem hiding this comment.
asLiteralLabel’s docstring says it “validates and casts”, but the implementation is a raw cast and never validates anything. Either reintroduce validation (and clearly document what is/isn’t allowed, e.g. empty labels) or adjust the docstring to avoid implying validation that doesn’t happen.
| if (fetching) return <p>Loading...</p>; | ||
| if (error) return <p>Error: {error.message}</p>; | ||
| if (!data?.domain) return <p>Not found</p>; | ||
| if (!data?.domain) return <p>A Domain with InterpretedName '{name}' was not found.</p>; | ||
|
|
There was a problem hiding this comment.
This JSX renders '{name}' literally (including braces) rather than interpolating the name variable. Use JSX interpolation (e.g. split the string and insert {name}) so the not-found message includes the actual InterpretedName.
| // if a user accesses /domain directly, redirec to /domain/etc | ||
| if (params.name === undefined || params.name === "") return <Navigate to="/domain/eth" replace />; |
There was a problem hiding this comment.
Typo in comment: “redirec” → “redirect”.
| // must be lowercase | ||
| expect( | ||
| // TODO: could use enssdk#isNormalizedAddress here | ||
| contractConfig.address === contractConfig.address.toLowerCase(), | ||
| `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not is lowercase format.`, | ||
| ).toBe(true); |
There was a problem hiding this comment.
Assertion message has a grammatical typo: “is not is lowercase format.” should be something like “is not in lowercase format.” (also consider updating the TODO to use enssdk#isNormalizedAddress as noted).
Greptile SummaryThis PR continues the ENS SDK refactoring across 88 files, introducing three main changes: (1) renaming Confidence Score: 5/5Safe to merge — refactoring is consistent and well-tested; all remaining findings are P2 style suggestions. All three main changes (NormalizedName→InterpretedName rename, NormalizedAddress introduction, UnixTimestamp re-export removal) are applied consistently across 88 files with no logic regressions observed. The only findings are P2: a pre-existing public API typo ( packages/enssdk/src/lib/interpreted-names-and-labels.ts — public API typo
|
| Filename | Overview |
|---|---|
| packages/enssdk/src/lib/types/evm.ts | Adds NormalizedAddress as a structural alias of Address (no brand) with clear documentation of the trade-off; safe but offers no compile-time enforcement. |
| packages/enssdk/src/lib/address.ts | Replaces asLowerCaseAddress with three well-tested helpers: isNormalizedAddress, toNormalizedAddress, and asNormalizedAddress; logic is correct. |
| packages/enssdk/src/lib/interpreted-names-and-labels.ts | Core label/name interpretation utilities; contains the pre-existing public API typo isInterpetedLabel (missing 'r') that is exported and consumed downstream — worth fixing while the file is being touched. |
| packages/enssdk/src/lib/names.ts | Refactored getParentInterpretedName to return `InterpretedName |
| packages/ensnode-sdk/src/shared/zod-schemas.ts | Adds makeNormalizedAddressSchema using toNormalizedAddress; correct and well-structured Zod schema for address normalization. |
| apps/ensapi/src/omnigraph-api/schema/scalars.ts | Uses misspelled isInterpetedLabel import from enssdk — functional since the export name matches, but the downstream typo is perpetuated. |
| packages/enssdk/src/lib/address.test.ts | Comprehensive tests for all three new address helpers covering lowercase, checksummed, zero, short, and non-hex inputs. |
| packages/enssdk/src/lib/names.test.ts | Tests updated to use getParentInterpretedName and ENS_ROOT_NAME; coverage for root, TLD, and multi-level names looks complete. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Address\nviem alias] --> B[NormalizedAddress\nstructural alias = Address]
B --> C[isNormalizedAddress\nchecks lowercase + valid]
B --> D[toNormalizedAddress\nlowercase + validate]
B --> E[asNormalizedAddress\nassert already normalized]
F[Name\nstring] --> G[InterpretedName\nnominally branded]
G --> H[getParentInterpretedName\nreturns InterpretedName or null]
G --> I[getNameHierarchy\nreturns InterpretedName array]
G --> J[ENS_ROOT_NAME\nasInterpretedName of empty string]
K[Label\nstring] --> L[InterpretedLabel\nnominally branded]
L --> M[isInterpetedLabel typo\nchecks EncodedLabelHash or normalized]
D --> N[makeNormalizedAddressSchema\nZod schema using toNormalizedAddress]
G --> O[makeReinterpretedNameSchema\nZod schema for InterpretedName]
Comments Outside Diff (2)
-
packages/enssdk/src/lib/interpreted-names-and-labels.ts, line 117 (link)Typo in public API function name
isInterpetedLabelis missing therin "Interpreted" — it should beisInterpretedLabel. Since this function is exported as part of the publicenssdkAPI (re-exported vialib/index.ts) and already consumed under the misspelled name inapps/ensapi/src/omnigraph-api/schema/scalars.ts, fixing it now is a breaking rename. Worth addressing while this file is being touched. -
packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts, line 22 (link)NormalizedAddresswould better express intent hereThe field
referrer: Addresscarries the invariant@invariant Guaranteed to be a valid EVM address in lowercase format, which is exactly whatNormalizedAddressnow models. The same pattern appears inReferrerMetrics.referrerin bothpackages/ens-referrals/src/referrer-metrics.tsandpackages/ens-referrals/src/v1/referrer-metrics.ts. SinceNormalizedAddress = Addressstructurally, this is non-breaking, but switching the declared type would make the invariant self-documenting without requiring the runtimevalidateLowercaseAddresscall.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "refactor Address to NormalizedAddress wh..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * EventWithArgs<{ address: NormalizedAddress }> in all of the Ponder event handler args to | ||
| * declare that the incoming event.args.address is a NormalizedAddress. | ||
| */ | ||
| export type NormalizedAddress = Address; |
There was a problem hiding this comment.
type NormalizedAddress = Address is a pure structural alias — TypeScript treats the two types as identical, so a raw checksummed Address can be silently assigned to a NormalizedAddress parameter without any cast or error. The comment acknowledges this trade-off, but it means callers relying on the type alone for correctness guarantees (e.g. EventWithArgs<{ address: NormalizedAddress }>) won't get a compile-time error if a non-lowercase address is passed. Consider noting in the doc comment that callers must call toNormalizedAddress() or asNormalizedAddress() themselves before assigning to this type.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ens-referrals/src/referrer-metrics.ts (1)
67-79:⚠️ Potential issue | 🔴 CriticalReplace lowercase validation with normalized-address validation.
buildReferrerMetrics(Line 67) normalizes withtoNormalizedAddress, which returns EIP-55 checksummed addresses (mixed-case per viem/enssdk documentation). However,validateReferrerMetrics(Line 78) enforces lowercase viavalidateLowercaseAddress, which will reject all checksummed addresses. This causes every call to throw—a blocking bug.Suggested fix
-import { validateLowercaseAddress } from "./address"; @@ export const validateReferrerMetrics = (metrics: ReferrerMetrics): void => { - validateLowercaseAddress(metrics.referrer); + if (toNormalizedAddress(metrics.referrer) !== metrics.referrer) { + throw new Error( + `ReferrerMetrics.referrer must be normalized. Got: ${metrics.referrer}.`, + ); + } validateNonNegativeInteger(metrics.totalReferrals);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ens-referrals/src/referrer-metrics.ts` around lines 67 - 79, buildReferrerMetrics normalizes referrer with toNormalizedAddress (EIP-55 checksummed), but validateReferrerMetrics still calls validateLowercaseAddress causing valid checksummed addresses to be rejected; replace the lowercase check with the matching normalized/checksummed validator — e.g., in validateReferrerMetrics remove validateLowercaseAddress(metrics.referrer) and call validateNormalizedAddress(metrics.referrer) (or the project's EIP-55/normalized address validator) so the validation matches toNormalizedAddress, keeping the existing validateNonNegativeInteger(metrics.totalReferrals) check.apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts (1)
119-126: 🧹 Nitpick | 🔵 TrivialInternal
handleTransferfunction still usesAddresstype.The internal helper
handleTransferusesto: Address(line 125), but it receivesNormalizedAddressvalues fromhandleTransferSingleandhandleTransferBatch. Consider updating this toNormalizedAddressfor consistency.♻️ Proposed fix for type consistency
async function handleTransfer( context: IndexingEngineContext, event: EventWithArgs, eventId: string, tokenId: bigint, - to: Address, + to: NormalizedAddress, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts` around lines 119 - 126, The internal handler handleTransfer currently declares the recipient parameter as to: Address but callers (handleTransferSingle and handleTransferBatch) pass NormalizedAddress; update handleTransfer's signature to use to: NormalizedAddress for type consistency, adjust any imports to include NormalizedAddress instead of Address, and remove or update any inline casts/usages inside handleTransfer that assume Address so they work with NormalizedAddress.
🤖 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/lib/resolution/resolve-calls-and-results.ts`:
- Around line 200-204: The catch block that currently only calls
span.recordException(error) when error instanceof Error should be changed to
record all throwables: always call span.recordException with an Error instance
by using the original Error if error instanceof Error, otherwise create a new
Error(String(error ?? 'non-error thrown')) before recording; keep the subsequent
throw error to rethrow the original value. Update the code around the catch in
resolve-calls-and-results (where span.recordException is invoked) to handle
null/undefined and non-Error objects as described.
In `@apps/ensapi/src/omnigraph-api/schema/scalars.ts`:
- Around line 39-43: The scalar type declaration for Address is misleading:
builder.scalarType("Address", ...) still describes addresses as "in all
lowercase" but the parse logic uses makeNormalizedAddressSchema which returns
EIP-55 checksummed (mixed-case) addresses; update the description to reflect
checksummed addresses or normalized EIP-55 format and ensure serialize/parse
semantics match that description (reference builder.scalarType("Address"), the
Address type, and makeNormalizedAddressSchema).
In `@apps/ensindexer/src/config/validations.ts`:
- Around line 130-133: The validation currently sets isValidAddress using both
isAddress(...) and isNormalizedAddress(...); remove the redundant isAddress call
and set isValidAddress by only calling
isNormalizedAddress(contractConfig.address as Address) (keep the variable name
isValidAddress and the contractConfig.address reference) so the check still
ensures lowercase normalized address without duplicating validation work.
In `@examples/enskit-react-example/src/DomainView.tsx`:
- Around line 85-108: Fix the typo in the inline comment inside the DomainView
component: change "redirec to /domain/etc" to "redirect to /domain/etc" (the
comment immediately above the Navigate fallback that checks params.name in the
DomainView function using useParams and Navigate).
In `@packages/datasources/src/invariants.test.ts`:
- Line 26: The assertion message in invariants.test.ts contains a grammar
mistake in the template literal for the contract address error; update the
string `The ContractConfig '${namespace}' > '${datasourceName}' >
'${contractName}' > '${contractConfig.address}' is not is lowercase format.` to
read `The ContractConfig '${namespace}' > '${datasourceName}' >
'${contractName}' > '${contractConfig.address}' is not in lowercase format.` so
the phrase uses "in lowercase format" (locate the template literal in the test
that references namespace, datasourceName, contractName, and
contractConfig.address).
In
`@packages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.ts`:
- Around line 58-60: The duplicate-check refinement currently re-normalizes
addresses by calling toNormalizedAddress(item.referrer) even though
item.referrer is already normalized by the schema; in the refinement callback
replace the toNormalizedAddress call and map directly using item.referrer (i.e.,
const addresses = items.map(item => item.referrer)) so the new Set size
comparison stays the same, and remove the unnecessary toNormalizedAddress
invocation to avoid redundant work in the refinement.
In `@packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts`:
- Around line 166-169: Change the model so disqualifications[].referrer is typed
as NormalizedAddress and ensure referrer values are normalized at the model
boundary (where disqualifications are created/loaded) rather than at every
consumer; update the disqualifications type alias and any factories/parsers to
call toNormalizedAddress once when constructing a disqualification, then in
rules.ts remove the runtime toNormalizedAddress(referrer) and the
normalizedReferrer local and compare rules.disqualifications[*].referrer
directly to referrer (now already a NormalizedAddress) so normalization is
enforced at the type level.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts`:
- Around line 130-133: The invariant
invariant_registrarActionDecodedReferrerBasedOnRawReferrer currently compares
decodedReferrer against rawReferrer using .toLowerCase(); update it to use the
same normalization as decodedReferrer by importing toNormalizedAddress from
"enssdk" and replacing the .toLowerCase() usage with
toNormalizedAddress(rawReferrer) (so both sides use
makeNormalizedAddressSchema/toNormalizedAddress), and add the import for
toNormalizedAddress at the top of the file.
In `@packages/enssdk/src/lib/constants.ts`:
- Around line 8-13: ENS_ROOT_NODE currently recreates the empty interpreted name
inline; change ENS_ROOT_NODE to use the existing ENS_ROOT_NAME constant as its
single source of truth by passing ENS_ROOT_NAME into namehashInterpretedName
(keep the Node type), i.e., replace the asInterpretedName("") usage in the
ENS_ROOT_NODE initializer with ENS_ROOT_NAME so the empty name is defined only
once and cannot drift.
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts`:
- Around line 218-224: The JSDoc for asLiteralLabel incorrectly claims it
"Validates and casts" while the implementation only performs a type cast; update
the documentation or implementation: either reintroduce runtime validation
inside the asLiteralLabel(label: Label): LiteralLabel function (perform checks
on format/characters and throw a clear error on invalid input) or change the
JSDoc to say it "Casts" (e.g., "Casts a string to a LiteralLabel without runtime
validation") so the comment accurately reflects the behavior; reference the
asLiteralLabel function to locate the change.
- Around line 29-40: nameToInterpretedName incorrectly transforms the ENS root
("") because "".split(".") yields [""] which gets encoded as a non-root label;
fix by handling the root early: in nameToInterpretedName, if name === "" return
interpretedLabelsToInterpretedName([]) (i.e., an empty labels array) before
calling .split and the label mapping logic so you don't encode a root label;
keep the rest of the mapping using asInterpretedLabel, isEncodedLabelHash,
isNormalizedLabel, encodeLabelHash, and labelhashLiteralLabel as-is for non-root
names.
---
Outside diff comments:
In `@apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts`:
- Around line 119-126: The internal handler handleTransfer currently declares
the recipient parameter as to: Address but callers (handleTransferSingle and
handleTransferBatch) pass NormalizedAddress; update handleTransfer's signature
to use to: NormalizedAddress for type consistency, adjust any imports to include
NormalizedAddress instead of Address, and remove or update any inline
casts/usages inside handleTransfer that assume Address so they work with
NormalizedAddress.
In `@packages/ens-referrals/src/referrer-metrics.ts`:
- Around line 67-79: buildReferrerMetrics normalizes referrer with
toNormalizedAddress (EIP-55 checksummed), but validateReferrerMetrics still
calls validateLowercaseAddress causing valid checksummed addresses to be
rejected; replace the lowercase check with the matching normalized/checksummed
validator — e.g., in validateReferrerMetrics remove
validateLowercaseAddress(metrics.referrer) and call
validateNormalizedAddress(metrics.referrer) (or the project's EIP-55/normalized
address validator) so the validation matches toNormalizedAddress, keeping the
existing validateNonNegativeInteger(metrics.totalReferrals) check.
🪄 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: 08e38f45-8726-4f05-bc61-391d4ba45bf1
⛔ Files ignored due to path filters (1)
packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (87)
apps/ensadmin/src/app/mock/display-identity/page.tsxapps/ensadmin/src/components/indexing-status/backfill-status.tsxapps/ensapi/src/handlers/api/explore/name-tokens-api.tsapps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.tsapps/ensapi/src/handlers/api/meta/realtime-api.test.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api-v1.routes.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.tsapps/ensapi/src/lib/handlers/params.schema.tsapps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.tsapps/ensapi/src/lib/protocol-acceleration/find-resolver.tsapps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensapi/src/lib/resolution/packetToBytes.test.tsapps/ensapi/src/lib/resolution/resolve-calls-and-results.tsapps/ensapi/src/lib/resolution/resolve-with-universal-resolver.tsapps/ensapi/src/omnigraph-api/builder.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/schema/account.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/permissions.integration.test.tsapps/ensapi/src/omnigraph-api/schema/scalars.tsapps/ensindexer/src/config/validations.tsapps/ensindexer/src/lib/heal-addr-reverse-subname-label.tsapps/ensindexer/src/lib/subgraph/subgraph-helpers.tsapps/ensindexer/src/lib/tokenscope/seaport-types.tsapps/ensindexer/src/lib/trace-transaction-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ETHRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/EnhancedAccessControl.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv2Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-action.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-events.tsapps/ensindexer/src/plugins/subgraph/plugins/subgraph/handlers/Registry.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Registrar.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Registry.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Resolver.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/ThreeDNSToken.tsexamples/enskit-react-example/src/DomainView.tsxpackages/datasources/src/invariants.test.tspackages/ens-referrals/src/address.tspackages/ens-referrals/src/api/zod-schemas.tspackages/ens-referrals/src/referrer-metrics.tspackages/ens-referrals/src/v1/address.tspackages/ens-referrals/src/v1/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/pie-split/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/rules.tspackages/ens-referrals/src/v1/referrer-metrics.tspackages/enskit/src/react/omnigraph/components/ensure-interpreted-name.tsxpackages/enskit/src/react/omnigraph/components/index.tsxpackages/enskit/src/react/omnigraph/index.tspackages/ensnode-sdk/src/ensapi/api/name-tokens/response.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/filters.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/request.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/response.tspackages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/cross-chain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/realtime-indexing-status-projection.tspackages/ensnode-sdk/src/omnigraph-api/example-queries.tspackages/ensnode-sdk/src/registrars/registration-lifecycle.tspackages/ensnode-sdk/src/registrars/zod-schemas.tspackages/ensnode-sdk/src/shared/cache/swr-cache.tspackages/ensnode-sdk/src/shared/cache/ttl-cache.tspackages/ensnode-sdk/src/shared/datetime.tspackages/ensnode-sdk/src/shared/interpretation/interpret-record-values.tspackages/ensnode-sdk/src/shared/types.tspackages/ensnode-sdk/src/shared/zod-schemas.tspackages/ensnode-sdk/src/tokenscope/name-token.tspackages/enssdk/src/lib/address.test.tspackages/enssdk/src/lib/address.tspackages/enssdk/src/lib/constants.tspackages/enssdk/src/lib/interpreted-names-and-labels.tspackages/enssdk/src/lib/names.test.tspackages/enssdk/src/lib/names.tspackages/enssdk/src/lib/normalization.tspackages/enssdk/src/lib/parse-reverse-name.tspackages/enssdk/src/lib/types/ens.tspackages/enssdk/src/lib/types/evm.tspackages/enssdk/src/omnigraph/graphql.tspackages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx
💤 Files with no reviewable changes (3)
- packages/ens-referrals/src/address.ts
- packages/ens-referrals/src/v1/address.ts
- packages/enssdk/src/lib/types/ens.ts
| // log the error if it wasn't a ContractFunctionExecutionError | ||
| if (error instanceof Error) span.recordException(error); | ||
|
|
||
| // then rethrow | ||
| throw error; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=tsx --type=js --type=jsx '\bthrow\s+("|\047|`|\{|\[)' -C2
rg -nP --type=ts --type=tsx --type=js --type=jsx '\bthrow\s+[A-Za-z_$][A-Za-z0-9_$]*\s*;' -C2Repository: namehash/ensnode
Length of output: 115
🏁 Script executed:
cat -n apps/ensapi/src/lib/resolution/resolve-calls-and-results.ts | sed -n '190,215p'Repository: namehash/ensnode
Length of output: 944
🏁 Script executed:
#!/bin/bash
# Search for non-Error throws using corrected syntax
rg -n --type=ts '\bthrow\s+("|\047|`|\{|\[)' --max-count=20
rg -n --type=ts '\bthrow\s+[a-zA-Z_$][a-zA-Z0-9_$]*\s*;' --max-count=20Repository: namehash/ensnode
Length of output: 3756
🌐 Web query:
OpenTelemetry span.recordException() JavaScript API documentation
💡 Result:
Span.recordException() (OpenTelemetry JavaScript / TypeScript)
Method (JS API @opentelemetry/api)
span.recordException(exception: Exception, time?: TimeInput): voidIt “sets exception as a span event”. [1]
Parameter types (as defined by the JS API docs)
Exceptionis a union that includesstringand structured exception objects (e.g., withname,message,code). [1]TimeInputcan be anHrTime,number, orDate. [1]
What it does (spec-level behavior)
RecordException is a specialized addEvent that must record an exception event using the exception semantic conventions. The exact signature is language-specific, but it must record an exception as an event and allow optional extra attributes (language permitting). [2]
Exception event fields (semantic conventions)
Exception events use attributes like exception.type, exception.message, and exception.stacktrace (for JavaScript, typically from error.stack). [3]
Minimal usage example (Node.js/JS manual instrumentation)
try {
// ...
} catch (err) {
span.recordException(err);
// often paired with:
// span.setStatus({ code: SpanStatusCode.ERROR, message: String(err?.message ?? err) });
throw err;
} finally {
span.end();
}(Example usage shown in the OpenTelemetry JS instrumentation docs.) [4]
Sources: OpenTelemetry JS API docs for Span.recordException and related types [1], Trace API specification (“Record Exception”) [2], exception semantic conventions [3], OpenTelemetry JavaScript instrumentation guide example [4].
References:
[1] (jsr.io)
[2] (opentelemetry.io)
[3] (opentelemetry.io)
[4] (opentelemetry.io)
Citations:
- 1: https://jsr.io/%40opentelemetry/api/doc
- 2: https://opentelemetry.io/docs/specs/otel/trace/api/
- 3: https://opentelemetry.io/docs/specs/semconv/exceptions/exceptions-spans/?utm_source=openai
- 4: https://opentelemetry.io/uk/docs/languages/js/instrumentation/
Record all throwables in telemetry, not just Error instances.
The catch block at line 201 only records telemetry when error instanceof Error. However, JavaScript allows throwing any value (strings, null, objects, etc.). The codebase contains examples of non-Error throws (e.g., throw null; in tests, throw element; in omnigraph-api), so telemetry will be incomplete for these cases.
To fix, wrap non-Error values as Error objects when recording:
- // log the error if it wasn't a ContractFunctionExecutionError
- if (error instanceof Error) span.recordException(error);
+ // record all throwables for telemetry
+ if (error instanceof Error) {
+ span.recordException(error);
+ } else {
+ span.recordException(new Error(String(error)));
+ }
// then rethrow
throw error;📝 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.
| // log the error if it wasn't a ContractFunctionExecutionError | |
| if (error instanceof Error) span.recordException(error); | |
| // then rethrow | |
| throw error; | |
| // record all throwables for telemetry | |
| if (error instanceof Error) { | |
| span.recordException(error); | |
| } else { | |
| span.recordException(new Error(String(error))); | |
| } | |
| // then rethrow | |
| throw error; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/lib/resolution/resolve-calls-and-results.ts` around lines 200
- 204, The catch block that currently only calls span.recordException(error)
when error instanceof Error should be changed to record all throwables: always
call span.recordException with an Error instance by using the original Error if
error instanceof Error, otherwise create a new Error(String(error ?? 'non-error
thrown')) before recording; keep the subsequent throw error to rethrow the
original value. Update the code around the catch in resolve-calls-and-results
(where span.recordException is invoked) to handle null/undefined and non-Error
objects as described.
| builder.scalarType("Address", { | ||
| description: "Address represents a lowercase (unchecksummed) viem#Address.", | ||
| description: "Address represents an EVM Address in all lowercase.", | ||
| serialize: (value: Address) => value.toString(), | ||
| parseValue: (value) => makeLowercaseAddressSchema("Address").parse(value), | ||
| parseValue: (value) => makeNormalizedAddressSchema("Address").parse(value), | ||
| }); |
There was a problem hiding this comment.
Description is inconsistent with the new normalization behavior.
The description still says "in all lowercase" but the schema now uses makeNormalizedAddressSchema, which produces checksummed addresses (mixed case per EIP-55), not lowercase.
📝 Proposed fix
builder.scalarType("Address", {
- description: "Address represents an EVM Address in all lowercase.",
+ description: "Address represents a normalized EVM Address (EIP-55 checksummed).",
serialize: (value: Address) => value.toString(),
parseValue: (value) => makeNormalizedAddressSchema("Address").parse(value),
});📝 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.
| builder.scalarType("Address", { | |
| description: "Address represents a lowercase (unchecksummed) viem#Address.", | |
| description: "Address represents an EVM Address in all lowercase.", | |
| serialize: (value: Address) => value.toString(), | |
| parseValue: (value) => makeLowercaseAddressSchema("Address").parse(value), | |
| parseValue: (value) => makeNormalizedAddressSchema("Address").parse(value), | |
| }); | |
| builder.scalarType("Address", { | |
| description: "Address represents a normalized EVM Address (EIP-55 checksummed).", | |
| serialize: (value: Address) => value.toString(), | |
| parseValue: (value) => makeNormalizedAddressSchema("Address").parse(value), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/omnigraph-api/schema/scalars.ts` around lines 39 - 43, The
scalar type declaration for Address is misleading: builder.scalarType("Address",
...) still describes addresses as "in all lowercase" but the parse logic uses
makeNormalizedAddressSchema which returns EIP-55 checksummed (mixed-case)
addresses; update the description to reflect checksummed addresses or normalized
EIP-55 format and ensure serialize/parse semantics match that description
(reference builder.scalarType("Address"), the Address type, and
makeNormalizedAddressSchema).
| expect( | ||
| // TODO: could use enssdk#isNormalizedAddress here | ||
| contractConfig.address === contractConfig.address.toLowerCase(), | ||
| `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not is lowercase format.`, |
There was a problem hiding this comment.
Fix grammatical error in assertion message.
The error message contains a grammatical error: "is not is lowercase format" should be "is not in lowercase format".
📝 Proposed fix
- `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not is lowercase format.`,
+ `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not in lowercase format.`,📝 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.
| `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not is lowercase format.`, | |
| `The ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' > '${contractConfig.address}' is not in lowercase format.`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/datasources/src/invariants.test.ts` at line 26, The assertion
message in invariants.test.ts contains a grammar mistake in the template literal
for the contract address error; update the string `The ContractConfig
'${namespace}' > '${datasourceName}' > '${contractName}' >
'${contractConfig.address}' is not is lowercase format.` to read `The
ContractConfig '${namespace}' > '${datasourceName}' > '${contractName}' >
'${contractConfig.address}' is not in lowercase format.` so the phrase uses "in
lowercase format" (locate the template literal in the test that references
namespace, datasourceName, contractName, and contractConfig.address).
| const normalizedReferrer = toNormalizedAddress(referrer); | ||
| const isAdminDisqualified = rules.disqualifications.some( | ||
| (d) => d.referrer === normalizedReferrer, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer type-level normalized referrer addresses over repeated runtime normalization.
Line 166 normalizes at comparison time, but this invariant is stable and reused. Consider typing disqualifications[].referrer as NormalizedAddress and comparing directly, so normalization is guaranteed at the model boundary rather than per call site.
As per coding guidelines "Do not duplicate definitions across multiple locations; use type aliases to document invariants, with each invariant documented exactly once on its type alias."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts` around
lines 166 - 169, Change the model so disqualifications[].referrer is typed as
NormalizedAddress and ensure referrer values are normalized at the model
boundary (where disqualifications are created/loaded) rather than at every
consumer; update the disqualifications type alias and any factories/parsers to
call toNormalizedAddress once when constructing a disqualification, then in
rules.ts remove the runtime toNormalizedAddress(referrer) and the
normalizedReferrer local and compare rules.disqualifications[*].referrer
directly to referrer (now already a NormalizedAddress) so normalization is
enforced at the type level.
There was a problem hiding this comment.
This is a good suggestion from CodeRabbit. I created a follow-up issue for Mykola: #1910
There was a problem hiding this comment.
@lightwalker-eth, thank you for creating the follow-up issue! I've noted #1910 for tracking the NormalizedAddress typing improvement on disqualifications[].referrer. Once that lands, the per-call-site normalization in isReferrerQualifiedRevShareLimit can be removed cleanly. 🐇
🧠 Learnings used
Learnt from: Goader
Repo: namehash/ensnode PR: 1663
File: packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts:74-96
Timestamp: 2026-02-24T15:53:20.841Z
Learning: In packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts, AwardedReferrerMetricsRevShareLimit.standardAwardValue should not duplicate the generic PriceUsdc non-negative invariant; keep only the context-specific formula (“rules.qualifiedRevenueShare × totalBaseRevenueContribution”) and notes about pool independence.
Learnt from: Y3drk
Repo: namehash/ensnode PR: 1849
File: packages/ens-referrals/src/v1/edition-defaults.test.ts:118-133
Timestamp: 2026-03-31T16:10:52.187Z
Learning: In `packages/ens-referrals/src/v1/edition-defaults.test.ts`, the `disqualifications` field of `ReferralProgramRulesRevShareLimit` is intentionally NOT asserted in the production-parity tests because disqualifications can change very dynamically and including them would cause brittle, frequently-failing tests. A code comment should note this omission is deliberate.
Learnt from: Goader
Repo: namehash/ensnode PR: 1874
File: packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts:154-164
Timestamp: 2026-04-08T15:33:31.919Z
Learning: In `packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts`, the per-event integer rounding accumulation in the already-qualified branch (computing incremental uncapped award from event delta rather than cumulative total) is a known, intentionally accepted issue. The effect is bounded/limited, and award computation logic cannot be changed during an active edition — the algorithm is frozen once an edition goes live. Do not flag this rounding pattern as a bug in future reviews.
Learnt from: Goader
Repo: namehash/ensnode PR: 1663
File: packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts:74-96
Timestamp: 2026-02-24T15:53:06.633Z
Learning: In TypeScript code reviews, prefer placing invariants on type aliases only when the invariant is context-independent or reused across multiple fields. If an invariant depends on surrounding rules or object semantics (e.g., field-specific metrics), keep the invariant as a field JSDoc instead. This guideline applies to TS files broadly (e.g., the repo's v1/award-models and similar modules).
Learnt from: Y3drk
Repo: namehash/ensnode PR: 1861
File: docs/ensnode.io/src/components/organisms/ENSNodeSuite.tsx:94-101
Timestamp: 2026-04-02T07:15:53.785Z
Learning: In `docs/ensnode.io/src/components/organisms/ENSNodeSuite.tsx` (namehash/ensnode, TypeScript/React), documentation links in the `ENSNodeSuite` component intentionally use `target="_blank"` and `rel="noopener noreferrer"` even for internal relative paths (e.g., `/ensadmin`, `/ensindexer`, `/ensrainbow`). Do not flag this as an issue in future reviews.
Learnt from: Goader
Repo: namehash/ensnode PR: 1732
File: packages/ens-referrals/src/v1/api/serialized-types.ts:43-47
Timestamp: 2026-03-06T23:35:31.761Z
Learning: In the packages/ens-referrals codebase, during active development breaking changes to exported public TypeScript types may occur and deprecated aliases for removed types are not required. Do not add backward-compatibility aliases for removed public types in this package; document migrations or provide guidance for consumers instead.
| decodedReferrer: makeNormalizedAddressSchema(`${valueLabel} Decoded Referrer`), | ||
| }) | ||
| .check(invariant_registrarActionDecodedReferrerBasedOnRawReferrer), | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect referral invariant + normalized schema usage:"
nl -ba packages/ensnode-sdk/src/registrars/zod-schemas.ts | sed -n '88,136p'
echo
echo "Locate toNormalizedAddress implementation/usage to confirm casing semantics:"
rg -n --type=ts -C3 'export (const|function)\s+toNormalizedAddress|toNormalizedAddress\(' packages/enssdk packages/ensnode-sdk
echo
echo "Find lowercase assumptions near address comparisons:"
rg -n --type=ts -C2 'decodedReferrer|toLowerCase\(\).*Address|must be based on' packages/ensnode-sdk/src/registrars/zod-schemas.tsRepository: namehash/ensnode
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect registrar zod-schemas around lines 88-136 ==="
cat -n packages/ensnode-sdk/src/registrars/zod-schemas.ts | sed -n '88,136p'
echo
echo "=== Check toNormalizedAddress implementation ==="
rg -n --type=ts 'export (const|function)\s+toNormalizedAddress' packages/enssdk packages/ensnode-sdk
echo
echo "=== Check toNormalizedAddress usage pattern ==="
rg -n --type=ts 'toNormalizedAddress\(' packages/enssdk packages/ensnode-sdk -A2 -B2Repository: namehash/ensnode
Length of output: 7076
🏁 Script executed:
#!/bin/bash
head -30 packages/ensnode-sdk/src/registrars/zod-schemas.ts | cat -nRepository: namehash/ensnode
Length of output: 1181
🏁 Script executed:
#!/bin/bash
echo "=== Check all imports in zod-schemas.ts ==="
head -30 packages/ensnode-sdk/src/registrars/zod-schemas.ts
echo
echo "=== Check if toNormalizedAddress is imported anywhere ==="
rg -n 'toNormalizedAddress|import.*from.*enssdk' packages/ensnode-sdk/src/registrars/zod-schemas.ts
echo
echo "=== Verify makeNormalizedAddressSchema implementation ==="
rg -n --type=ts -A5 'export.*makeNormalizedAddressSchema' packages/ensnode-sdk/src/shared/zod-schemas.tsRepository: namehash/ensnode
Length of output: 1382
🏁 Script executed:
#!/bin/bash
echo "=== Full makeNormalizedAddressSchema implementation ==="
rg -n --type=ts -A15 'export const makeNormalizedAddressSchema' packages/ensnode-sdk/src/shared/zod-schemas.tsRepository: namehash/ensnode
Length of output: 640
Use consistent address normalization in invariant.
decodedReferrer is normalized via makeNormalizedAddressSchema (which applies toNormalizedAddress), but the invariant at line 99 uses raw .toLowerCase(). While both currently produce lowercase output, using inconsistent normalization paths creates maintenance risk: if toNormalizedAddress semantics change, the invariant's direct .toLowerCase() call won't align. Import toNormalizedAddress from "enssdk" and use it in the invariant for consistency.
Suggested fix
import type { Address } from "enssdk";
+import { toNormalizedAddress } from "enssdk";
import { z } from "zod/v4";
@@
- // decodeEncodedReferrer returns checksummed address, but ENSNode work on lowercase address values
- // so we lowercase the result before using for checks
- const expectedDecodedReferrer = decodeEncodedReferrer(encodedReferrer).toLowerCase() as Address;
+ const expectedDecodedReferrer = toNormalizedAddress(
+ decodeEncodedReferrer(encodedReferrer) as Address,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts` around lines 130 - 133,
The invariant invariant_registrarActionDecodedReferrerBasedOnRawReferrer
currently compares decodedReferrer against rawReferrer using .toLowerCase();
update it to use the same normalization as decodedReferrer by importing
toNormalizedAddress from "enssdk" and replacing the .toLowerCase() usage with
toNormalizedAddress(rawReferrer) (so both sides use
makeNormalizedAddressSchema/toNormalizedAddress), and add the import for
toNormalizedAddress at the top of the file.
There was a problem hiding this comment.
This looks like a good catch by CodeRabbit about line 99 in this file.
There was a problem hiding this comment.
@lightwalker-eth, glad you agree! Here's the planned fix:
- Update the
enssdkimport inpackages/ensnode-sdk/src/registrars/zod-schemas.tsto includetoNormalizedAddress - Replace
.toLowerCase() as AddresswithtoNormalizedAddress(...)insideinvariant_registrarActionDecodedReferrerBasedOnRawReferrer
🧠 Learnings used
Learnt from: tk-o
Repo: namehash/ensnode PR: 1671
File: packages/ensnode-sdk/src/ensapi/api/indexing-status/zod-schemas.ts:64-68
Timestamp: 2026-03-02T17:40:32.679Z
Learning: In `packages/ensnode-sdk/src/ensapi/api/indexing-status/zod-schemas.ts`, the serialized Ok response schema (`makeSerializedEnsApiIndexingStatusResponseOkSchema`) intentionally uses `z.object` instead of `z.strictObject` to allow extra fields, while non-serialized schemas use `z.strictObject`. This design choice is deliberate for forward compatibility.
Learnt from: tk-o
Repo: namehash/ensnode PR: 1639
File: packages/ensnode-sdk/src/ensapi/api/indexing-status/zod-schemas.ts:21-76
Timestamp: 2026-02-16T17:53:41.849Z
Learning: In the ENSNode SDK, all schema builder functions exported from zod-schemas.ts files (e.g., makeEnsApiIndexingStatusResponseSchema) are internal API and may change without requiring deprecated aliases, even if exported via an internal entry point. During code reviews, treat these as internal and avoid relying on them as public API surface. Do not mandate public-facing deprecation notices or aliases for changes to these schemas; instead, plan changes with internal consumers in mind and communicate breaking changes accordingly.
Learnt from: notrab
Repo: namehash/ensnode PR: 1673
File: apps/ensapi/src/handlers/registrar-actions-api.ts:102-115
Timestamp: 2026-02-25T07:04:53.636Z
Learning: The registrar-actions-api endpoint (apps/ensapi/src/handlers/registrar-actions-api.ts) uses `serializeRegistrarActionsResponse` with `RegistrarActionsResponseError` and includes a `responseCode` field. This differs from the standard ENSApi `errorResponse` helper pattern to maintain backward compatibility and avoid breaking the established API contract.
Learnt from: Goader
Repo: namehash/ensnode PR: 1732
File: packages/ens-referrals/src/v1/api/serialized-types.ts:43-47
Timestamp: 2026-03-06T23:35:37.231Z
Learning: In `packages/ens-referrals`, the team is not concerned with preserving backward-compatibility aliases for removed public TypeScript types (e.g., SerializedReferrerEditionMetricsRanked, SerializedReferrerEditionMetricsUnranked) during active development. Breaking changes to exported types in this package are acceptable and do not require deprecated aliases.
Learnt from: tk-o
Repo: namehash/ensnode PR: 1615
File: packages/ensnode-sdk/src/api/indexing-status/deserialize.ts:38-40
Timestamp: 2026-02-07T12:22:32.900Z
Learning: In `packages/ensnode-sdk/src/api/indexing-status/deserialize.ts`, the pattern of using `z.preprocess()` with `buildUnvalidatedIndexingStatusResponse` (which returns `unknown`) is intentional. This enforces a "parse serialized → preprocess to unvalidated → validate final schema" flow, where `z.preprocess` is semantically correct because it runs before final validation. Using `.transform()` would be incorrect here as it runs after parsing and receives a typed input.
Learnt from: Goader
Repo: namehash/ensnode PR: 1663
File: packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts:74-96
Timestamp: 2026-02-24T15:53:06.633Z
Learning: In TypeScript code reviews, prefer placing invariants on type aliases only when the invariant is context-independent or reused across multiple fields. If an invariant depends on surrounding rules or object semantics (e.g., field-specific metrics), keep the invariant as a field JSDoc instead. This guideline applies to TS files broadly (e.g., the repo's v1/award-models and similar modules).
Learnt from: Y3drk
Repo: namehash/ensnode PR: 1849
File: packages/ens-referrals/src/v1/edition-defaults.test.ts:118-133
Timestamp: 2026-03-31T16:10:52.187Z
Learning: In `packages/ens-referrals/src/v1/edition-defaults.test.ts`, the `disqualifications` field of `ReferralProgramRulesRevShareLimit` is intentionally NOT asserted in the production-parity tests because disqualifications can change very dynamically and including them would cause brittle, frequently-failing tests. A code comment should note this omission is deliberate.
Learnt from: tk-o
Repo: namehash/ensnode PR: 1798
File: packages/ensdb-sdk/src/client/ensdb-reader.ts:97-103
Timestamp: 2026-03-19T18:42:52.611Z
Learning: In `packages/ensdb-sdk/src/client/ensdb-reader.ts`, the `EnsDbReader` read methods (e.g., `getEnsDbVersion`, `getEnsIndexerPublicConfig`, `getIndexingStatusSnapshot`) intentionally do NOT re-validate data fetched from ENSDb using Zod or similar. Data in ENSDb is considered valid by design: all write operations go through `EnsDbWriter`, which enforces validity before persisting. Therefore, flagging missing read-side Zod validation in `EnsDbReader` is incorrect.
Learnt from: Y3drk
Repo: namehash/ensnode PR: 1861
File: docs/ensnode.io/src/components/organisms/ENSNodeSuite.tsx:94-101
Timestamp: 2026-04-02T07:15:53.785Z
Learning: In `docs/ensnode.io/src/components/organisms/ENSNodeSuite.tsx` (namehash/ensnode, TypeScript/React), documentation links in the `ENSNodeSuite` component intentionally use `target="_blank"` and `rel="noopener noreferrer"` even for internal relative paths (e.g., `/ensadmin`, `/ensindexer`, `/ensrainbow`). Do not flag this as an issue in future reviews.
Learnt from: Goader
Repo: namehash/ensnode PR: 1663
File: packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts:74-96
Timestamp: 2026-02-24T15:53:20.841Z
Learning: In packages/ens-referrals/src/v1/award-models/rev-share-limit/metrics.ts, AwardedReferrerMetricsRevShareLimit.standardAwardValue should not duplicate the generic PriceUsdc non-negative invariant; keep only the context-specific formula (“rules.qualifiedRevenueShare × totalBaseRevenueContribution”) and notes about pool independence.
Learnt from: Goader
Repo: namehash/ensnode PR: 1907
File: apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts:104-106
Timestamp: 2026-04-09T16:37:43.273Z
Learning: In `apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts`, the `if (c.var.indexingStatus instanceof Error) { throw new Error("Indexing status has not been loaded yet"); }` guards inside `getRegistrarActionsRoute` and `getRegistrarActionsByParentNodeRoute` handlers are intentionally unreachable. `registrarActionsApiMiddleware` (in `apps/ensapi/src/middleware/registrar-actions.middleware.ts`) already checks `c.var.indexingStatus instanceof Error` and short-circuits with a serialized 503 `serializeRegistrarActionsResponse` response before calling `next()`. The throw-based guards in the handlers exist solely for TypeScript narrowing so the compiler knows `indexingStatus` is not an Error after the check.
Failed to handle agent chat message. Please try again.
| export const ENS_ROOT_NAME = asInterpretedName(""); | ||
|
|
||
| /** | ||
| * The {@link Node} that identifies the ENS Root Name (""). | ||
| */ | ||
| export const ENS_ROOT_NODE: Node = namehashInterpretedName(asInterpretedName("")); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use ENS_ROOT_NAME as the single source for ENS_ROOT_NODE.
ENS_ROOT_NODE re-creates the empty interpreted name inline; reusing ENS_ROOT_NAME avoids duplication drift.
Suggested refactor
-export const ENS_ROOT_NODE: Node = namehashInterpretedName(asInterpretedName(""));
+export const ENS_ROOT_NODE: Node = namehashInterpretedName(ENS_ROOT_NAME);As per coding guidelines: "Do not duplicate definitions across multiple locations; use type aliases to document invariants, with each invariant documented exactly once on its type alias."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/enssdk/src/lib/constants.ts` around lines 8 - 13, ENS_ROOT_NODE
currently recreates the empty interpreted name inline; change ENS_ROOT_NODE to
use the existing ENS_ROOT_NAME constant as its single source of truth by passing
ENS_ROOT_NAME into namehashInterpretedName (keep the Node type), i.e., replace
the asInterpretedName("") usage in the ENS_ROOT_NODE initializer with
ENS_ROOT_NAME so the empty name is defined only once and cannot drift.
| export function nameToInterpretedName(name: Name): InterpretedName { | ||
| return interpretedLabelsToInterpretedName( | ||
| name | ||
| .split(".") | ||
| .map((label) => { | ||
| if (isEncodedLabelHash(label)) return label; | ||
| if (isNormalizedLabel(label)) return label; | ||
| return encodeLabelHash(labelhashLiteralLabel(asLiteralLabel(label))); | ||
| }) | ||
| .map(asInterpretedLabel), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle ENS root ("") before mapping labels.
For Line 31–37, name === "" becomes [""] via .split("."), then gets encoded into a non-root label. That changes root-name semantics and can produce incorrect output for valid root input.
💡 Proposed fix
export function nameToInterpretedName(name: Name): InterpretedName {
+ if (name === "") return name as InterpretedName;
+
return interpretedLabelsToInterpretedName(
name
.split(".")
.map((label) => {
if (isEncodedLabelHash(label)) return label;
if (isNormalizedLabel(label)) return label;
return encodeLabelHash(labelhashLiteralLabel(asLiteralLabel(label)));
})
.map(asInterpretedLabel),
);
}📝 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 function nameToInterpretedName(name: Name): InterpretedName { | |
| return interpretedLabelsToInterpretedName( | |
| name | |
| .split(".") | |
| .map((label) => { | |
| if (isEncodedLabelHash(label)) return label; | |
| if (isNormalizedLabel(label)) return label; | |
| return encodeLabelHash(labelhashLiteralLabel(asLiteralLabel(label))); | |
| }) | |
| .map(asInterpretedLabel), | |
| ); | |
| } | |
| export function nameToInterpretedName(name: Name): InterpretedName { | |
| if (name === "") return name as InterpretedName; | |
| return interpretedLabelsToInterpretedName( | |
| name | |
| .split(".") | |
| .map((label) => { | |
| if (isEncodedLabelHash(label)) return label; | |
| if (isNormalizedLabel(label)) return label; | |
| return encodeLabelHash(labelhashLiteralLabel(asLiteralLabel(label))); | |
| }) | |
| .map(asInterpretedLabel), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 29 -
40, nameToInterpretedName incorrectly transforms the ENS root ("") because
"".split(".") yields [""] which gets encoded as a non-root label; fix by
handling the root early: in nameToInterpretedName, if name === "" return
interpretedLabelsToInterpretedName([]) (i.e., an empty labels array) before
calling .split and the label mapping logic so you don't encode a root label;
keep the rest of the mapping using asInterpretedLabel, isEncodedLabelHash,
isNormalizedLabel, encodeLabelHash, and labelhashLiteralLabel as-is for non-root
names.
| /** | ||
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. | ||
| */ | ||
| export function asLiteralLabel(label: Label): LiteralLabel { | ||
| return label as LiteralLabel; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align asLiteralLabel docs with implementation.
Line 219 says this function “validates,” but it now only casts. Please either reintroduce runtime validation or adjust the wording to avoid implying checks that no longer happen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 218 -
224, The JSDoc for asLiteralLabel incorrectly claims it "Validates and casts"
while the implementation only performs a type cast; update the documentation or
implementation: either reintroduce runtime validation inside the
asLiteralLabel(label: Label): LiteralLabel function (perform checks on
format/characters and throw a clear error on invalid input) or change the JSDoc
to say it "Casts" (e.g., "Casts a string to a LiteralLabel without runtime
validation") so the comment accurately reflects the behavior; reference the
asLiteralLabel function to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/enssdk/src/lib/interpreted-names-and-labels.ts (2)
219-224:⚠️ Potential issue | 🟡 MinorMake the
asLiteralLabeldocs match the implementation.Line 220 still says this helper “validates and casts,” but Lines 223-224 now do a plain cast only. Either restore validation or update the JSDoc to avoid implying runtime checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 219 - 224, The JSDoc for asLiteralLabel incorrectly claims it "validates and casts" while the implementation only does a TypeScript cast; either implement runtime validation inside asLiteralLabel (e.g., check Label shape and throw or return a safe result) or update the JSDoc to remove "validates" and state it simply performs a TypeScript cast/typing assertion. Refer to the function name asLiteralLabel and ensure the chosen change is consistent across the file (update tests/docs if needed).
30-40:⚠️ Potential issue | 🟠 MajorHandle the ENS root before splitting labels.
Line 33 still turns
""into[""], sonameToInterpretedName("")encodes an empty label instead of preserving the root name.💡 Suggested fix
export function nameToInterpretedName(name: Name): InterpretedName { + if (name === "") return interpretedLabelsToInterpretedName([]); + return interpretedLabelsToInterpretedName( name .split(".")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 30 - 40, The function nameToInterpretedName currently splits the ENS root ("") into [""] and then encodes that empty label; instead, detect the root case up-front and return the interpreted-root representation (i.e., call interpretedLabelsToInterpretedName with an empty array) when name === "" (or canonical root form), otherwise proceed with the existing split-and-map logic; update nameToInterpretedName to short-circuit on the empty root before performing split and referencing helpers like isEncodedLabelHash, isNormalizedLabel, encodeLabelHash, labelhashLiteralLabel, asLiteralLabel, and asInterpretedLabel.
🤖 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/ensindexer/src/config/validations.ts`:
- Line 130: Fix the grammar in the error message string in validations.ts:
change "not a enssdk#NormalizedAddress" to "not an enssdk#NormalizedAddress"
where the message is composed using config.namespace, datasourceName,
contractName and contractConfig.address so the corrected article applies to the
identifier enssdk#NormalizedAddress in that error text.
In `@examples/enskit-react-example/src/DomainView.tsx`:
- Line 58: The UI currently returns a message exposing internal type name
"InterpretedName" (in the conditional that checks data?.domain), so update the
user-facing text in DomainView (the early return that uses data?.domain and
name) to a friendly phrase like "A domain named '{name}' was not found." or "No
domain found with the name '{name}'.", replacing "InterpretedName" with plain
language.
In `@packages/ens-referrals/src/address.ts`:
- Line 6: Update the thrown error message in
packages/ens-referrals/src/address.ts to use the new NormalizedAddress
terminology: replace the existing message "Invalid address: '${address}'.
Address must be a lowercase EVM Address." with a message that references
"NormalizedAddress" (e.g., "Invalid address: '${address}'. Address must be a
NormalizedAddress.") so diagnostics match the refactor; locate the throw new
Error(...) in the address validation logic where the variable address is used
and change only the message text.
In `@packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts`:
- Around line 39-41: Create a shared alias (e.g., NonEmptyInterpretedName) that
documents the “non-empty InterpretedName” invariant and replace the inline
typing on both columns with that alias; specifically add a type alias named
NonEmptyInterpretedName documenting the non-empty contract and then change
occurrences like value: t.text().notNull().$type<InterpretedName>() (and the
other occurrence around lines 131-133) to use $type<NonEmptyInterpretedName>()
so the invariant is declared once and reused.
In `@packages/enssdk/src/lib/address.test.ts`:
- Around line 47-53: Tests currently assert a broad /does not/ message which may
match unrelated errors; update the two specs ("should throw for a partial
address" and "should throw for a non-hex string") to assert the actual error
shape/message thrown by toNormalizedAddress instead of the vague regex — either
use toThrowError(/exact expected message/) with the precise message the function
produces (e.g., /invalid address/i) or assert the concrete error type with
toThrow(TypeError) if it throws a TypeError; change the expectations around
calls to toNormalizedAddress(...) accordingly.
---
Duplicate comments:
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts`:
- Around line 219-224: The JSDoc for asLiteralLabel incorrectly claims it
"validates and casts" while the implementation only does a TypeScript cast;
either implement runtime validation inside asLiteralLabel (e.g., check Label
shape and throw or return a safe result) or update the JSDoc to remove
"validates" and state it simply performs a TypeScript cast/typing assertion.
Refer to the function name asLiteralLabel and ensure the chosen change is
consistent across the file (update tests/docs if needed).
- Around line 30-40: The function nameToInterpretedName currently splits the ENS
root ("") into [""] and then encodes that empty label; instead, detect the root
case up-front and return the interpreted-root representation (i.e., call
interpretedLabelsToInterpretedName with an empty array) when name === "" (or
canonical root form), otherwise proceed with the existing split-and-map logic;
update nameToInterpretedName to short-circuit on the empty root before
performing split and referencing helpers like isEncodedLabelHash,
isNormalizedLabel, encodeLabelHash, labelhashLiteralLabel, asLiteralLabel, and
asInterpretedLabel.
🪄 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: 9f31fc64-0b5d-4403-bdb0-b4d60d9244eb
📒 Files selected for processing (11)
apps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensindexer/src/config/validations.tsexamples/enskit-react-example/src/DomainView.tsxpackages/ens-referrals/src/address.tspackages/ens-referrals/src/referrer-metrics.tspackages/ens-referrals/src/v1/address.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/rules.tspackages/ens-referrals/src/v1/referrer-metrics.tspackages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.tspackages/enssdk/src/lib/address.test.tspackages/enssdk/src/lib/interpreted-names-and-labels.ts
| if (!isNormalizedAddress(contractConfig.address)) { | ||
| throw new Error( | ||
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a lowercase viem#Address). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, | ||
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, |
There was a problem hiding this comment.
Fix article typo in the error message.
Line 130 says “not a enssdk#NormalizedAddress”; this should be “not an enssdk#NormalizedAddress”.
✏️ Proposed text fix
- `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`,
+ `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not an enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`,📝 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.
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, | |
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not an enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/config/validations.ts` at line 130, Fix the grammar in
the error message string in validations.ts: change "not a
enssdk#NormalizedAddress" to "not an enssdk#NormalizedAddress" where the message
is composed using config.namespace, datasourceName, contractName and
contractConfig.address so the corrected article applies to the identifier
enssdk#NormalizedAddress in that error text.
| if (fetching) return <p>Loading...</p>; | ||
| if (error) return <p>Error: {error.message}</p>; | ||
| if (!data?.domain) return <p>Not found</p>; | ||
| if (!data?.domain) return <p>A Domain with InterpretedName '{name}' was not found.</p>; |
There was a problem hiding this comment.
Avoid exposing internal type names in user-facing copy.
Line 58 shows InterpretedName in UI text, which leaks implementation terminology to end users.
💬 Suggested copy update
- if (!data?.domain) return <p>A Domain with InterpretedName '{name}' was not found.</p>;
+ if (!data?.domain) return <p>Domain '{name}' was not found.</p>;📝 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.
| if (!data?.domain) return <p>A Domain with InterpretedName '{name}' was not found.</p>; | |
| if (!data?.domain) return <p>Domain '{name}' was not found.</p>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/enskit-react-example/src/DomainView.tsx` at line 58, The UI
currently returns a message exposing internal type name "InterpretedName" (in
the conditional that checks data?.domain), so update the user-facing text in
DomainView (the early return that uses data?.domain and name) to a friendly
phrase like "A domain named '{name}' was not found." or "No domain found with
the name '{name}'.", replacing "InterpretedName" with plain language.
| throw new Error(`Invalid address: ${address}. Address must be a valid EVM address.`); | ||
| export const validateAddress = (address: Address): void => { | ||
| if (!isNormalizedAddress(address)) { | ||
| throw new Error(`Invalid address: '${address}'. Address must be a lowercase EVM Address.`); |
There was a problem hiding this comment.
Use normalized terminology in the thrown error.
Line 6 still says “lowercase EVM Address”, which conflicts with the new NormalizedAddress terminology used elsewhere in this refactor. Aligning the message will keep diagnostics consistent.
Suggested patch
- throw new Error(`Invalid address: '${address}'. Address must be a lowercase EVM Address.`);
+ throw new Error(`Invalid address: '${address}'. Address must be a NormalizedAddress.`);📝 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.
| throw new Error(`Invalid address: '${address}'. Address must be a lowercase EVM Address.`); | |
| throw new Error(`Invalid address: '${address}'. Address must be a NormalizedAddress.`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ens-referrals/src/address.ts` at line 6, Update the thrown error
message in packages/ens-referrals/src/address.ts to use the new
NormalizedAddress terminology: replace the existing message "Invalid address:
'${address}'. Address must be a lowercase EVM Address." with a message that
references "NormalizedAddress" (e.g., "Invalid address: '${address}'. Address
must be a NormalizedAddress.") so diagnostics match the refactor; locate the
throw new Error(...) in the address validation logic where the variable address
is used and change only the message text.
| * The value of this field is guaranteed to be a non-empty {@link InterpretedName}. | ||
| */ | ||
| value: t.text().notNull(), | ||
| value: t.text().notNull().$type<InterpretedName>(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the non-empty-name invariant onto a shared alias.
These two fields now repeat the same “non-empty InterpretedName” rule in JSDoc, but both columns are still typed as plain InterpretedName. Please lift that contract into a single alias (for example, NonEmptyInterpretedName) and use $type<...>() here instead of documenting the same invariant twice.
As per coding guidelines, "Do not duplicate definitions across multiple locations; use type aliases to document invariants, with each invariant documented exactly once on its type alias."
Also applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts`
around lines 39 - 41, Create a shared alias (e.g., NonEmptyInterpretedName) that
documents the “non-empty InterpretedName” invariant and replace the inline
typing on both columns with that alias; specifically add a type alias named
NonEmptyInterpretedName documenting the non-empty contract and then change
occurrences like value: t.text().notNull().$type<InterpretedName>() (and the
other occurrence around lines 131-133) to use $type<NonEmptyInterpretedName>()
so the invariant is declared once and reused.
| it("should throw for a partial address", () => { | ||
| expect(() => toNormalizedAddress("0xdeaf")).toThrow(/does not/); | ||
| }); | ||
|
|
||
| it("should throw for a non-hex string", () => { | ||
| expect(() => toNormalizedAddress("not-an-address" as Address)).toThrow(/does not/); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten throw assertions to avoid false positives.
Line 48 and Line 52 use /does not/, which is broad and can pass on unrelated errors. Prefer asserting a more specific message or error type to keep failure signals precise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/enssdk/src/lib/address.test.ts` around lines 47 - 53, Tests
currently assert a broad /does not/ message which may match unrelated errors;
update the two specs ("should throw for a partial address" and "should throw for
a non-hex string") to assert the actual error shape/message thrown by
toNormalizedAddress instead of the vague regex — either use toThrowError(/exact
expected message/) with the precise message the function produces (e.g.,
/invalid address/i) or assert the concrete error type with toThrow(TypeError) if
it throws a TypeError; change the expectations around calls to
toNormalizedAddress(...) accordingly.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| if (!isNormalizedAddress(contractConfig.address)) { | ||
| throw new Error( | ||
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a lowercase viem#Address). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, | ||
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, |
There was a problem hiding this comment.
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not a enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, | |
| `The '${config.namespace}' namespace's '${datasourceName}' Datasource does not define a valid address for ${contractName}: '${contractConfig.address}'. This occurs if the address property of any ContractConfig in the Datasource is malformed (i.e. not an enssdk#NormalizedAddress). This is only likely to occur if you are actively editing the Datasource and typo'd an address.`, |
Error message uses incorrect article "a" instead of "an" before "enssdk#NormalizedAddress"
| * @dev this is similar to the concept of Reinterpretation (packages/enssdk/src/lib/reinterpretation.ts) | ||
| * but is distinct in that we're interpreting a Name (i.e. from user input) rather than a previously | ||
| * interpreted InterpretedName. | ||
| */ |
There was a problem hiding this comment.
nameToInterpretedName("") incorrectly encodes empty string as label hash instead of returning ENS root name
| // must be lowercase adddress | ||
| // must be lowercase | ||
| expect( | ||
| // TODO: could use enssdk#isNormalizedAddress here |
There was a problem hiding this comment.
Error message in test assertion contains grammatical error: "is not is lowercase format" instead of "is not in lowercase format"
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. |
There was a problem hiding this comment.
| * Validates and casts a string to a {@link LiteralLabel}. | |
| * A LiteralLabel is a label as it literally appears onchain. | |
| * Casts a string to a {@link LiteralLabel}. | |
| * A LiteralLabel is a label as it literally appears onchain. | |
| * No validation is performed - any string is accepted including empty strings. |
The asLiteralLabel function's docstring claims it "validates and casts" but the implementation only performs a type cast without any validation
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Awesome updates here 😄 Reviewed and shared some suggestions 👍 Appreciate your advice!
| // UniversalResolver returns the offset in bytes within the DNS Encoded Name where the activeName begins | ||
| const activeName: Name = bytesToPacket(dnsEncodedNameBytes.slice(offset)); | ||
| // Invariant: the decoded name must be an InterpretedName | ||
| const activeName = asInterpretedName(bytesToPacket(dnsEncodedNameBytes.slice(offset))); |
There was a problem hiding this comment.
Shouldn't we say that bytesToPacket returns a LiteralName and that we therefore need to transform from the LiteralName to an InterpretedName?
| options: Omit<Parameters<typeof _resolveForward>[2], "registry">, | ||
| ): Promise<ForwardResolutionResult<SELECTION>> { | ||
| // Invariant: Name must be an InterpretedName | ||
| const interpretedName = asInterpretedName(name); |
There was a problem hiding this comment.
We have a related issue open: #1032
What do you suggest we do in relation to issue 1032 within the scope of this PR (PR 1908)? Should we work to confirm that issue 1032 can be successfully closed, or maybe keep issue 1032 open and make any new relevant comments on it that might help us with working on it later?
| options: Omit<Parameters<typeof _resolveForward>[2], "registry">, | ||
| ): Promise<ForwardResolutionResult<SELECTION>> { | ||
| // Invariant: Name must be an InterpretedName | ||
| const interpretedName = asInterpretedName(name); |
There was a problem hiding this comment.
I believe the name param (currently typed as ForwardResolutionArgs<SELECTION>["name"]) should be updated so that it is already a validated InterpretedName and that the validation that it is a valid interpreted name might be handled by Zod when we're parsing the query params?
We could then also remove the need for creating interpretedName here as a distinct field from the name param.
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. |
There was a problem hiding this comment.
| * Validates and casts a string to a {@link LiteralLabel}. | |
| * A LiteralLabel is a label as it literally appears onchain. | |
| * Casts a string to a {@link LiteralLabel}. | |
| * A LiteralLabel is a label that should be interpreted as a string literal. It may or may not be normalized or normalizable. It may also be formatted as an encoded labelhash, but that does not mean it should be interpreted as an encoded labelhash. |
Goals include expanding the definition here for cases that aren't strictly onchain. Ex: A literal label can come from any source of input, such as a database, somewhere in DNS, etc.
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. | ||
| */ | ||
| export function asLiteralLabel(label: Label): LiteralLabel { |
There was a problem hiding this comment.
Maybe we completely remove the Label and Name type aliases? I worry they will be a source of problems and that only our more precise type names should be used.
The only case I can think of right now that's not covered by our more formal type names would be what happens when we beautify an InterpretedName. This doesn't return an InterpretedName. Instead it returns what we might give a new type alias for: a BeautifiedName, which is the same as an InterpretedName except it loosens the constraints such that a label might be normalizable but not necessarily normalized.
In other words:
- An
InterpretedNameis all normalized labels or encoded labelhashes. - A
BeautifiedNameis all normalizable labels or encoded labelhashes.
When we display names in a UI we should render them as BeautifiedName values.
Please also note that I believe raffy's beautify name function won't properly handle labels that are encoded labelhashes and would throw on those. So we would need to implement our own wrapper around raffy's beautify name function to correctly convert any InterpretedName into a BeautifiedName.
Appreciate your advice 👍
|
|
||
| /** | ||
| * Validates and casts a string to an {@link InterpretedName}. | ||
| * An InterpretedName is composed entirely of InterpretedLabels joined by dots. |
There was a problem hiding this comment.
Suggest we also make it explicit how an empty name is also an InterpretedName.
|
|
||
| /** | ||
| * Validates and casts a string to an {@link InterpretedLabel}. | ||
| * An InterpretedLabel is either a normalized label or an EncodedLabelHash. |
There was a problem hiding this comment.
One worry I have is about how we handle the parsing of EncodedLabelHash values. Do we need to modify anything here in relation to case sensitivity?
| */ | ||
| export function asLiteralLabel(label: string): LiteralLabel { | ||
| if (label === "") throw new Error("LiteralLabel must not be empty"); | ||
| export function asInterpretedName(name: Name): InterpretedName { |
There was a problem hiding this comment.
This feedback is in relation to your work on nameToInterpretedName which is really awesome.
A worry I have is that devs using this package will be lazy with how they use asInterpretedName. Specifically worried that they will use it when parsing names from user input instead of using nameToInterpretedName.
Perhaps we should rename asInterpretedName to something that makes it more explicit it is just for "system inputs" and not user inputs? Appreciate your advice.
closes #1876
NormalizedNamein favor ofInterpretedName, refactors Resolution libs to operate overInterpretedNamesNormalizedAddresstype (not branded) to replace theasLowerCaseAddressandmakeLowerCaseAddressSchemaNormalizedAddress0x{string}but it should help