Fix indexing invariants logic#2350
Conversation
Align and helpers with the indexed smart contracts' logic.
🦋 Changeset detectedLatest commit: c3a4db6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe SDK updates expiration helper types and grace-period boundaries, with tests for timestamp and null cases. API and indexer consumers now use derived expiry information and numeric timestamps. ENSv2 registry handling exempts validated reverse-name registrations from the existing expiration invariant. ChangesRegistration Expiration and Reverse-Name Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
This PR aligns ENS registration expiry/grace-period helpers (used across SDK, indexer, and API layers) with onchain boundary semantics, and relaxes an ENSIndexer ENSv2 invariant to handle repeated LabelRegistered events for reverse-name reservations without halting indexing.
Changes:
- Updated
isRegistrationFullyExpiredandisRegistrationInGracePeriodto treatexpiry + gracePeriodas an inclusive grace endpoint (renewal still valid at the last second). - Added unit tests covering expiry/grace boundary conditions for the registration expiration helpers.
- Updated ENSv2 registry handler invariant logic to tolerate existing reverse-name registrations that should not be considered expirable.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ensnode-sdk/src/registrars/registration-expiration.ts | Adjusts expiry/grace boundary comparisons to match onchain semantics. |
| packages/ensnode-sdk/src/registrars/registration-expiration.test.ts | Adds coverage for boundary conditions and null-handling behavior. |
| apps/ensindexer/src/plugins/unigraph/handlers/ensv2/ENSv2Registry.ts | Updates invariant handling to account for repeated reverse-name LabelRegistered events. |
| .changeset/modern-groups-occur.md | Publishes SDK helper behavior alignment as a patch changeset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR fixes two off-by-one boundary conditions in the ENS registration expiration helpers and adds a special-case handler for reverse-name registrations that can be re-registered without expiring.
Confidence Score: 5/5Safe to merge — the changes are well-scoped boundary fixes backed by new unit tests and confirmed against onchain behaviour. Both boundary corrections are minimal and consistent with each other: the last second of the grace period is now correctly non-expired in both helpers. The ENSv2Registry handler correctly adds a null guard and a narrowly scoped reverse-name bypass that is gated on two independent conditions (label is a valid normalized address AND the previous registrant equals that address). The new test suite exercises every relevant boundary value, including null inputs and exact-boundary timestamps. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[LabelRegistered / LabelReserved event] --> B{isReservation?}
B -- Yes --> C{existing registration?}
C -- No --> G[Insert new Registration]
C -- Yes --> D{isRegistrationFullyExpired?\nnow > expiry + grace}
D -- Yes\nexpired --> G
D -- No\nnot expired --> ERR1[Invariant Error:\nUnexpired registration found]
B -- No --> E{existing registration?}
E -- No --> G
E -- Yes --> F{isReverseNameRegistration?\nlabel==address AND registrantId==label}
F -- Yes\nreverse name --> G
F -- No --> H{type != Reservation\nAND not fully expired?}
H -- Yes --> ERR2[Invariant Error:\nUnexpired registration found]
H -- No --> G
G --> DONE[Upsert Domain / Insert Registration]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[LabelRegistered / LabelReserved event] --> B{isReservation?}
B -- Yes --> C{existing registration?}
C -- No --> G[Insert new Registration]
C -- Yes --> D{isRegistrationFullyExpired?\nnow > expiry + grace}
D -- Yes\nexpired --> G
D -- No\nnot expired --> ERR1[Invariant Error:\nUnexpired registration found]
B -- No --> E{existing registration?}
E -- No --> G
E -- Yes --> F{isReverseNameRegistration?\nlabel==address AND registrantId==label}
F -- Yes\nreverse name --> G
F -- No --> H{type != Reservation\nAND not fully expired?}
H -- Yes --> ERR2[Invariant Error:\nUnexpired registration found]
H -- No --> G
G --> DONE[Upsert Domain / Insert Registration]
Reviews (6): Last reviewed commit: "Apply AI PR feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Update invariant logic to meet the relevant smart contract implementation which allows reverse name registration overrides.
…tionInGracePeriod` helpers with onchain logic.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Shared some suggestions. Please take the lead to merge when ready 👍
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensindexer/src/plugins/unigraph/handlers/ensv2/ENSv2Registry.ts (1)
105-111: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing null guard for
registrationin the reservation branch causes false invariant failure.When
getLatestRegistrationreturnsnull(no existing registration) andisReservationis true,buildRegistrationExpiryInfo(null)produces{ expiry: null, gracePeriod: null }.isRegistrationFullyExpiredthen returnsfalse(no expiry = never expired), so!falseevaluates totrueand the error is thrown — even though there is no existing registration. This contradicts the error message itself ("expected none or expired") and would rejectLabelReservedevents for any previously-unregistered label, halting ENSv2 indexing.Compare with
BaseRegistrar.tsline 131 which correctly usesif (registration && !isFullyExpired).🐛 Proposed fix: add `registration &&` guard
if (isReservation) { // Invariant: if this is a Reservation, any existing Registration should be fully expired - if (!isRegistrationFullyExpired(registrationExpiryInfo, blockTimestamp)) { + if (registration && !isRegistrationFullyExpired(registrationExpiryInfo, blockTimestamp)) { throw new Error( `Invariant(ENSv2Registry:Label[Registered|Reserved]): Existing unexpired Registration found, expected none or expired.\n${toJson(registration, { pretty: true })}`, ); }🤖 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/ensindexer/src/plugins/unigraph/handlers/ensv2/ENSv2Registry.ts` around lines 105 - 111, Update the reservation branch in ENSv2Registry to only evaluate the expiry invariant when registration exists: guard the isRegistrationFullyExpired(registrationExpiryInfo, blockTimestamp) check with registration, while preserving the existing error for present but unexpired registrations.
🤖 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.
Outside diff comments:
In `@apps/ensindexer/src/plugins/unigraph/handlers/ensv2/ENSv2Registry.ts`:
- Around line 105-111: Update the reservation branch in ENSv2Registry to only
evaluate the expiry invariant when registration exists: guard the
isRegistrationFullyExpired(registrationExpiryInfo, blockTimestamp) check with
registration, while preserving the existing error for present but unexpired
registrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45832028-9cc5-4be6-9600-4cd75c441bc2
📒 Files selected for processing (8)
apps/ensapi/src/omnigraph-api/context.tsapps/ensapi/src/omnigraph-api/schema/registration.tsapps/ensindexer/src/plugins/unigraph/handlers/ensv1/BaseRegistrar.tsapps/ensindexer/src/plugins/unigraph/handlers/ensv1/NameWrapper.tsapps/ensindexer/src/plugins/unigraph/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/unigraph/handlers/ensv2/ETHRegistrar.tspackages/ensnode-sdk/src/registrars/registration-expiration.test.tspackages/ensnode-sdk/src/registrars/registration-expiration.ts
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
isRegistrationFullyExpiredandisRegistrationInGracePeriodhelpers to accurately represent the logic implemented on chain: the very last second of grace period is still a valid moment to renew a registration.handleRegistrationOrReservationevent handler to accurately represent the logic implemented onchain: theLabelRegisteredevent can be emitted multiple times for an existing registration of a reverse name. Such registrations never expire.Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)