Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 5228c69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOrders deterministic sorting: registrar actions now sort by Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make the ENSApi “registrar actions” list ordering deterministic when multiple actions share the same block timestamp, improving pagination stability and repeatability of results.
Changes:
- Update “latest registrar actions” ordering to sort by
registrar_actions.id(Ponder checkpoint) instead oftimestamp. - Add a Changesets entry to release the fix as an
ensapipatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts | Changes the ORDER BY used for “latest” registrar actions to use id for deterministic ordering. |
| .changeset/tall-icons-return.md | Declares a patch release for ensapi describing the ordering fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR fixes non-deterministic ordering of Registrar Actions by switching the SQL Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as SDK Client
participant ENSApi as ENSApi Server
participant DB as Database (registrarActions)
participant Leaderboard as Leaderboard Builder
note over Client,DB: Registrar Actions API – deterministic ordering fix
Client->>ENSApi: GET /api/registrar-actions?orderBy=orderBy[id]=desc
ENSApi->>ENSApi: buildOrderByClause(LatestRegistrarActions)<br/>→ desc(registrarActions.id) [was: desc(timestamp)]
ENSApi->>DB: SELECT … ORDER BY id DESC LIMIT n OFFSET m
note over DB: id is TEXT primary key (Ponder checkpoint)<br/>Lexicographic DESC = Chronological DESC<br/>because id is constant-length (75-char) decimal string
DB-->>ENSApi: Rows ordered newest-first, fully deterministic
ENSApi-->>Client: RegistrarActionsResponse
note over Leaderboard,DB: Referral Leaderboard – sort simplification
Leaderboard->>DB: SELECT id, referrer, … ORDER BY id ASC
DB-->>Leaderboard: ReferralEvent[] (pre-sorted by id)
Leaderboard->>Leaderboard: events.sort((a,b) => a.id < b.id ? -1 : …)<br/>[was: compareEventIds with BigInt + zeroed eventType]
note over Leaderboard: Lexicographic string sort ≡ numeric sort<br/>for constant-length decimal strings
Leaderboard->>Leaderboard: Run sequential race algorithm
Leaderboard-->>Client: ReferrerLeaderboardRevShareLimit
Last reviewed commit: 8cb3820 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts
Show resolved
Hide resolved
packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.test.ts:79
- In these tests,
makeEvent()accepts atimestampbut the generated checkpointiddoes not incorporate it (the prefix hard-codesblockTimestampto all zeros). Since production ordering relies onidencoding the block timestamp, consider either encoding thetimestampinto the checkpoint prefix for realism or clarifying/removing the separatetimestampparam to avoid misleading “chronological order” assumptions in test cases.
function makeEvent(
referrer: `0x${string}`,
timestamp: number,
incrementalDuration: number,
opts: Partial<Pick<ReferralEvent, "id">> = {},
): ReferralEvent {
const counter = ++eventIdCounter;
return {
id: opts.id ?? `${CHECKPOINT_PREFIX}${String(counter).padStart(16, "0")}`,
referrer,
timestamp,
incrementalDuration,
incrementalRevenueContribution: ZERO_ETH,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
lightwalker-eth
approved these changes
Mar 15, 2026
Member
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Goader Hey looks good. Thanks. Shared a few suggestions. Please feel welcome to merge when ready 👍
| * A deterministic and globally unique fixed-length identifier for the "logical registrar action" | ||
| * associated with the ReferralEvent. | ||
| * | ||
| * A Ponder-encoded checkpoint string that uniquely and deterministically identifies |
Member
There was a problem hiding this comment.
Suggest we not document the implementation details of this id string value at this layer of responsibility.
Suggested action items:
- We should move all the strong documentation about the implementation details of this
idfield into theensnode-schemapackage. Specifically here: https://github.com/namehash/ensnode/blob/main/packages/ensnode-schema/src/schemas/registrars.schema.ts#L170 - ... therefore, we can expose the implementation details of this
idfield to those building directly on ENSDb. - The documentation about this
idfield within theens-referralspackage should treat theidfield as an opaque identifier without any mention of its implementation details. Within theens-referralspackage we should just identify the following invariants about thisidfield:- It is deterministic and globally unique.
- Sorting by this value achieves a chronological ordering of each registrar action by the order they were executed onchain.
- ... all the other implementation details, including any reference to Ponder or details of the "sub-fields" encoded within this string, etc.. should move into
ensnode-schema.
| * Records Orders | ||
| */ | ||
| export const RegistrarActionsOrders = { | ||
| LatestRegistrarActions: "orderBy[timestamp]=desc", |
Member
There was a problem hiding this comment.
Suggest we revert the change here and keep the sort at an API layer as sorting by timestamp, not by id.
Yes, it's true that we would actually implement timestamp sorting by sorting by id, but that seems like an internal implementation detail of the API.
At the level of an external API consumer, if I say I want to sort by timestamp then I interpret that as wanting to sort in chronological order. The implementation of the API then translates that intention into a sort by id.
For background: Each block has a single timestamp and contain contain hundreds / thousands of events. Each of those events therefore share the same timestamp, but each event was executed in a specific order within the block.
Sorting by id is the same as sorting by timestamp with the added benefit that it correctly sorts events that share the same timestamp into the correct chronological order of when each event was executed within the same block.
| /** | ||
| * Returns registrar actions newest-first. | ||
| * | ||
| * Sorts by the `id` field descending. Each `id` is a Ponder checkpoint string |
Member
There was a problem hiding this comment.
This is a great comment, but suggest a few action items:
- Transfer all the nice ideas in this comment into a new "sort events" utility function in the
ens-referralspackage. - Please see the feedback above suggesting to revert the removal of
"orderBy[timestamp]=desc". Suggest to add a comment there about how sorting by timestamp not only sorts by timestamp, but further sorts by the chronological execution order of each registrar action.
| // Ponder log (smart-contract event) handlers and therefore share the same | ||
| // eventType digit. If mixed event types are ever introduced, ordering by id | ||
| // directly would need revisiting. | ||
| const sortedEvents = [...events].sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); |
Member
There was a problem hiding this comment.
Thanks for this 👍 Suggest moving this logic and it's associated comments into a dedicated utility function.
| "ensapi": patch | ||
| --- | ||
|
|
||
| Fix inconsistent ordering of registrar actions by sorting on the constant-length Ponder checkpoint `id` field (lexicographic = chronological order) instead of timestamp alone. |
Member
There was a problem hiding this comment.
Suggested change
| Fix inconsistent ordering of registrar actions by sorting on the constant-length Ponder checkpoint `id` field (lexicographic = chronological order) instead of timestamp alone. | |
| Provide deterministic sorting of registrar actions by their execution order. |
Goal: Avoid reference to Ponder implementation details in our release notes unless there's a special reason to mention them.
| "@namehash/ens-referrals": patch | ||
| --- | ||
|
|
||
| Simplify rev-share-limit leaderboard race sort to use direct lexicographic comparison of the constant-length Ponder checkpoint `id`, removing the now-unnecessary `compareEventIds` and `resetEncodedEventType` helpers. |
Member
There was a problem hiding this comment.
Suggested change
| Simplify rev-share-limit leaderboard race sort to use direct lexicographic comparison of the constant-length Ponder checkpoint `id`, removing the now-unnecessary `compareEventIds` and `resetEncodedEventType` helpers. | |
| Simplify how rev-share-limit leaderboard race sorting achieves deterministic sorting by execution order. |
Goal: Avoid reference to Ponder implementation details in our release notes unless there's a special reason to mention them.
| "@ensnode/ensnode-sdk": minor | ||
| --- | ||
|
|
||
| Rename `RegistrarActionsOrders.LatestRegistrarActions` query parameter value from `orderBy[timestamp]=desc` to `orderBy[id]=desc` to accurately reflect the sort column. |
Member
There was a problem hiding this comment.
Please see related comment where I suggested to revert this change.
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
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.
Inconsistent Order of Registrar Actions
closes: #1704
Summary
Why
idjust like we did withrev-share-limitleaderboard building (not thateventTypeis not processed contrary to leaderboard building)Testing
Notes for Reviewer (Optional)
idis sufficient for the deterministic orderingPre-Review Checklist (Blocking)