feat: add ENSv2 BENS subgraph - #3
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new subgraph adapter for the DOS Name Service (.dos), an ENSv2 fork on the DOS Chain compatible with Blockscout BENS. The review feedback highlights several key improvement opportunities: consistently using the namehash (node) as the ID for Registration entities to simplify lookup logic and prevent subdomain ID collisions; removing redundant Domain.load database reads in subregistry update handlers; cleaning up dead or redundant utility code (such as the custom byteArrayFromHex in favor of the built-in ByteArray.fromHexString); ensuring referential integrity by creating the Account entity for the empty address; and updating the test suite to reflect these entity ID changes.
| // Create Registration entity (id = labelHash hex) | ||
| let registration = new Registration(labelHash.toHexString()); |
There was a problem hiding this comment.
The Registration entity is created here using labelHash.toHexString() as its ID, whereas in userRegistry.ts it is created using node (the namehash). This inconsistency can lead to load failures and requires complex null checks on domain.labelhash when loading registrations. Furthermore, using labelHash as the ID can cause collisions if subdomains in different registries share the same label (e.g., test.dos and test.alice.dos). Consistently using node (the namehash) as the Registration ID everywhere solves these issues and simplifies the code.
| // Create Registration entity (id = labelHash hex) | |
| let registration = new Registration(labelHash.toHexString()); | |
| // Create Registration entity (id = node namehash) | |
| let registration = new Registration(node); |
| if (domain.labelhash !== null) { | ||
| let registration = Registration.load(domain.labelhash!.toHexString()); | ||
| if (registration !== null) { | ||
| registration.expiryDate = newExpiry; | ||
| registration.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (domain.labelhash !== null) { | ||
| let registration = Registration.load(domain.labelhash!.toHexString()); | ||
| if (registration !== null) { | ||
| registration.registrant = account.id; | ||
| registration.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (domain.labelhash !== null) { | ||
| let registration = Registration.load(domain.labelhash!.toHexString()); | ||
| if (registration !== null) { | ||
| registration.registrant = zeroAccount.id; | ||
| registration.expiryDate = event.block.timestamp; | ||
| registration.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since Registration ID is now consistently set to node (the namehash), we can load it directly using mapping.domain without checking domain.labelhash.
let registration = Registration.load(mapping.domain);
if (registration !== null) {
registration.registrant = zeroAccount.id;
registration.expiryDate = event.block.timestamp;
registration.save();
}| // Update registration with cost | ||
| if (domain.labelhash !== null) { | ||
| let registration = Registration.load(domain.labelhash!.toHexString()); | ||
| if (registration !== null) { | ||
| registration.labelName = label; | ||
| registration.cost = cost; | ||
| registration.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since Registration ID is now consistently set to node (the namehash), we can load it directly using node without checking domain.labelhash.
// Update registration with cost
let registration = Registration.load(node);
if (registration !== null) {
registration.labelName = label;
registration.cost = cost;
registration.save();
}| export function byteArrayFromHex(s: string): ByteArray { | ||
| if (s.length % 2 !== 0) { | ||
| throw new TypeError("Hex string must have an even number of characters"); | ||
| } | ||
| let out = new Uint8Array(s.length / 2); | ||
| for (var i = 0; i < s.length; i += 2) { | ||
| out[i / 2] = parseInt(s.substring(i, i + 2), 16) as u32; | ||
| } | ||
| return changetype<ByteArray>(out); | ||
| } |
| var dosNode: ByteArray = byteArrayFromHex( | ||
| DOS_NODE.slice(2) // strip 0x prefix | ||
| ); |
| export function createOrLoadDomain(node: string): Domain { | ||
| let domain = Domain.load(node); | ||
| if (domain == null) { | ||
| domain = new Domain(node); | ||
| domain.owner = EMPTY_ADDRESS; | ||
| domain.isMigrated = true; | ||
| domain.createdAt = BigInt.fromI32(0); | ||
| domain.subdomainCount = 0; | ||
| domain.storedOffchain = false; | ||
| domain.resolvedWithWildcard = false; | ||
| domain.save(); | ||
| } | ||
| return domain; | ||
| } |
There was a problem hiding this comment.
In createOrLoadDomain, domain.owner is set to EMPTY_ADDRESS. However, the corresponding Account entity for EMPTY_ADDRESS is not created or loaded. This can lead to broken references or referential integrity issues when querying the domain's owner. Ensuring that the Account entity is created/loaded via createOrLoadAccount(EMPTY_ADDRESS) resolves this.
| export function createOrLoadDomain(node: string): Domain { | |
| let domain = Domain.load(node); | |
| if (domain == null) { | |
| domain = new Domain(node); | |
| domain.owner = EMPTY_ADDRESS; | |
| domain.isMigrated = true; | |
| domain.createdAt = BigInt.fromI32(0); | |
| domain.subdomainCount = 0; | |
| domain.storedOffchain = false; | |
| domain.resolvedWithWildcard = false; | |
| domain.save(); | |
| } | |
| return domain; | |
| } | |
| export function createOrLoadDomain(node: string): Domain { | |
| let domain = Domain.load(node); | |
| if (domain == null) { | |
| domain = new Domain(node); | |
| let zeroAccount = createOrLoadAccount(EMPTY_ADDRESS); | |
| domain.owner = zeroAccount.id; | |
| domain.isMigrated = true; | |
| domain.createdAt = BigInt.fromI32(0); | |
| domain.subdomainCount = 0; | |
| domain.storedOffchain = false; | |
| domain.resolvedWithWildcard = false; | |
| domain.save(); | |
| } | |
| return domain; | |
| } |
| handleNameRegisteredByRegistrar(registrarRegistration(123)); | ||
|
|
||
| assert.fieldEquals("Domain", ALICE_DOS, "name", "alice.dos"); | ||
| assert.fieldEquals("Registration", LABEL_HASH, "cost", "12"); |
There was a problem hiding this comment.
| assert.fieldEquals("Registration", LABEL_HASH, "cost", "7"); | ||
| assert.fieldEquals("Registration", LABEL_HASH, "expiryDate", "2100000000"); |
There was a problem hiding this comment.
Since the Registration entity ID is now consistently set to node (the namehash, which is ALICE_DOS), we should assert on ALICE_DOS instead of LABEL_HASH.
| assert.fieldEquals("Registration", LABEL_HASH, "cost", "7"); | |
| assert.fieldEquals("Registration", LABEL_HASH, "expiryDate", "2100000000"); | |
| assert.fieldEquals("Registration", ALICE_DOS, "cost", "7"); | |
| assert.fieldEquals("Registration", ALICE_DOS, "expiryDate", "2100000000"); |
Summary
Validation
Scope
The custom indexing adapter stays in DOS Names. DOScan consumes it by immutable commit and runs only official BENS, Graph Node, IPFS, and PostgreSQL images.