fix(registration): use unique registration ID #82

tk-o · 2025-01-26T21:30:58Z

This PR fixes a primary key constraint issue on registrations table in database. All names registered for plugins, except for the eth plugin, will use node value (namehash function output) for registration ID. The eth plugin will continue using registration IDs based of the label value only.

Before this change, all registrations would have their respective ID derived from the label only. Which means that there would be multiple names competing for the same registration (i.e. www.vitalik.eth & www.vitalik.linea.eth). In consequence, subgraph-compatible indexing logic would assume that for an existing (conflicting) registration entity, there must be a related domain entity.

That was not always the case, and the runtime error would occur. Not anymore.

Fixes:

lightwalker-eth

@tk-o Hey super effort to advance this on the weekend 💪 Much appreciated! Reviewed and shared some ideas with feedback 😄

lightwalker-eth · 2025-01-26T21:59:28Z

packages/ensnode-utils/src/subname-helpers.ts

+      // handled by `eth` plugin
+      labelhash
+    : // For other plugins, we use the namehash to ensure uniqueness
+      namehash;


Collisions between labelhash and namehash should be quite unlikely, but we should make a stronger 100% guarantee that collisions are impossible. Ex: Return a concatenation of ownedName, a symbol such as ":" (or whatever you suggest), and namehash.

Part of the motivation for including ownedName in this key is to protect against a plugin writing names to their shadow registry outside the scope of names they are expected to manage.

For example, consider the case that the base.eth plugin emits events related to the registration of example.linea.eth or example.eth.

In fact, suggest that we take this opportunity to add some validation checks on both the base.eth and linea.eth plugins to validate that they only ever index names that are subnames of their respective ownedName.

It's ok for those to be subnames of subnames of subnames, etc.. But ultimately the ownedName must be the root of all subnames added to the shadow registry associated with those extensions.

If this change introduces too much scope for this existing PR 82, please feel welcome to create a separate issue for us to track this goal. Thanks.

personally i think collisions can be safely ignored given that they're keccak hashes. no known collision has ever been discovered and if such an event occurs i don't think anyone would say ensnode is at fault for not handling that case.

re: validation — it seems safe to throw that validation logic in, but i question the need for it, since we do have verified contract source. perhaps that may not be true in the future, if we integrate a malicious registrar? but it doesn't seem like a likely threat at the moment. lmk if i'm off here. regardless should definitely be a separate issue

ps. sadly a concatenation also isn't as trivial because of the Hex type of the field, it may break some assumptions within ponder if we commit non-hex data, even if it is represented as TEXT in postgres. sticking with label or node is efficient, clear, and causes 0 likely problems, so i think it's a perfect solution.

lightwalker-eth · 2025-01-26T22:00:03Z

packages/ensnode-utils/src/subname-helpers.ts

+// Makes a cross-chain unique registration ID for a given owned name.
+export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>
+  ownedName === "eth"
+    ? // To keep subgraph compatibility, we still use labelhash for registrations


Suggested change

? // To keep subgraph compatibility, we still use labelhash for registrations

? // To keep subgraph compatibility, we still use labelhash for RegistrationIds

lightwalker-eth · 2025-01-26T22:12:22Z

packages/ensnode-utils/src/subname-helpers.ts

@@ -11,6 +11,15 @@ export const makeSubnodeNamehash = (node: Hex, label: Hex) => keccak256(concat([
 // converts uint256-encoded nodes to hex
 export const tokenIdToLabel = (tokenId: bigint) => toHex(tokenId, { size: 32 });

+// Makes a cross-chain unique registration ID for a given owned name.
+export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>


Suggested change

export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>

export const makeRegistrationId = (ownedName: string, labelhash: Hex, node: Hex) =>

"namehash" refers to the function, while "node" returns to the return value of that function. There should be detailed documentation for this param, ex: that this is the node of the name that was registered.

Please also refine the following:

Please add detailed documentation for ownedName and what this represents. Ex: It is the ownedName of the ENSNode plugin that is processing the registration event.

Please add detailed documentation for labelhash and what this represents. For example, it needs to be 100% clear what this is the labelhash of. Ex: Is it the labelhash of the direct subname of ownedName? What if the registration is more than 1 level of subname beneath ownedName? What if labelhash has no child relationship at all with ownedName? For example: Imagine the "eth" plugin where a name such as tko.box is explicitly added to the ENS Registry. Exactly what is labelhash the labelhash of in this case? The same question if the name abc.123.456.tko.box was explicitly added to the ENS Registry under the "eth" plugin. I'm assuming that this is the labelhash of the subname being registered, but that we also need to make it clear that the subname being registered could be any subname at any level within the scope of registrations indexed by the ENSNode plugin associated with ownedName. It should also be noted how the "eth" plugin can theoretically index all possible names given the NameWrapper.

Please also note how a source of some of the complexity here is how this makeRegistrationId utility function is in a totally generic "subname-helpers.ts" file, which suggests that the logic inside of it should apply universally in all cases. This is a key reason for some of the feedback above.

Alternatively, it could be considered to only put this function in apps/ensnode/src/handlers/Registrar.ts and NOT export it. It would still need careful documentation, but perhaps a bit less, since as I understand it would only apply to direct subnames of the ownedName of the associated plugin.

Advice appreciated 👍

i support this helper living in ids.ts

i also think the included documentation is mostly acceptable, perhaps some additional context at the top is warranted like:

a Registration record's id is historically the labelhash of the label directly under the name the Registrar manages (i.e. for a registration of test.eth, a Registration's id is labelhash('test')). Because the original subgraph implementation didn't consider indexing any Registrars besides the .eth Registrar, this leaves no room in the namespace for Registration records from other Registrars (like the .base.eth and .linea.eth Registrars). To account for this, while preserving backwards compatibility, Registration records created for the .eth Registrar use label as id and those created by any other Registrar use node (i.e. namehash(test.base.eth) to avoid collisions that would otherwise occur.

i think the inline comments are excellent, though perhaps unnecessary if the larger comment on the helper looks like my suggestion above

apps/ensnode/src/handlers/Registrar.ts

shrugs · 2025-01-27T05:07:40Z

packages/ensnode-utils/src/subname-helpers.ts

@@ -11,6 +11,15 @@ export const makeSubnodeNamehash = (node: Hex, label: Hex) => keccak256(concat([
 // converts uint256-encoded nodes to hex
 export const tokenIdToLabel = (tokenId: bigint) => toHex(tokenId, { size: 32 });

+// Makes a cross-chain unique registration ID for a given owned name.
+export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>


i support this helper living in ids.ts

shrugs · 2025-01-27T05:15:02Z

packages/ensnode-utils/src/subname-helpers.ts

+      // handled by `eth` plugin
+      labelhash
+    : // For other plugins, we use the namehash to ensure uniqueness
+      namehash;


personally i think collisions can be safely ignored given that they're keccak hashes. no known collision has ever been discovered and if such an event occurs i don't think anyone would say ensnode is at fault for not handling that case.

re: validation — it seems safe to throw that validation logic in, but i question the need for it, since we do have verified contract source. perhaps that may not be true in the future, if we integrate a malicious registrar? but it doesn't seem like a likely threat at the moment. lmk if i'm off here. regardless should definitely be a separate issue

ps. sadly a concatenation also isn't as trivial because of the Hex type of the field, it may break some assumptions within ponder if we commit non-hex data, even if it is represented as TEXT in postgres. sticking with label or node is efficient, clear, and causes 0 likely problems, so i think it's a perfect solution.

shrugs · 2025-01-27T05:20:02Z

packages/ensnode-utils/src/subname-helpers.ts

@@ -11,6 +11,15 @@ export const makeSubnodeNamehash = (node: Hex, label: Hex) => keccak256(concat([
 // converts uint256-encoded nodes to hex
 export const tokenIdToLabel = (tokenId: bigint) => toHex(tokenId, { size: 32 });

+// Makes a cross-chain unique registration ID for a given owned name.
+export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>


i also think the included documentation is mostly acceptable, perhaps some additional context at the top is warranted like:

a Registration record's id is historically the labelhash of the label directly under the name the Registrar manages (i.e. for a registration of test.eth, a Registration's id is labelhash('test')). Because the original subgraph implementation didn't consider indexing any Registrars besides the .eth Registrar, this leaves no room in the namespace for Registration records from other Registrars (like the .base.eth and .linea.eth Registrars). To account for this, while preserving backwards compatibility, Registration records created for the .eth Registrar use label as id and those created by any other Registrar use node (i.e. namehash(test.base.eth) to avoid collisions that would otherwise occur.

i think the inline comments are excellent, though perhaps unnecessary if the larger comment on the helper looks like my suggestion above

Allows naming arguments more precisely with primitive types and doc comments attached to them

shrugs

nice

shrugs · 2025-01-27T19:31:31Z

apps/ensnode/src/lib/primitives.ts

@@ -0,0 +1,31 @@
+import type { Hex } from "viem";


this is nice actually, good idea! a nitpick on filename: maybe ens-types.ts? or i've just used just types.ts in the past but maybe too generic.

I've had it as types, but primitives also worked in my mind. However, let's stick to common naming standards for TS, and go with just types for the time being.

rename `primitives.ts` to `types.ts`

lightwalker-eth

@tk-o Hey nice work here. Reviewed and shared feedback 👍

lightwalker-eth · 2025-01-28T08:02:51Z

apps/ensnode/src/lib/types.ts

+ * ```
+ * @link https://docs.ens.domains/ensip/1#namehash-algorithm
+ */
+export type Node = Hex;


These are great! Suggest moving the definition of things that will likely be shared across different layers of our project, such as Node and Labelhash into the ensnode-utils package. Open to doing that only when required though. Appreciate your advice.

lightwalker-eth · 2025-01-28T08:11:52Z

apps/ensnode/src/lib/ids.ts

@@ -10,3 +11,24 @@ export const makeEventId = (blockNumber: bigint, logIndex: number, transferIndex
  [blockNumber.toString(), logIndex.toString(), transferIndex?.toString()]
    .filter(Boolean)
    .join("-");
+
+/**
+ * Makes a cross-chain unique registration ID.


Suggested change

* Makes a cross-chain unique registration ID.

* Makes a cross-registrar unique registration ID.

The key to the logic here is making it so registration IDs will be unique across Registrars. It wouldn't matter if those Registrars are on the same chain or not.

lightwalker-eth · 2025-01-28T08:49:52Z

apps/ensnode/test/ids.spec.ts

@@ -17,4 +17,18 @@ describe("ids", () => {
      expect(makeEventId(123n, 456)).toEqual("123-456");
    });
  });
+
+  describe("makeRegistrationId", () => {
+    it("should use label when ownedName is exactly `eth` to ensure subgraph compatibility", () => {


Suggested change

it("should use label when ownedName is exactly `eth` to ensure subgraph compatibility", () => {

it("should use labelhash when ownedName is `eth` to ensure subgraph compatibility", () => {

lightwalker-eth · 2025-01-28T08:50:26Z

apps/ensnode/test/ids.spec.ts

+      );
+    });
+
+    it("should use node when ownedName is not exactly `eth`", () => {


Suggested change

it("should use node when ownedName is not exactly `eth`", () => {

it("should use node when ownedName is not `eth`", () => {

lightwalker-eth · 2025-01-28T11:07:20Z

apps/ensnode/src/lib/ids.ts

+ * @param label registration's label
+ * @param node registration's node
+ */
+export const makeRegistrationId = (ownedName: OwnedName, label: Labelhash, node: Node) =>


Suggested change

export const makeRegistrationId = (ownedName: OwnedName, label: Labelhash, node: Node) =>

export const makeRegistrationId = (ownedName: OwnedName, labelHash: Labelhash, node: Node) =>

Let's please always be specific in the differences between a label and a labelHash, both as types and as variable names.

Lots of issues and mistakes are more likely to happen otherwise.

based on my understanding of the terminology, a nitpick: labelhash over labelHash for these variables and typenames

@shrugs Worry here isn't for capitalization. Instead the issue here is I've noticed several places in the code where we are using "label" where we should instead be using "labelHash". There's an important distinction there.

lightwalker-eth · 2025-01-28T11:08:24Z

apps/ensnode/src/lib/ids.ts

+ * collisions that would otherwise occur.
+ *
+ * @param ownedName the ownedName of the ENSNode plugin that is processing the registration event
+ * @param label registration's label


Suggested change

* @param label registration's label

* @param labelHash the labelHash of the direct subname of `registrarName` that was registered

Please note here how the definition of this param has a relation to registrarName.

lightwalker-eth · 2025-01-28T11:09:16Z

apps/ensnode/src/lib/ids.ts

+ *
+ * @param ownedName the ownedName of the ENSNode plugin that is processing the registration event
+ * @param label registration's label
+ * @param node registration's node


Suggested change

* @param node registration's node

* @param node the node of the full name that was registered

lightwalker-eth · 2025-01-28T11:12:55Z

apps/ensnode/src/lib/ids.ts

+ * any other Registrar use node (i.e. namehash(test.base.eth) to avoid
+ * collisions that would otherwise occur.
+ *
+ * @param ownedName the ownedName of the ENSNode plugin that is processing the registration event


Suggested change

* @param ownedName the ownedName of the ENSNode plugin that is processing the registration event

* @param registrarName the name of the registrar issuing the registration

Also suggest changing the type of registrarName from OwnedName to just a regular string. Otherwise we're mixing up the concept of a registrar's name from a plugin's name. For more details please see the issues I flagged inside #87

lightwalker-eth · 2025-01-28T11:41:49Z

apps/ensnode/src/lib/ids.ts

+/**
+ * Makes a cross-chain unique registration ID.
+ *
+ * A Registration record's id is historically the labelhash of the label


Really appreciate the efforts here! This is good and helps a lot 💪 I had a few suggestions while reviewing this. I thought it would be more efficient to share this feedback in a separate PR for you to review: #88

lightwalker-eth

@tk-o Thanks for updates. A few follow ups.

lightwalker-eth · 2025-01-28T14:34:29Z

apps/ensnode/test/ids.spec.ts

@@ -17,4 +17,18 @@ describe("ids", () => {
      expect(makeEventId(123n, 456)).toEqual("123-456");
    });
  });
+
+  describe("makeRegistrationId", () => {
+    it("should use labelhash when ownedName is `eth` to ensure subgraph compatibility", () => {


Suggest we change the text here now that the ownedName param is renamed

lightwalker-eth · 2025-01-28T14:34:49Z

apps/ensnode/test/ids.spec.ts

+      );
+    });
+
+    it("should use node when ownedName is not `eth`", () => {


Please see related comment above

lightwalker-eth · 2025-01-28T14:37:18Z

apps/ensnode/src/lib/ids.ts

+ * the .eth Registrar, this leaves no room in the namespace for Registration
+ * records from other Registrars (like the base.eth and linea.eth Registrars).
+ * To account for this, while preserving backwards compatibility, Registration
+ * records created for the .eth Registrar use label as id and those created by


lightwalker-eth · 2025-01-28T14:39:13Z

apps/ensnode/src/lib/ids.ts

+ * @param node the node of the full name that was registered
+ */
+export const makeRegistrationId = (registrarName: string, labelHash: Labelhash, node: Node) =>
+  registrarName === "eth" ? labelHash : node;


Prefer to keep the inline comments I proposed in the separate PR. Ex: It matters that we explicitly defend details of how this function is implemented.

Proposes refined documentation for makeRegistrationId.

…-id-unique

lightwalker-eth

@tk-o Looks great! Approved 🚀

fix(registration): use unique registration ID

d7424cf

tk-o requested a review from a team as a code owner January 26, 2025 21:30

lightwalker-eth requested changes Jan 26, 2025

View reviewed changes

shrugs approved these changes Jan 27, 2025

View reviewed changes

refactor: use primitive types

953cab5

Allows naming arguments more precisely with primitive types and doc comments attached to them

shrugs approved these changes Jan 27, 2025

View reviewed changes

fix: apply pr feedback

0296122

rename `primitives.ts` to `types.ts`

This was referenced Jan 28, 2025

Use more specific types #86

Open

Refine the OwnedName concept #87

Open

lightwalker-eth requested changes Jan 28, 2025

View reviewed changes

fix: apply pr feedback

7cb6fc2

tk-o force-pushed the fix/make-registration-id-unique branch from 0386066 to 7cb6fc2 Compare January 28, 2025 13:58

lightwalker-eth requested changes Jan 28, 2025

View reviewed changes

tk-o and others added 3 commits January 28, 2025 15:59

fix: apply pr feedback

ca825c3

Refine docs for makeRegistrationId (#88)

a63db45

Proposes refined documentation for makeRegistrationId.

Merge remote-tracking branch 'origin/main' into fix/make-registration…

4f775ff

…-id-unique

tk-o force-pushed the fix/make-registration-id-unique branch from f1254dc to 4f775ff Compare January 28, 2025 18:17

lightwalker-eth approved these changes Jan 28, 2025

View reviewed changes

lightwalker-eth merged commit bdfcd08 into main Jan 28, 2025
2 checks passed

lightwalker-eth deleted the fix/make-registration-id-unique branch January 28, 2025 20:33

This was referenced Jan 31, 2025

Missing domain error #70

Closed

Registration not found error #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(registration): use unique registration ID #82

fix(registration): use unique registration ID #82

tk-o commented Jan 26, 2025 •

edited

Loading

lightwalker-eth left a comment

lightwalker-eth Jan 26, 2025

shrugs Jan 27, 2025

lightwalker-eth Jan 26, 2025

lightwalker-eth Jan 26, 2025

shrugs Jan 27, 2025

shrugs Jan 27, 2025

shrugs Jan 27, 2025

shrugs Jan 27, 2025

shrugs Jan 27, 2025

shrugs left a comment

shrugs Jan 27, 2025

tk-o Jan 27, 2025

lightwalker-eth left a comment

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

shrugs Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth left a comment

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth Jan 28, 2025

lightwalker-eth left a comment

	? // To keep subgraph compatibility, we still use labelhash for registrations
	? // To keep subgraph compatibility, we still use labelhash for RegistrationIds

	export const makeRegistrationId = (ownedName: string, labelhash: Hex, namehash: Hex) =>
	export const makeRegistrationId = (ownedName: string, labelhash: Hex, node: Hex) =>

	* Makes a cross-chain unique registration ID.
	* Makes a cross-registrar unique registration ID.

	it("should use label when ownedName is exactly `eth` to ensure subgraph compatibility", () => {
	it("should use labelhash when ownedName is `eth` to ensure subgraph compatibility", () => {

	it("should use node when ownedName is not exactly `eth`", () => {
	it("should use node when ownedName is not `eth`", () => {

	* @param label registration's label
	* @param labelHash the labelHash of the direct subname of `registrarName` that was registered

	* @param node registration's node
	* @param node the node of the full name that was registered

	* @param ownedName the ownedName of the ENSNode plugin that is processing the registration event
	* @param registrarName the name of the registrar issuing the registration

fix(registration): use unique registration ID #82

fix(registration): use unique registration ID #82

Conversation

tk-o commented Jan 26, 2025 • edited Loading

lightwalker-eth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightwalker-eth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightwalker-eth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightwalker-eth left a comment

Choose a reason for hiding this comment

tk-o commented Jan 26, 2025 •

edited

Loading