tests: add seed script to improve resolution tests#1994
Open
tests: add seed script to improve resolution tests#1994
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request consolidates devnet account management by migrating account definitions from the SDK to a shared datasources module, establishes devnet seeding infrastructure to populate primary names and resolver records, updates chain configuration from custom ID to chainId 1, and refactors integration tests and SDK internals accordingly. Changes
Sequence DiagramsequenceDiagram
participant Orchestrator as Orchestrator
participant Seeder as Seeding Module
participant VClient as Viem Wallet Clients
participant DevNet as Devnet Contracts
participant Registry as ethReverseRegistrar
participant Resolver as Resolver
Orchestrator->>Seeder: seedDevnet(rpcUrl)
Seeder->>VClient: Build wallet clients (deployer, owner, user, user2)
VClient-->>Seeder: Configured clients with accounts
Seeder->>Seeder: seedPrimaryNameRecords(clients)
Seeder->>Registry: clients.owner → setName("test.eth")
Registry-->>Seeder: tx hash
Seeder->>DevNet: waitForTransactionReceipt()
Seeder->>Seeder: seedResolverRecords(clients)
Seeder->>DevNet: findResolver("test.eth")
DevNet-->>Seeder: resolver address
Seeder->>Resolver: setText (avatar, twitter, github, etc.)
Seeder->>Resolver: setAddr (BTC, LTC coin types)
Seeder->>Resolver: setContenthash()
Seeder->>Resolver: setPubkey()
Seeder->>Resolver: setABI()
Seeder->>Resolver: setInterface()
Resolver-->>Seeder: tx receipts
Seeder-->>Orchestrator: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 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 |
sevenzing
commented
Apr 24, 2026
Contributor
Greptile SummaryThis PR adds a seeding mechanism for the devnet integration test environment, bootstrapping primary name records and resolver records for
Confidence Score: 4/5Safe to merge with awareness that the primary-name seeding calls a contract not watched by the ENSIndexer, which will cause primary-name integration tests to fail The resolver-records seeding is correct and comprehensive. The orchestrator integration is sound (chain-ID check runs before seeding, all receipts are awaited). However, packages/integration-test-env/src/seed/primary-names.ts — the contract address passed to Important Files Changed
Sequence DiagramsequenceDiagram
participant O as Orchestrator
participant DC as Docker Compose
participant S as Seed Script
participant D as Devnet (Anvil)
participant I as ENSIndexer
participant A as ENSApi
participant T as Integration Tests
O->>DC: Start ENSDb + Devnet containers
DC-->>O: Devnet ready (chain ID verified)
O->>S: seedDevnet(rpcUrl)
S->>D: setPrimaryNameRecord("test.eth") via ETHReverseRegistrar
D-->>S: tx receipt
S->>D: seedResolverRecords for test.eth (texts, addrs, contenthash, pubkey, ABI, interfaces)
D-->>S: tx receipts (all awaited)
S-->>O: Devnet seeded
O->>I: Start ENSIndexer (pnpm start)
I->>D: Index on-chain events from block 0
I-->>O: omnichain-following / omnichain-completed
O->>A: Start ENSApi (pnpm start)
A-->>O: /health 200
O->>T: pnpm test:integration
T->>A: GET /api/resolve/primary-name/:address/1
T->>A: GET /api/resolve/records/test.eth
A-->>T: Assertions pass
Reviews (10): Last reviewed commit: "fix cicd" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sevenzing
commented
Apr 24, 2026
| @@ -0,0 +1,97 @@ | |||
| /** | |||
Member
Author
There was a problem hiding this comment.
decided to keep all addresses from pnpm devnet logs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ensapi/src/handlers/api/resolution/resolve-primary-names.integration.test.ts`:
- Around line 16-22: Update the test description string in the
resolve-primary-names integration test to reflect that a primary name is now
seeded for DEVNET_ACCOUNTS.owner on chain 1; locate the test case that currently
has description "resolves primary names for owner address on chain 1 (no primary
name set in devnet)" and change it to mention that test.eth is the seeded
primary (e.g., "resolves primary name test.eth for owner address on chain 1") so
the description matches expectedBody.names["1"] === "test.eth".
In `@packages/ensnode-sdk/src/shared/devnet/accounts.ts`:
- Line 26: The entry named one in DEVNET_ACCOUNTS is an undocumented hardcoded
address (toNormalizedAddress(`0x${"1".repeat(40)}`)) and is inconsistent with
sibling entries derived from DEVNET_MNEMONIC; either add a comment explaining
its purpose and why it’s included alongside mnemonic-derived signers (e.g.,
"Placeholder address used for X — not a signer") or remove/move it out of
DEVNET_ACCOUNTS into a clearer constant (e.g., DEVNET_BYTES or
DEVNET_PLACEHOLDER_ADDRESSES) and update any consumers to reference the new
constant so DEVNET_ACCOUNTS only contains mnemonic-derived signers. Ensure
references to DEVNET_MNEMONIC and toNormalizedAddress remain correct if you keep
it.
In `@packages/ensnode-sdk/src/shared/devnet/data.ts`:
- Around line 3-11: The DEVNET_BYTES object is currently annotated as
Record<string, Hex>, which widens and loses its literal keys; replace the
explicit type annotation with a satisfies clause so keys are preserved while
values remain constrained to Hex. Concretely, remove ": Record<string, Hex>" and
export the object as a constant using the satisfies operator, e.g. declare
DEVNET_BYTES with "export const DEVNET_BYTES = { ... } as const satisfies
Record<string, Hex>;" (or at minimum "export const DEVNET_BYTES = { ... }
satisfies Record<string, Hex>;") so IDE autocomplete and key-level type
narrowing for DEVNET_BYTES is retained.
In `@packages/integration-test-env/src/seed/index.ts`:
- Around line 19-24: Update the stale inline comments on the DevnetWalletClients
type so they no longer reference legacy constants
DEVNET_OWNER/DEVNET_USER/DEVNET_USER2; instead, describe each wallet's
role/purpose consistent with DEVNET_ACCOUNTS and the comments in
packages/ensnode-sdk/src/shared/devnet/accounts.ts (e.g., deployer — index 0 has
REGISTRAR role on ETHRegistry; owner — index 1 owns test.eth; user — index 2
general test user; user2 — index 3 secondary test user). Locate these comments
next to the DevnetWalletClients type and replace the old constant names with the
role/purpose descriptions.
In `@packages/integration-test-env/src/seed/primary-names.ts`:
- Around line 10-18: The setPrimaryNameRecord function (and all set*Record
functions in resolver-records.ts such as setTextRecord, setMulticoinAddress,
setContenthash, setPubkey, setAbi, setInterfaceImplementer) currently returns
immediately after walletClient.writeContract (which only submits to mempool);
modify each seeding function to await transaction finality by calling
publicClient.waitForTransactionReceipt({ hash }) after writeContract returns its
hash and before returning (use the returned hash from walletClient.writeContract
and ensure the wait call is awaited so seedPrimaryNameRecords and seedDevnet
only proceed after the tx is mined).
In `@packages/integration-test-env/src/seed/resolver-records.ts`:
- Around line 144-152: Remove the commented-out dead block for
clearResolverRecords (the async function referencing PUBLIC_RESOLVER) since it
uses an undefined symbol and clutters the file; either delete the entire
commented section or restore it as a working helper by updating the symbol to
the file's RESOLVER constant, import/define any missing types/abi
(DevnetWalletClient, Hex, publicResolverAbi) and adjust the writeContract call
to use RESOLVER and valid args so it compiles and works for local debugging.
- Around line 12-40: seedResolverRecords currently fires many writeContract
calls (via helpers like setTextRecord, setMulticoinAddress, setContenthash,
setPubkey, setAbi, setInterfaceImplementer) which return RPC tx hashes but do
not wait for mining; collect the returned tx hashes (or receipts) from those
helper calls and then use the public client's waitForTransactionReceipt (e.g.,
clients.owner.publicClient.waitForTransactionReceipt) to await the last tx (or
await each hash) before returning from seedResolverRecords so indexing cannot
start before transactions are mined and reverted txs surface.
- Line 14: The call to assertTestEthResolver is reaching into viem internals via
clients.owner.transport.url; instead, add an explicit rpcUrl parameter to
seedResolverRecords and assertTestEthResolver, replace uses of
clients.owner.transport.url with the rpcUrl argument inside seedResolverRecords
and assertTestEthResolver, and update the caller (from seedDevnet/seed/index.ts)
to pass the existing rpcUrl value through to seedResolverRecords so no transport
internals are accessed.
🪄 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: 96a22959-433e-410a-95be-35ae2754d0b0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/ensapi/src/handlers/api/resolution/resolve-primary-name.integration.test.tsapps/ensapi/src/handlers/api/resolution/resolve-primary-names.integration.test.tsapps/ensapi/src/handlers/api/resolution/resolve-records.integration.test.tsapps/ensapi/src/omnigraph-api/schema/account.integration.test.tspackages/datasources/src/lib/chains.tspackages/ensnode-sdk/src/internal.tspackages/ensnode-sdk/src/omnigraph-api/example-queries.tspackages/ensnode-sdk/src/shared/devnet-accounts.tspackages/ensnode-sdk/src/shared/devnet/accounts.tspackages/ensnode-sdk/src/shared/devnet/addresses.tspackages/ensnode-sdk/src/shared/devnet/data.tspackages/integration-test-env/package.jsonpackages/integration-test-env/src/orchestrator.tspackages/integration-test-env/src/seed/abi.tspackages/integration-test-env/src/seed/index.tspackages/integration-test-env/src/seed/primary-names.tspackages/integration-test-env/src/seed/resolver-records.ts
💤 Files with no reviewable changes (1)
- packages/ensnode-sdk/src/shared/devnet-accounts.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
shrugs
reviewed
Apr 24, 2026
Member
shrugs
left a comment
shrugs
left a comment
There was a problem hiding this comment.
LOVE the direction, way better than forking; i'd forgotten that we just know the test mnemonic and can hook in like that. definitely the correct direction, thanks for prototyping this
Member
|
i didn't see the devnet commit version be updated in this pr, make sure that's done in the docker-compose so that the orchestrator is definitely using the correct version and the chain id update is locked in. all devnet-specific stuff must be pinned to a specific commit or we're going to have a really annoying time when things change behind the scenes (as they do a LOT) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integration-test-env/src/orchestrator.ts (1)
7-13:⚠️ Potential issue | 🟡 MinorFile-level phase docstring is out of sync with code.
The header still lists 5 phases, but the implementation now has 7 phases (Phase 2 = seed devnet, Phase 5 = poll indexing status, Phase 6 = ENSApi, Phase 7 = tests). New readers will miss that seeding is part of the orchestration.
📝 Proposed update
* Phases: * 1. Postgres + devnet via docker-compose (testcontainers DockerComposeEnvironment) - * 2. Download pre-built ENSRainbow LevelDB, extract, start ENSRainbow from source - * 3. Start ENSIndexer, wait for omnichain-following / omnichain-completed - * 4. Start ENSApi - * 5. Run `pnpm test:integration` at the monorepo root + * 2. Seed devnet with primary names and resolver records + * 3. Download pre-built ENSRainbow LevelDB, extract, start ENSRainbow from source + * 4. Start ENSIndexer + * 5. Wait for omnichain-following / omnichain-completed + * 6. Start ENSApi + * 7. Run `pnpm test:integration` at the monorepo root🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integration-test-env/src/orchestrator.ts` around lines 7 - 13, Update the top-of-file phase docstring in orchestrator.ts to match the current implementation: replace the old 5-phase list with the accurate 7-phase sequence and explicitly include the new "seed devnet" and "poll indexing status" steps (which correspond to the code paths that perform devnet seeding and polling for omnichain-following/omnichain-completed before starting ENSApi and running tests); reference the existing phase logic implemented in the orchestration functions (the devnet seed routine, the indexing poll routine, the ENSApi start routine, and the final test-run invocation) and reorder the docstring so each implemented phase is described in the same order the code executes.
♻️ Duplicate comments (1)
packages/ensnode-sdk/src/shared/devnet/addresses.ts (1)
13-100: 🧹 Nitpick | 🔵 Trivial
DEVNET_CONTRACTSvalues are not normalized (lowercase) addresses.These are checksummed
Addressliterals, notNormalizedAddress. Consumers likeseedResolverRecordsForNamealready work around this withactualResolver.toLowerCase() !== resolver.toLowerCase()(resolver-records.ts line 29), but any future strict equality / set membership comparison against anotherNormalizedAddresswill silently mismatch. Recommend normalizing once at definition (and ideally typing viatoNormalizedAddressso the type alias enforces the invariant).♻️ Proposed change
+import { type NormalizedAddress, toNormalizedAddress } from "enssdk"; + +const n = toNormalizedAddress; + -export const DEVNET_CONTRACTS = { +export const DEVNET_CONTRACTS = { // -- DNS -- - dnssecGatewayProvider: "0x5FbDB2315678afecb367f032d93F642f64180aa3", + dnssecGatewayProvider: n("0x5FbDB2315678afecb367f032d93F642f64180aa3"), // …apply to each entry… -} as const; +} as const satisfies Record<string, NormalizedAddress>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/shared/devnet/addresses.ts` around lines 13 - 100, DEVNET_CONTRACTS currently contains checksummed Address literals which can mismatch NormalizedAddress comparisons (see seedResolverRecordsForName’s toLowerCase workaround); update the DEVNET_CONTRACTS object so every contract address value is normalized (lowercase) at definition and typed as a NormalizedAddress (use the existing toNormalizedAddress helper or equivalent) so the invariant is enforced (refer to DEVNET_CONTRACTS, seedResolverRecordsForName, and toNormalizedAddress for locating spots to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/datasources/src/lib/chains.ts`:
- Around line 3-7: Update the JSDoc comment in
packages/datasources/src/lib/chains.ts to remove the trailing colon and clearly
state that the ens-test-env intentionally uses chain id 1 to mirror mainnet (so
callers should not treat id==1 as a sentinel for real mainnet), and mention the
potential for mainnet-collision to discourage future sentinel-style checks
against the value; update the comment text above the file/module (where the
current /** The ens-test-env chain id is 1: */ block appears) accordingly.
- Around line 8-13: The current gate that disables Ponder's cache uses a numeric
comparison against ensTestEnvChain.id (e.g., `chainId === 31337 || chainId ===
1337 || chainId === ensTestEnvChain.id`) which collides with mainnet id; change
that logic to use the namespace instead — replace the `... || chainId ===
ensTestEnvChain.id` check with a namespace check such as `config.namespace ===
ENSNamespaceIds.EnsTestEnv` (use the existing ENSNamespaceIds enum/const and the
`config.namespace` value) so local-dev detection relies on `config.namespace`
rather than the numeric `ensTestEnvChain.id`, leaving other chainId checks
intact.
In `@packages/ensnode-sdk/src/shared/devnet/addresses.ts`:
- Around line 107-114: The JSDoc describing "Named signer accounts from the
ens-test-env devnet…" is incorrectly attached to the helper
deriveNormalizedAccount; move that doc block to sit directly above the exported
DEVNET_ACCOUNTS constant so the comment documents the export it describes, and
either remove or replace the JSDoc on deriveNormalizedAccount (or add a brief
helper-specific comment) so deriveNormalizedAccount, DEVNET_ACCOUNTS and
DEVNET_MNEMONIC are correctly and clearly documented.
---
Outside diff comments:
In `@packages/integration-test-env/src/orchestrator.ts`:
- Around line 7-13: Update the top-of-file phase docstring in orchestrator.ts to
match the current implementation: replace the old 5-phase list with the accurate
7-phase sequence and explicitly include the new "seed devnet" and "poll indexing
status" steps (which correspond to the code paths that perform devnet seeding
and polling for omnichain-following/omnichain-completed before starting ENSApi
and running tests); reference the existing phase logic implemented in the
orchestration functions (the devnet seed routine, the indexing poll routine, the
ENSApi start routine, and the final test-run invocation) and reorder the
docstring so each implemented phase is described in the same order the code
executes.
---
Duplicate comments:
In `@packages/ensnode-sdk/src/shared/devnet/addresses.ts`:
- Around line 13-100: DEVNET_CONTRACTS currently contains checksummed Address
literals which can mismatch NormalizedAddress comparisons (see
seedResolverRecordsForName’s toLowerCase workaround); update the
DEVNET_CONTRACTS object so every contract address value is normalized
(lowercase) at definition and typed as a NormalizedAddress (use the existing
toNormalizedAddress helper or equivalent) so the invariant is enforced (refer to
DEVNET_CONTRACTS, seedResolverRecordsForName, and toNormalizedAddress for
locating spots to change).
🪄 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: ba17a835-903e-45c1-8fbf-d54b284898b5
📒 Files selected for processing (13)
apps/ensapi/src/handlers/api/resolution/resolve-primary-name.integration.test.tsapps/ensapi/src/handlers/api/resolution/resolve-primary-names.integration.test.tsapps/ensapi/src/handlers/api/resolution/resolve-records.integration.test.tsapps/ensapi/src/omnigraph-api/schema/account.integration.test.tsdocker/services/devnet.ymlpackages/datasources/src/lib/chains.tspackages/ensnode-sdk/src/internal.tspackages/ensnode-sdk/src/omnigraph-api/example-queries.tspackages/ensnode-sdk/src/shared/devnet/addresses.tspackages/ensnode-sdk/src/shared/devnet/data.tspackages/integration-test-env/src/orchestrator.tspackages/integration-test-env/src/seed/index.tspackages/integration-test-env/src/seed/resolver-records.ts
shrugs
reviewed
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add seeding mechanism for devnet to improve our testing
Why
We cannot test ensapi right now
Pre-Review Checklist (Blocking)