feat(ensapi): improve seedDevnet - add names with contenthash#2286
feat(ensapi): improve seedDevnet - add names with contenthash#2286sevenzing wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces complete devnet seeding infrastructure for ENSv2 registered names and resolver records. It defines contract ABIs for registrar operations, adds test fixtures for contenthash values and name structures, implements registration and seeding primitives, and extends integration tests to validate domain resolution queries against seeded data. ChangesENSv2 Devnet Seeding Infrastructure and Integration Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const [log] = parseEventLogs({ | ||
| abi: VerifiableFactoryABI, | ||
| eventName: "ProxyDeployed", | ||
| logs: receipt.logs, |
There was a problem hiding this comment.
Missing null check on destructured event log causes crash when ProxyDeployed event is not found in transaction receipt
| }); | ||
| }); | ||
|
|
||
| it.each(registeredNames.flatMap((entry) => [...entry.subnames]))( |
There was a problem hiding this comment.
| it.each(registeredNames.flatMap((entry) => [...entry.subnames]))( | |
| it.each(registeredNames.flatMap((entry) => [...(entry.subnames ?? [])]))( |
Spreading undefined value when entry.subnames is optional causes TypeError
| // Derived from `registeredNames` fixture — expands automatically when new names are added there. | ||
| const seededDevnetNames = registeredNames.flatMap((entry) => [ | ||
| { name: entry.name, canonical: entry.name }, | ||
| ...entry.subnames.map((sub) => ({ name: sub.name, canonical: sub.name })), |
There was a problem hiding this comment.
| ...entry.subnames.map((sub) => ({ name: sub.name, canonical: sub.name })), | |
| ...(entry.subnames ?? []).map((sub) => ({ name: sub.name, canonical: sub.name })), |
Missing null check causes crash when entry.subnames is undefined - calling .map() on undefined throws TypeError
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ensapi/src/test/integration/devnet-names.ts`:
- Around line 30-33: The code maps over registeredNames and uses entry.subnames
without handling the optional field, which can crash if subnames is undefined;
update the mapping expressions (e.g., where seededDevnetNames is created using
registeredNames.flatMap and any other similar mapping in the domain integration
test) to use a fallback like (entry.subnames ?? []) before calling .map, i.e.
replace ...entry.subnames.map(...) with ...(entry.subnames ?? []).map(...),
ensuring RegisteredName.subnames is safely handled.
In `@apps/ensapi/src/test/integration/find-domains/test-domain-pagination.ts`:
- Around line 97-112: The test uses String.prototype.localeCompare with an
implicit locale; update both calls in the ascending/descending checks in
test-domain-pagination.ts to pass an explicit locale (e.g., "en-US") as the
first argument while keeping the { ignorePunctuation: true } option so ordering
matches Postgres ICU collation consistently across environments; ensure you
change both occurrences inside the if (dir === "ASC") and else blocks where
av.localeCompare(bv, undefined, { ignorePunctuation: true }) is used.
In `@packages/integration-test-env/src/seed/registrar.ts`:
- Around line 174-196: The balance check, mint and approve must use the same
payer identity: determine a single payer (e.g., const payer =
client.account?.address ?? owner) and use that payer for the balanceOf and mint
calls (replace owner with payer) and ensure the approve is executed from the
same payer (either call writeContract with the client's signer set to payer or
use a client instance tied to payer) so balance/mint/approve are consistent for
registerEthName (references: owner, client.account.address, paymentToken,
erc20Abi, MockTokenABI, writeContract, readContract, waitForTransactionReceipt,
contracts.ETHRegistrar).
🪄 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: 202653ab-6581-4827-8161-ead196f4e94c
📒 Files selected for processing (13)
apps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/test/integration/devnet-names.tsapps/ensapi/src/test/integration/find-domains/test-domain-pagination.tspackages/datasources/src/abis/ensv2/ETHRegistrar.tspackages/datasources/src/abis/ensv2/MockToken.tspackages/datasources/src/abis/ensv2/UserRegistry.tspackages/datasources/src/abis/ensv2/VerifiableFactory.tspackages/datasources/src/index.tspackages/integration-test-env/src/devnet/fixtures.tspackages/integration-test-env/src/seed/index.tspackages/integration-test-env/src/seed/registered-names.tspackages/integration-test-env/src/seed/registrar.tspackages/integration-test-env/src/seed/resolver-records.ts
| const seededDevnetNames = registeredNames.flatMap((entry) => [ | ||
| { name: entry.name, canonical: entry.name }, | ||
| ...entry.subnames.map((sub) => ({ name: sub.name, canonical: sub.name })), | ||
| ]); |
There was a problem hiding this comment.
Missing optional chaining on entry.subnames in both files.
Both devnet-names.ts and domain.integration.test.ts access entry.subnames without checking for undefined. Since RegisteredName.subnames is optional, any future fixture entry that omits subnames will cause a runtime crash. Add ?? [] fallback in both locations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/test/integration/devnet-names.ts` around lines 30 - 33, The
code maps over registeredNames and uses entry.subnames without handling the
optional field, which can crash if subnames is undefined; update the mapping
expressions (e.g., where seededDevnetNames is created using
registeredNames.flatMap and any other similar mapping in the domain integration
test) to use a fallback like (entry.subnames ?? []) before calling .map, i.e.
replace ...entry.subnames.map(...) with ...(entry.subnames ?? []).map(...),
ensuring RegisteredName.subnames is safely handled.
| // Use localeCompare with ignorePunctuation to match Postgres ICU collation, which treats | ||
| // punctuation (including ".") as variable/ignorable at the primary level. Without this, | ||
| // JS bytewise ordering disagrees with the DB for names like "onion3.x" vs "onion.x": | ||
| // Postgres sorts "onion3" before "onion." (digits < letters after stripping dots), while | ||
| // JS places "." (code 46) before "3" (code 51). | ||
| if (dir === "ASC") { | ||
| expect(av <= bv, `expected "${av}" <= "${bv}" at indices ${i},${i + 1} (NAME ASC)`).toBe( | ||
| true, | ||
| ); | ||
| expect( | ||
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) <= 0, | ||
| `expected "${av}" <= "${bv}" at indices ${i},${i + 1} (NAME ASC)`, | ||
| ).toBe(true); | ||
| } else { | ||
| expect(av >= bv, `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`).toBe( | ||
| true, | ||
| ); | ||
| expect( | ||
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) >= 0, | ||
| `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`, | ||
| ).toBe(true); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider explicit locale for localeCompare to match Postgres configuration.
The code uses localeCompare(bv, undefined, { ignorePunctuation: true }) with an implicit locale (line 104, 109). While this works in the current devnet environment, the default locale may vary across systems. If the Postgres ICU collation uses a specific locale (e.g., en-US), explicitly passing that locale would ensure consistent ordering across environments.
♻️ Optional improvement
- av.localeCompare(bv, undefined, { ignorePunctuation: true }) <= 0,
+ av.localeCompare(bv, "en-US", { ignorePunctuation: true }) <= 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/test/integration/find-domains/test-domain-pagination.ts`
around lines 97 - 112, The test uses String.prototype.localeCompare with an
implicit locale; update both calls in the ascending/descending checks in
test-domain-pagination.ts to pass an explicit locale (e.g., "en-US") as the
first argument while keeping the { ignorePunctuation: true } option so ordering
matches Postgres ICU collation consistently across environments; ensure you
change both occurrences inside the if (dir === "ASC") and else blocks where
av.localeCompare(bv, undefined, { ignorePunctuation: true }) is used.
| const balance = await client.readContract({ | ||
| address: paymentToken, | ||
| abi: erc20Abi, | ||
| functionName: "balanceOf", | ||
| args: [owner], | ||
| }); | ||
|
|
||
| if (balance < price) { | ||
| const mintHash = await client.writeContract({ | ||
| address: paymentToken, | ||
| abi: MockTokenABI, | ||
| functionName: "mint", | ||
| args: [owner, price - balance], | ||
| }); | ||
| await waitForTransactionReceipt(client, mintHash); | ||
| } | ||
|
|
||
| const approveHash = await client.writeContract({ | ||
| address: paymentToken, | ||
| abi: erc20Abi, | ||
| functionName: "approve", | ||
| args: [contracts.ETHRegistrar, price], | ||
| }); |
There was a problem hiding this comment.
Align payer identity across balance/mint/approve in registerEthName.
Line 174 checks ERC20 balance for owner and Line 186 mints to owner, but Line 191 approves from the signer (client.account). If owner !== client.account.address, this creates inconsistent funding/allowance setup and can fail registration.
Suggested fix
export async function registerEthName(
client: DevnetWalletClient,
{
label,
owner,
@@
): Promise<void> {
+ const signer = client.account.address;
+ if (owner !== signer) {
+ throw new Error("registerEthName requires owner to match signer");
+ }
+
const duration = BigInt(durationDays * ONE_DAY_SECONDS);
@@
const balance = await client.readContract({
address: paymentToken,
abi: erc20Abi,
functionName: "balanceOf",
- args: [owner],
+ args: [signer],
});
@@
if (balance < price) {
const mintHash = await client.writeContract({
address: paymentToken,
abi: MockTokenABI,
functionName: "mint",
- args: [owner, price - balance],
+ args: [signer, price - balance],
});
await waitForTransactionReceipt(client, mintHash);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/integration-test-env/src/seed/registrar.ts` around lines 174 - 196,
The balance check, mint and approve must use the same payer identity: determine
a single payer (e.g., const payer = client.account?.address ?? owner) and use
that payer for the balanceOf and mint calls (replace owner with payer) and
ensure the approve is executed from the same payer (either call writeContract
with the client's signer set to payer or use a client instance tied to payer) so
balance/mint/approve are consistent for registerEthName (references: owner,
client.account.address, paymentToken, erc20Abi, MockTokenABI, writeContract,
readContract, waitForTransactionReceipt, contracts.ETHRegistrar).
| ): Promise<void> { | ||
| const duration = BigInt(durationDays * ONE_DAY_SECONDS); | ||
| const paymentToken = contracts.MockUSDC; | ||
| const referrer = "0x0000000000000000000000000000000000000000000000000000000000000000" as Hex; |
| * | ||
| * @see https://github.com/ensdomains/content-hash/blob/master/src/index.test.ts | ||
| */ | ||
| export const contenthashFixtures = { |
There was a problem hiding this comment.
looks like no need to export, and perhaps could even hardcode these values into the registeredNames const?
| * To add more names: append entries here. Seeding, DEVNET_NAMES, and tests pick them up | ||
| * automatically — no other files need changing. | ||
| */ | ||
| export const registeredNames = [ |
There was a problem hiding this comment.
i would name this REGISTERED_NAMES const case
There was a problem hiding this comment.
perhaps even ADDITIONALLY_REGISTERED_NAMES
| export const DEVNET_NAMES = [ | ||
| import { registeredNames } from "@ensnode/integration-test-env/devnet"; | ||
|
|
||
| const staticDevnetNames = [ |
There was a problem hiding this comment.
this too, const-case STATIC_DEVNET_NAMES
| ] as const; | ||
|
|
||
| // Derived from `registeredNames` fixture — expands automatically when new names are added there. | ||
| const seededDevnetNames = registeredNames.flatMap((entry) => [ |
|
Greptile SummaryThis PR extends the devnet seeding infrastructure to register a set of ENS names with real contenthash values (ipfs, swarm, ipns, onion, onion3, skynet, arweave) via the ENSv2 ETHRegistrar commit-reveal flow, and adds integration tests that verify the seeded records are returned correctly by the GraphQL API.
Confidence Score: 4/5Safe to merge; all changes are devnet seeding and test infrastructure with no production code paths affected. The registration flow, ABI definitions, and test assertions all look correct. The only findings are a pair of inconsistent null-guard patterns (direct entry.subnames access vs. ?? []) and a system-locale-dependent localeCompare call that is safe for ASCII names but could theoretically vary across environments. apps/ensapi/src/test/integration/devnet-names.ts and domain.integration.test.ts for the subnames null-guard inconsistency; apps/ensapi/src/test/integration/find-domains/test-domain-pagination.ts for the undefined locale. Important Files Changed
Sequence DiagramsequenceDiagram
participant Seed as seedRegisteredNames
participant VF as VerifiableFactory
participant ETHReg as ETHRegistrar
participant UR as UserRegistry
participant ETHRegistry as ETHRegistry
participant Resolver as PermissionedResolver
loop for each entry in registeredNames
Seed->>VF: deployProxy(UserRegistryImpl, salt, initData)
VF-->>Seed: ProxyDeployed → userRegistry address
Seed->>ETHReg: makeCommitment(label, owner, secret, subregistry, resolver, duration, referrer)
ETHReg-->>Seed: commitment hash
Seed->>ETHReg: commit(commitment)
Seed->>Seed: advanceTime(minAge + 1s)
Seed->>ETHReg: rentPrice(label, owner, duration, token)
ETHReg-->>Seed: base + premium
Seed->>MockUSDC: mint(owner, shortfall) if needed
Seed->>MockUSDC: approve(ETHRegistrar, price)
Seed->>ETHReg: register(label, owner, secret, subregistry, resolver, duration, token, referrer)
Seed->>UR: setParent(ETHRegistry, label)
opt entry.records.contenthash
Seed->>Resolver: setContenthash(node, contenthash)
end
loop for each subname
Seed->>UR: register(label, owner, 0x0, resolver, ROLES_ALL, MAX_EXPIRY)
opt sub.records.contenthash
Seed->>Resolver: setContenthash(subnode, contenthash)
end
end
end
Reviews (1): Last reviewed commit: "add registration of names with contentha..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const seededDevnetNames = registeredNames.flatMap((entry) => [ | ||
| { name: entry.name, canonical: entry.name }, | ||
| ...entry.subnames.map((sub) => ({ name: sub.name, canonical: sub.name })), | ||
| ]); |
There was a problem hiding this comment.
registered-names.ts seeding code uses entry.subnames ?? [] as a defensive fallback, but here entry.subnames.map(...) is accessed directly. Both are TypeScript-safe today because as const infers subnames as always-present, but the inconsistency means that if a future fixture entry omits subnames, this file will fail to compile while registered-names.ts would silently skip subnames via ?? []. Aligning on (entry.subnames ?? []).map(...) keeps the behaviour consistent across both call-sites.
| const seededDevnetNames = registeredNames.flatMap((entry) => [ | |
| { name: entry.name, canonical: entry.name }, | |
| ...entry.subnames.map((sub) => ({ name: sub.name, canonical: sub.name })), | |
| ]); | |
| const seededDevnetNames = registeredNames.flatMap((entry) => [ | |
| { name: entry.name, canonical: entry.name }, | |
| ...(entry.subnames ?? []).map((sub) => ({ name: sub.name, canonical: sub.name })), | |
| ]); |
| }); | ||
| }); | ||
|
|
||
| it.each(registeredNames.flatMap((entry) => [...entry.subnames]))( |
There was a problem hiding this comment.
devnet-names.ts: [...entry.subnames] accesses subnames without a null guard, unlike registered-names.ts which uses ?? []. Spreading (entry.subnames ?? []) makes the intent explicit and matches the defensive pattern used in the seeding code.
| it.each(registeredNames.flatMap((entry) => [...entry.subnames]))( | |
| it.each(registeredNames.flatMap((entry) => [...(entry.subnames ?? [])]))( |
| expect( | ||
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) <= 0, | ||
| `expected "${av}" <= "${bv}" at indices ${i},${i + 1} (NAME ASC)`, | ||
| ).toBe(true); | ||
| } else { | ||
| expect(av >= bv, `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`).toBe( | ||
| true, | ||
| ); | ||
| expect( | ||
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) >= 0, | ||
| `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`, | ||
| ).toBe(true); |
There was a problem hiding this comment.
undefined as the locale falls back to the runtime's system locale, which can differ between developer machines and CI environments. For pure ASCII ENS names the difference is almost certainly benign, but pinning an explicit neutral locale like "en-US" makes the comparison deterministic regardless of the environment's LANG setting.
| expect( | |
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) <= 0, | |
| `expected "${av}" <= "${bv}" at indices ${i},${i + 1} (NAME ASC)`, | |
| ).toBe(true); | |
| } else { | |
| expect(av >= bv, `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`).toBe( | |
| true, | |
| ); | |
| expect( | |
| av.localeCompare(bv, undefined, { ignorePunctuation: true }) >= 0, | |
| `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`, | |
| ).toBe(true); | |
| expect( | |
| av.localeCompare(bv, "en-US", { ignorePunctuation: true }) <= 0, | |
| `expected "${av}" <= "${bv}" at indices ${i},${i + 1} (NAME ASC)`, | |
| ).toBe(true); | |
| } else { | |
| expect( | |
| av.localeCompare(bv, "en-US", { ignorePunctuation: true }) >= 0, | |
| `expected "${av}" >= "${bv}" at indices ${i},${i + 1} (NAME DESC)`, | |
| ).toBe(true); |
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!
| if (entry.records) { | ||
| await seedNameRecords(client, resolver, namehash(entry.name) as Hex, entry.records); | ||
| } | ||
|
|
||
| // 5. Register subnames and seed their records. |
There was a problem hiding this comment.
// 5. — step 4 ("Seed resolver records on the 2LD") has no matching inline comment. Adding it keeps the numbering consistent with the doc.
| if (entry.records) { | |
| await seedNameRecords(client, resolver, namehash(entry.name) as Hex, entry.records); | |
| } | |
| // 5. Register subnames and seed their records. | |
| // 4. Seed resolver records on the 2LD. | |
| if (entry.records) { | |
| await seedNameRecords(client, resolver, namehash(entry.name) as Hex, entry.records); | |
| } | |
| // 5. Register subnames and seed their records. |
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!
2 participants
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)