feat(ensapi): implement Registrar Actions API#1265
Conversation
Also includes related serialization functionality.
Introduces a new HTTP route at `GET /api/registrar-actions/latest` which allows fetching the latest "logical registrar actions", including domain information.
🦋 Changeset detectedLatest commit: 3415c85 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Take domain data model away from registrar actions data model. This help keeping types simple, and still enable clients to get data about registration lifecycle domain in an easy way.
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks for advancing this. Reviewed and shared feedback 👍
| /** | ||
| * Latest Registrar Actions | ||
| */ | ||
| app.get("/latest", async (c) => { |
There was a problem hiding this comment.
| /** | |
| * Latest Registrar Actions | |
| */ | |
| app.get("/latest", async (c) => { | |
| /** | |
| * Get Registrar Actions | |
| */ | |
| app.get("/", async (c) => { |
Idea: Implement this API so that it implicitly defaults to a sort that returns the latest, but isn't explicitly called "latest".
| const MAX_REALTIME_DISTANCE: Duration = 10 * 60; // 10 minutes in seconds | ||
|
|
||
| // inject c.var.isRealtime derived from MAX_REALTIME_DISTANCE | ||
| app.use(makeIsRealtimeMiddleware("registrar-actions-api", MAX_REALTIME_DISTANCE)); | ||
| // 404 if Registrar Actions API routes cannot be served over HTTP | ||
| app.use(requireRegistrarActionsPluginMiddleware()); |
There was a problem hiding this comment.
Am I correct to understand this will mean that if our production ENSNode instance becomes more than 10 minutes beyond realtime this API will start returning 404 errors and therefore cause the landing pages we're building on ENSAwards to break?
We need to fix the strategy here. It's wrong to proactively and FORCEFULLY break client-side apps just because we aren't sufficiently realtime. Client side apps are given all the tools they need to decide for themselves how to handle such a case. Why are we forcing this upon them?
| /** | ||
| * Subname | ||
| * | ||
| * A child name like `sub.name.eth`, whose parent is `name.eth`. | ||
| */ | ||
| subname: InterpretedLabel; |
There was a problem hiding this comment.
Am I correct to assume there's another key idea that's missing in the naming and documentation of this field?
Yes, this is a subname, but more specifically in the context of RegistrationLifecycleDomain isn't it the subname that the registration lifecycle is associated with?
| /** | ||
| * Globally unique, deterministic ID of an indexed onchain event. | ||
| */ | ||
| type RegistrarActionEventId = string; | ||
| export type RegistrarActionEventId = string; |
There was a problem hiding this comment.
The JSDocs suggest this type should be for ANY indexed onchain event, but the name of this type suggests it is only for a specific subset of indexed onchain events (those associated with Registrar Actions).
Which is it?
| registrarActions: RegistrarAction[]; | ||
| registrationLifecycleDomains: Record<Node, RegistrationLifecycleDomain>; |
There was a problem hiding this comment.
We need to refactor the data model here so that each value returned in the registrarActions field should include the fields currently inside RegistrationLifecycleDomain.
The issue here is that we're too tightly connecting the indexed data model (the data model in Postgres) with the data model returned via the API. This is not the correct path.
The data model returned via the API needs to be an abstraction on top of the data model in Postgres that is optimized for API consumption.
The current design here is definitely not optimized for API consumption.
apps/ensapi/src/lib/middleware/require-registrar-actions-plugins..middleware.ts
Show resolved
Hide resolved
apps/ensapi/src/lib/middleware/require-registrar-actions-plugins..middleware.ts
Show resolved
Hide resolved
| ({ registrationLifecycle }) => registrationLifecycle.node, | ||
| ); | ||
| // 2. b) Find the associated Registrar Lifecycle Domain info. | ||
| const registrationLifecycleDomains = |
There was a problem hiding this comment.
Red flag here.
This looks like a single query to this API endpoint is performing N queries to the database. Why?
A single query to this API endpoint should relate to exactly 1 query to the database, where that query performs the necessary joins and where the data model we're building in memory is a distinct abstraction from the data model in the database.
| * @param nodes is array containing a list of nodes. | ||
| * @returns Registration Lifecycle Domains for nodes. | ||
| */ | ||
| export async function findRegistrationLifecycleDomains( |
There was a problem hiding this comment.
This should not be a distinct operation.
The ideas here should be merged into findRegistrarActions such that it performs the appropriate database joins.
We need our in-memory data model to be optimized for use in-memory. Therefore it needs to be a distinct abstraction on top of the data model in the database.
…ponse Clients calling the API will have a complete piece of information about each Registrar Action result handy.
Implement client method for the Registrar Actions API. Also, optimizes the Registrar Actions HTTP route for DB calls. Additionally, allows filterning Registrar Action by subregistry node (which on the client call is refered to as "parent node filter").
…chema to store hex values.
a61c673 to
377cca5
Compare
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Hey great to see the work here. Reviewed and shared feedback 👍
| }; | ||
|
|
||
| /** | ||
| * A status code for indexing status responses. |
There was a problem hiding this comment.
Need to update this comment.
packages/ensnode-sdk/src/client.ts
Outdated
| * | ||
| * const client: ENSNodeClient; | ||
| * | ||
| * // get latest 25 registrar action records across all indexed subregistries |
There was a problem hiding this comment.
Suggest to look for other places where it would be relevant to document how this defaults to 25.
There was a problem hiding this comment.
I think I should replace the concrete count with a reference to the const defined in ENSApi. The const value might change, but the reference will stay.
packages/ensnode-sdk/src/client.ts
Outdated
| try { | ||
| errorResponse = deserializeErrorResponse(responseData); | ||
| } catch { | ||
| // if errorResponse is could not be determined, |
There was a problem hiding this comment.
| // if errorResponse is could not be determined, | |
| // if errorResponse could not be determined, |
| eq(schema.registrationLifecycles.node, schema.subgraph_domain.id), | ||
| ) | ||
| // join Subregistries associated with Registration Lifecycles | ||
| .innerJoin( |
There was a problem hiding this comment.
Should this query only conditionally involve the subregistries table when a filter by subregistry is part of the query?
Please ignore this comment if not relevant.
| timestamp: bigIntToNumber(record.registrarActions.timestamp), | ||
| } satisfies BlockRef; | ||
|
|
||
| // build the result referencing the "logical registrar action" |
There was a problem hiding this comment.
| // build the result referencing the "logical registrar action" | |
| // build the resulting "logical registrar action" |
| */ | ||
| export type RegistrarActionsResponseOk = { | ||
| responseCode: typeof RegistrarActionsResponseCodes.Ok; | ||
| registrarActions: RegistrarActionWithDomain[]; |
There was a problem hiding this comment.
I was really confused trying to unpack all the inheritance in the current data model for RegistrarActionWithDomain.
In my mind the response data model should simply be defined as something like the following.
// NOTE: I see no need for any inheritance at all in our response data model.
export interface NamedRegistrarAction {
// NOTE: `action.lifecycle` is already a `RegistrationLifecycle`. There's
// no need to include multiple copies of a `RegistrationLifecycle` in the response object.
action: RegistrarAction;
// NOTE:
// 1. We don't need a full `RegistrationLifecycleDomain` here, just the `name`
// associated with the `RegistrarAction`. There's no need or benefit of including
// the `subname` field from the `RegistrationLifecycleDomain` here.
// 2. We can simply document an invariant in this object for this `name` field that
// it is guaranteed that `namehash(name)` is equal to `action.registrationLifecycle.node`
// when using an "encoded labelhash aware" implementation of `namehash` such as the
// `namehash` implementation in `viem`.
name: InterpretedName;
}
... and then of course to still wrap an array of this type using a separate RegistrarActionsResponseOk type which would include the responseCode, etc.
| Track all "logical registrar actions" performed by users across multiple | ||
| subregistries with the Registrar Actions API. This endpoint returns a list |
There was a problem hiding this comment.
| Track all "logical registrar actions" performed by users across multiple | |
| subregistries with the Registrar Actions API. This endpoint returns a list | |
| Query the "logical registrar actions" performed on indexed | |
| subregistries with the Registrar Actions API. This endpoint returns a list |
|
|
||
| **Endpoint:** `GET /api/registrar-actions` | ||
| **Endpoint with custom results count limit:** `GET /api/registrar-actions?limit=5` | ||
| **Endpoint with results sourced for the `eth` parent name:** `GET /api/registrar-actions/eth` |
There was a problem hiding this comment.
I thought I read code suggesting this filter is implemented by node, not name?
| * | ||
| * Responds with: | ||
| * - 400 error response for bad input, such as: | ||
| * - (if provided) `parentName` path param points to a name not associated with any indexed subregistries. |
There was a problem hiding this comment.
Please see feedback below suggesting this not be an error.
Update HTTP route for Registrar Actions API to use the `parentNode` param instead of the `parentName` one.
Refine response data model by using `NamedRegistrarAction` type.
Introduce `reinterpretName` helper for client-side validation.
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Hey super updates here 🚀 Shared a few comments and suggestions. Please take the lead to merge when ready 👍
| /** | ||
| * Reinterpret Label | ||
| */ | ||
| export function reinterpretLabel(label: Label | LabelHash): Label | LabelHash { |
There was a problem hiding this comment.
This function should only take in Label values and not LabelHash values.
It also should only return InterpretedLabel values and not LabelHash values.
| // a LabelHash must be encoded | ||
| if (isLabelHash(label)) return encodeLabelHash(label); |
There was a problem hiding this comment.
| // a LabelHash must be encoded | |
| if (isLabelHash(label)) return encodeLabelHash(label); |
This logic should be removed. It is not part of this operation.
| // no change required for NormalizedLabel | ||
| if (isNormalizedLabel(label)) return label; | ||
|
|
||
| // otherwise, label cannot be reinterpreted |
There was a problem hiding this comment.
This needs an update:
- The current logic that throws an error here should be removed.
- We need to add another check here if the label is an empty string. If it is then this is the only case where this function should throw an error. It violates the invariant that the provided label must never be an empty label.
... now, after all the checks above are completed, we know we have an unnormalized label that is non-empty. For this case we need to do the following:
- calculate the labelhash of the (unnormalized) label
- build an encoded labelhash from that labelhash (in other words, wrap it in square brackets)
- return the encoded labelhash you built
| * - If label is a {@link LiteralLabel}, then it was mapped into | ||
| * {@link EncodedLabelHash}. | ||
| */ | ||
| export type ReinterpretedLabel = Label | EncodedLabelHash; |
There was a problem hiding this comment.
This type alias can be removed. It doesn't need to exist. The goal of reinterpretLabel is it returns the existing InterpretedLabel type.
| * | ||
| * A Name built only from {@link ReinterpretedLabel}s. | ||
| */ | ||
| export type ReinterpretedName = Name; |
There was a problem hiding this comment.
This type alias can be removed. It doesn't need to exist. The goal of reinterpretName is it returns the existing InterpretedName type.
| * - `namehash(name)` is always `action.registrationLifecycle.node`. | ||
| */ | ||
| registrationLifecycle: RegistrationLifecycleWithDomain; | ||
| name: ReinterpretedName; |
There was a problem hiding this comment.
| name: ReinterpretedName; | |
| name: InterpretedName; |
| value: subregistryNode, | ||
| }; | ||
| } | ||
| const MAX_RESPONSE_ITEMS_COUNT_LIMIT = 1000; |
There was a problem hiding this comment.
Maybe better to make it only 100?
| * | ||
| * Allows the ENSNode Client to reinterpret Names returned from ENSApi. | ||
| */ | ||
| export function reinterpretName(name: Name): ReinterpretedName { |
There was a problem hiding this comment.
| export function reinterpretName(name: Name): ReinterpretedName { | |
| export function reinterpretName(name: InterpretedName): InterpretedName { |
| /** | ||
| * Build a "parent node" filter object for Registrar Actions query. | ||
| */ | ||
| byParentNode(parentNode: Node | undefined): RegistrarActionsFilter | undefined { |
There was a problem hiding this comment.
Thanks for aligning if this query param was by name or node.
I continue to believe the query param should actually be the AccountId of a subregistry (a subregistryId).
Reasons for this include:
- The data model in the database for registrar actions stores an associated subregistryId rather than name or node.
- The subregistry for a name (or node) can change across time. For example when ENS made their registry migration the subregistryId for Ethnames changed. Let's imagine this happens again for some reason. This would mean the subregistryId for Ethnames would change again and therefore suddenly all RegistrarActions returned by an API call passing in parent name / node X would suddenly disappear.
- Please also have a look at the work Mykola is advancing in PR 1239: feat: introduce ENSAnalytics API into ENSApi #1239 Please note how the API he's working on there builds the in-memory cache of aggregated referrer metrics based on a specific subregistryId, not on a name or node. It would be nice if we align related logic across what both of you guys are working on.
What do you think?
There was a problem hiding this comment.
If AccountId is the ID we'd like to use, I'm ok with having it implemented 👍
There was a problem hiding this comment.
@lightwalker-eth deeper thinking, my suggestion is to continue using the parent node filter for registrar actions, as it feels like a perfect balance between user experience (namehash of a particular name) and our current data model constraints (unique node values across all subregistries.
Update `reinterpretName` implementation
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Hey thanks for your updates on this PR. Here's some feedback that would be good to refine in your next PR. Thanks 👍
| export function reinterpretName(name: Name): ReinterpretedName { | ||
| if (name === "") return name; | ||
| export function reinterpretName(name: InterpretedName): InterpretedName { | ||
| if (name === "") return name as InterpretedName; |
There was a problem hiding this comment.
name is already an InterpretedName, therefore should be no need for "as ..." here.
| export type ReinterpretedName = Name; | ||
|
|
||
| /** | ||
| * Reinterpret Label |
There was a problem hiding this comment.
We should document here that:
* Reinterprets {@link InterpretedLabel} values received by "External System"
* into {@link InterpretedLabel} values in "Internal System" where:
* 1. "External System" is not guaranteed to be using the same ENSNormalize
* version as "Internal System", and therefore the `InterpretedLabel` passed into
* this function (from "External System") is not guaranteed to 100% align
* with the invariants of an `InterpretedLabel` in "Internal System".
* 2. The `InterpretedLabel` returned by this function is guaranteed to match
* the invariants of `InterpretedLabel` in "Internal System".
| * Reinterpret Label | ||
| * | ||
| * @returns reinterpreted label. | ||
| * @throws an error when the provided label must never be an empty label. |
There was a problem hiding this comment.
"@throws an error if the provided label is an empty label (and therefore violates the invariants of an InterpretedLabel)."
|
|
||
| // no change required for EncodedLabelHash | ||
| if (isEncodedLabelHash(label)) return label; | ||
| export function reinterpretLabel(label: Label): InterpretedLabel { |
There was a problem hiding this comment.
The param for this function should change from Label to InterpretedLabel.
| // no change required for EncodedLabelHash | ||
| if (isEncodedLabelHash(label)) return label; | ||
| export function reinterpretLabel(label: Label): InterpretedLabel { | ||
| // Invariant: the provided label must never be an empty label. |
There was a problem hiding this comment.
"Invariant: InterpretedLabel values must never be an empty label."
|
|
||
| // no change required for NormalizedLabel | ||
| if (isNormalizedLabel(label)) return label; | ||
| if (isNormalizedLabel(label)) return label as InterpretedLabel; |
There was a problem hiding this comment.
Should be no need for "as InterpretedLabel" here.
| export function reinterpretName(name: Name): ReinterpretedName { | ||
| if (name === "") return name; | ||
| export function reinterpretName(name: InterpretedName): InterpretedName { | ||
| if (name === "") return name as InterpretedName; |
There was a problem hiding this comment.
I'm not 100% confdent on how the split / join operations work on an empty string input. But possibly we don't need this explicit check for the empty string.
| * Reinterprets {@link InterpretedName} values received by "External System" | ||
| * into {@link InterpretedName} values in "Internal System" where: | ||
| * 1. "External System" is not guaranteed to be using the same ENSNormalize | ||
| * version as "Internal System", and therefore the `Name` passed into |
There was a problem hiding this comment.
Name in this sentence should change to InterpretedName
I made this typo in my earlier PR feedback.
| if (label === "") { | ||
| throw new Error(`The label must not be an empty string to be reinterpreted.`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ah there's a key issue here! We must keep the check here if the label is an encoded labelhash. If it is, it needs to be returned without changes.
| * 3) The omnichain indexing status of the connected ENSIndexer is other than | ||
| * 2) ENSApi has not yet successfully cached the Indexing Status in memory from | ||
| * the connected ENSIndexer. | ||
| * 3) The omnichain indexing status of the connected ENSIndexer is not |
There was a problem hiding this comment.
"The omnichain indexing status of the connected ENSIndexer that is cached in memory is not"
Above adds "... that is cached in memory ..."
Co-authored-by: lightwalker.eth <126201998+lightwalker-eth@users.noreply.github.com>
Scope
Sample response
{ "responseCode": "ok", "registrarActions": [ { "id": "176279354400000000111551110000000009601035000000000000012750000000000000184", "type": "registration", "incrementalDuration": 31536000, "registrant": "0x168a4ac73c7d8ac6d1cd56a36d94a4d91825dc68", "registrationLifecycle": { "subregistry": { "subregistryId": "eip155:11155111:0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85", "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0x5d54aa7328d700144737df21f6ea5d211b84331fa848195bcb4f9853c0b6d1a0", "expiresAt": 1794329544, "domain": { "subname": "undersystem", "name": "undersystem.eth" } }, "pricing": { "baseCost": { "currency": "ETH", "amount": "3125000000003490" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "3125000000003490" } }, "referral": { "encodedReferrer": null, "decodedReferrer": null }, "block": { "number": 9601035, "timestamp": 1762793544 }, "transactionHash": "0x08aa690fd7baab1e815da63773cbda3780f4414ef362aae6dbcf053c594fe808", "eventIds": [ "176279354400000000111551110000000009601035000000000000012750000000000000184", "176279354400000000111551110000000009601035000000000000012750000000000000196" ] } ] }Sample client calls
Suggested review order
ENSNode SDK
sharedmodule updatespackages/ensnode-sdk/src/shared/*registrarmodule updatespackages/ensnode-sdk/src/registrars/*apimodule updatespackages/ensnode-sdk/src/api/*clientmodule updateregistrarActions()method was introduced, allows filtering by parent nameENSNode Schema
registrarschema updatespackages/ensnode-schema/src/schemas/registrars.schema.tsENSApi
libmodule updatesapps/ensapi/src/lib/*handlersmodule updatesapps/ensapi/src/handlers/registrar-actions-api.tsReplaces:
Resolves: