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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SDK updates exact expiration and grace-period boundary semantics with timestamp and null-case tests. ENSv2 registry handling identifies reverse-name registrations and skips the existing expiration invariant for them. Patch changesets document both updates. ChangesRegistration Expiration Helpers
ENSv2 Reverse-Name Registrations
Estimated code review effort: 3 (Moderate) | ~20 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.
3 Skipped Deployments
|
|
@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 indexing invariant logic bugs: the boundary condition in
Confidence Score: 5/5Safe to merge — the two boundary fixes are consistent with each other and correctly align the expiration helpers with onchain semantics, and the reverse-name bypass is well-scoped to a clearly identified protocol special case. The expiration predicate changes are a single-character operator fix with new tests that explicitly cover the previously-broken boundary values. The reverse-name bypass in No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[LabelRegistered event received] --> B{isReservation?}
B -- yes --> C{existing registration?}
C -- yes --> D{isRegistrationFullyExpired?\nnow > expiry + grace}
D -- no --> E[❌ throw invariant error]
D -- yes --> F[proceed to upsert domain]
C -- no --> F
B -- no --> G{existing registration?}
G -- no --> F
G -- yes --> H{isReverseNameRegistration?\nisNormalizedAddress AND\nregistrantId === 0x+label}
H -- yes --> F
H -- no --> I{registration.type ===\nENSv2RegistryReservation?}
I -- yes --> F
I -- no --> J{isRegistrationFullyExpired?\nnow > expiry + grace}
J -- yes --> F
J -- no --> E
F --> K[insert domain upsert]
K --> L[insertLatestRegistration\nnew record, incremented index]
%%{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 event received] --> B{isReservation?}
B -- yes --> C{existing registration?}
C -- yes --> D{isRegistrationFullyExpired?\nnow > expiry + grace}
D -- no --> E[❌ throw invariant error]
D -- yes --> F[proceed to upsert domain]
C -- no --> F
B -- no --> G{existing registration?}
G -- no --> F
G -- yes --> H{isReverseNameRegistration?\nisNormalizedAddress AND\nregistrantId === 0x+label}
H -- yes --> F
H -- no --> I{registration.type ===\nENSv2RegistryReservation?}
I -- yes --> F
I -- no --> J{isRegistrationFullyExpired?\nnow > expiry + grace}
J -- yes --> F
J -- no --> E
F --> K[insert domain upsert]
K --> L[insertLatestRegistration\nnew record, incremented index]
Reviews (3): 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.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
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 👍
| @@ -23,8 +23,8 @@ export function isRegistrationFullyExpired(info: RegistrationExpiryInfo, now: bi | |||
| // no expiry, never expired | |||
There was a problem hiding this comment.
@tk-o Should we rename this idea of being "full expired" as being "released"?
| return now > info.expiry + (info.gracePeriod ?? 0n); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Why are all the timestamps in this file typed as bigint? Don't we have a more meaningful type alias for this?
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)